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>