Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the documentation for Tempfile.create and recommend Tempfile.open instead #5

Merged
merged 1 commit into from
Oct 5, 2020

Conversation

eregon
Copy link
Member

@eregon eregon commented Sep 25, 2020

See https://bugs.ruby-lang.org/issues/17144
Follow-up of #4

My understanding is nobody should use Tempfile.open and it only exists for compatibility.

Probably we should consider issuing a deprecation warning (will file an issue on https://bugs.ruby-lang.org for that).

cc @hsbt @akr @jeremyevans

@eregon
Copy link
Member Author

eregon commented Sep 25, 2020

BTW I was trying to see if finalizers always run before the process exits, and it seems not even on CRuby:

Or maybe there is something wrong with my usage of a finalizer?

$ ruby -e 'def bye; STDERR.puts "finalizing"; end; Object.new.tap { |o| ObjectSpace.define_finalizer(o, method(:bye)) }; p :done'
:done

=> the finalizer callable needs to accept one argument, which is the object id

$ ruby -e 'def bye(objid); STDERR.puts "finalizing #{objid}"; end; Object.new.tap { |o| ObjectSpace.define_finalizer(o, method(:bye))}; p :done;'       
:done
finalizing 47163596383240

And when using exit! the finalizer is not called:

$ ruby -e 'def bye(objid); STDERR.puts "finalizing #{objid}"; end; Object.new.tap { |o| ObjectSpace.define_finalizer(o, method(:bye))}; p :done; exit!'
:done

My impression is finalizers are not reliable at all, and might not even run. And I think this is even more the case on TruffleRuby/JRuby. On TruffleRuby we tried to run finalizers for live objects on exit, but that failed in various ways because finalizer procs can depend on each other and then the order to run them matters.

Also considering that CRuby uses a conservative GC, it's not impossible for a random (e.g., long) value on stack to accidentally keep an unrelated (except that object has the same bits for its address) Ruby object alive.

@akr
Copy link
Contributor

akr commented Sep 25, 2020

Tempfile.open would be useful when a user want to remove the file by GC.
Tempfile.create doesn't provide the feature, intentionally.

exit! is required with fork method. Tempfile.open(); fork { ...; exit! }
It is not desirable that a finalizer is invoked at both child process and parent process.

As far as a user needs temporary file removal by GC, we should not deprecate Tempfile.open
(unless we provide an alternative).

@eregon
Copy link
Member Author

eregon commented Sep 26, 2020

@akr I think there is no case where someone actually wants to remove the file by GC, it's undeterministic, leads to weird CI transient failures (that's why both test-all and test-spec explicitly check against it) and generally unreliable. Same thing as relying on File.new being closed by GC, it's a bad idea (and can run out of open fds).

But anyway, this PR does not deprecate the method, only recommends Tempfile.create instead and explains why.
Are you OK with this documentation change and can you approve it? If not could you point out what should be changed in the diff?

@akr
Copy link
Contributor

akr commented Sep 26, 2020

open-uri without block can return a tempfile.
cgi uses tempfile for multipart in params.

GC based tempfile removal is actually used.

So, the logic, discourage Tempfile.open because GC based tempfile removal is not used, is too naive.

@eregon
Copy link
Member Author

eregon commented Sep 26, 2020

@akr

So, the logic, discourage Tempfile.open because GC based tempfile removal is not used, is too naive.

Not because it's not used, but because it's unreliable, inefficient and very rarely needed.
I think we both agree Tempfile.create is preferable whenever it's possible to use it, right?

#
# Tempfile.open is still appropriate if you need the Tempfile to be unlinked
# by a finalizer and you cannot explicitly know where in the program the
# Tempfile can be unlinked safely.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akr I added this paragraph and changed above exists only => exists mostly. Sounds good?

@akr
Copy link
Contributor

akr commented Sep 27, 2020

It seems fine.

I found that the document of Tempfile.new is also problematic, though.
(It doesn't mention about file removal.)

@eregon
Copy link
Member Author

eregon commented Oct 2, 2020

@akr I added a note for Tempfile.new and also in the documentation of the Tempfile class.

I hope this is ready now, even if not perfect I think it's a major improvement for the documentation of Tempfile.

@hsbt Could you review and merge?

@hsbt hsbt merged commit 3eb7945 into ruby:master Oct 5, 2020
@eregon eregon deleted the improve-tempfile-docs branch October 5, 2020 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants