If we don”Ēt change the behaviour, could we at least modify the
documentation for `Tempfile.open` to recommend most people use
`Tempfile.create`, since I don”Ēt think that I”Ēve ever used it and reach for
`Tempfile.open` most of the time, because of its similarity to `File.open`
(and I”Ēve been using Ruby since 2002).

-a

On Thu, Sep 3, 2020 at 9:18 PM <akr / fsij.org> wrote:

> Issue #17144 has been updated by akr (Akira Tanaka).
>
>
> Eregon (Benoit Daloze) wrote in #note-9:
> > Dan0042 (Daniel DeLorme) wrote in #note-6:
> > > -1 for breaking compatibility with no deprecation, just for the sake
> of perceived consistency.
> >
> > Not "the sake of perceived consistency".
> > It's leaking a resource (a file on the disk) outside of a resource block.
> > Seems a severe issue to me.
> >
>
> It is compared to wrong use of Tempfile.open
>
> I understand "perceived consistency" is compared to Tempfile.create.
>
> > And people must be annoyed all the time by this bug/surprising behavior.
> > I think we annoy less people by fixing this behavior (very few places
> need to adapt) than keeping it (all future usages have to use
> `Tempfile.open { ... }; ensure` or discover the non-standard-named method
> `Tempfile.create { ... }`).
>
> I feel that "create" is the second standard choice of factory method.
>
> > Regarding the pattern, Tempfile.open requires this complete anti-pattern
> to correctly release all resources:
> > (from
> https://github.com/ruby/ruby/commit/e8c3872555fc85640505974e6b1c39d315572689
> )
> > ```ruby
> >   begin
> >     tf = Tempfile.open 'test' do |io| # yet another variable name for
> the same thing
> >       io.write "..."
> >       io # leaking the argument of the resource block, that feels very
> hacky
> >     end
> >     # use tf.path
> >   ensure
> >     tf.close! if tf # Am I using Ruby or Go? The block should deal with
> this.
> >   end
> > ```
> > Do we want to perpetuate this anti-pattern?
> >
> > With this change, it's:
> > ```ruby
> >   Tempfile.open 'test' do |io|
> >     io.write "..."
> >     io.close # or io.flush
> >     # use tf.path
> >   end
> > ```
> >
> > If we want to deprecate then we'd deprecate Tempfile.open with a block
> (and use Tempfile.create instead).
> > That's for sure causing more pain than the change I did.
>
> We can use Tempfile.create.
> There is no incompatibility pain if we don't change Tempfile.open behavior.
>
> Also, Tempfile.create has advantages to the proposed Tempfile.open change:
> - It is available for all maintained Ruby versions.  (It is available
> since Ruby 2.1. Zero-argument call is permitted since Ruby 2.4.)
> Tempfile.create is usable without worrying Ruby version dependencies.
> - The proposed change doesn't eliminate all curiousness of Tempfile.open.
> It still use Tempfile class for delegation which is eliminated in
> Tempfile.create.
>
> As far as I understand the advantage of the proposed Tempfile.open change
> over Tempfile.create is just a method name which is consistent with other
> classes.
>
>
>
> ----------------------------------------
> Bug #17144: Tempfile.open { ... } does not unlink the file
> https://bugs.ruby-lang.org/issues/17144#change-87441
>
> * Author: Eregon (Benoit Daloze)
> * Status: Open
> * Priority: Normal
> * Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN
> ----------------------------------------
> ```
> ruby -rtempfile -e 'Tempfile.open("txt") { |f| $path = f.path }; p
> File.exist?($path)'
> true
> ```
> but it should be `false`.
>
> This means even after the block finishes to execute the file still exists
> on a disk
> And this might or not be addressed by finalization much later on.
>
> This is inconsistent with the resource block pattern and basically all
> usages of `SomeClass.open { ... }`.
> There are more than 10 SomeClass.open in core+stdlib, and AFAIK all these
> methods release all resources at the end of the block, except Tempfile.open.
>
> Since the block creates the file, it should also delete it, so there are
> no leftovers after the block.
>
> The (English) docs don't mention the file is kept on disk after the block:
> https://docs.ruby-lang.org/en/2.7.0/Tempfile.html#method-c-open
>
> I made a PR to do unlink in https://github.com/ruby/tempfile/pull/3 and
> some commits in ruby/ruby (notably
> https://github.com/ruby/ruby/commit/fa21985a7a2f8f52a8bd82bd12a724e9dca74934
> ).
>
> However it can cause some incompatibility, if an existing usage relied on
> the block only closing the file descriptor but not unlink the path.
> See https://github.com/ruby/tempfile/issues/2#issuecomment-686323507
>
> When integrating this change in ruby/ruby, I found that many usages
> expected that the file be unlinked automatically, but had to add an extra
> `ensure tempfile.close!` on top of the block:
>
> https://github.com/ruby/ruby/commit/e8c3872555fc85640505974e6b1c39d315572689
> (later partially reverted because such libraries probably want to keep
> compat with older Rubies)
>
> In all of ruby/ruby I found only [2 usages](
> https://github.com/ruby/ruby/commit/3beecafc2cae86290a191c1e841be13f5b08795d)
> depending on the file to still be on disk after the block.
>
> @shugo brought to my attention that `Tempfile.create { ... }` unlinks the
> file (Tempfile.create seems to exist since 2.1 yet very few seem to know
> about it).
> I think the semantics of `Tempfile.create { ... }` is what the vast
> majority of usages want for `Tempfile.open { ... }`.
> `open` is the name everyone expects, so I think it's best if
> `Tempfile.open { ... }` also links.
> `create` doesn't sound like it will cleanup to me.
>
> As an optimization we can use `Tempfile.create(&block)` to implement
> `Tempfile.open { ... }` to avoid creating needlessly a finalizer.
>
> Found by
> https://github.com/ruby/spec/commit/d347e89ef6c817e469a1c25985dbd729c52b80fd
> and the leak detector.
>
> From https://github.com/ruby/tempfile/issues/2
>
> So, OK to keep this change and make `Tempfile.open { ... }` do what most
> usages expect,
> even if we have to update a few usages that relied on the file to still
> exist after the block?
>
>
>
> --
> https://bugs.ruby-lang.org/
>
> Unsubscribe: <mailto:ruby-core-request / ruby-lang.org?subject=unsubscribe>
> <http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>
>


-- 
Austin Ziegler  halostatue / gmail.com  austin / halostatue.ca
http://www.halostatue.ca/  http://twitter.com/halostatue
(supressed text/html)
Unsubscribe: <mailto:ruby-core-request / ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>