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

[eregon]

See https://bugs.ruby-lang.org/issues/17144 Follow-up of https://github.com/ruby/tempfile/pull/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]

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]

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]

@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]

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]

@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?

[akr]

It seems fine.

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

[eregon]

@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?