From: Austin Ziegler Date: 2020-09-04T11:34:50-04:00 Subject: [ruby-core:99923] Re: [Ruby master Bug#17144] Tempfile.open { ... } does not unlink the file --===============1286644039== Content-Type: multipart/alternative; boundary="000000000000fae75205ae7e9c0f" --000000000000fae75205ae7e9c0f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable If we don=E2=80=99t change the behaviour, could we at least modify the documentation for `Tempfile.open` to recommend most people use `Tempfile.create`, since I don=E2=80=99t think that I=E2=80=99ve ever used = it and reach for `Tempfile.open` most of the time, because of its similarity to `File.open` (and I=E2=80=99ve been using Ruby since 2002). -a On Thu, Sep 3, 2020 at 9:18 PM 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 bloc= k. > > 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-patter= n > to correctly release all resources: > > (from > https://github.com/ruby/ruby/commit/e8c3872555fc85640505974e6b1c39d315572= 689 > ) > > ```ruby > > begin > > tf =3D 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 behavio= r. > > 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 =3D 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.op= en. > > 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/fa21985a7a2f8f52a8bd82bd12a724e9dca74= 934 > ). > > 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/e8c3872555fc85640505974e6b1c39d315572= 689 > (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/3beecafc2cae86290a191c1e841be13f5b087= 95d) > 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/d347e89ef6c817e469a1c25985dbd729c52b8= 0fd > 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: > > --=20 Austin Ziegler =E2=80=A2 halostatue@gmail.com =E2=80=A2 austin@halostatue.c= a http://www.halostatue.ca/ =E2=80=A2 http://twitter.com/halostatue --000000000000fae75205ae7e9c0f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
If we don=E2=80=99t change the behaviour, could we at leas= t modify the documentation for `Tempfile.open` to recommend most people use= `Tempfile.create`, since I don=E2=80=99t think that I=E2=80=99ve ever used= it and reach for `Tempfile.open` most of the time, because of its similari= ty to `File.open` (and I=E2=80=99ve 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 s= ake 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.crea= te.

> And people must be annoyed all the time by this bug/surprising behavio= r.
> 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.ope= n { ... }; ensure` or discover the non-standard-named method `Tempfile.crea= te { ... }`).

I feel that "create" is the second standard choice of factory met= hod.

> Regarding the pattern, Tempfile.open requires this complete anti-patte= rn to correctly release all resources:
> (from https://githu= b.com/ruby/ruby/commit/e8c3872555fc85640505974e6b1c39d315572689)
> ```ruby
>=C2=A0 =C2=A0begin
>=C2=A0 =C2=A0 =C2=A0tf =3D Tempfile.open 'test' do |io| # yet a= nother variable name for the same thing
>=C2=A0 =C2=A0 =C2=A0 =C2=A0io.write "..."
>=C2=A0 =C2=A0 =C2=A0 =C2=A0io # leaking the argument of the resource bl= ock, that feels very hacky
>=C2=A0 =C2=A0 =C2=A0end
>=C2=A0 =C2=A0 =C2=A0# use tf.path
>=C2=A0 =C2=A0ensure
>=C2=A0 =C2=A0 =C2=A0tf.close! if tf # Am I using Ruby or Go? The block = should deal with this.
>=C2=A0 =C2=A0end
> ```
> Do we want to perpetuate this anti-pattern?
>
> With this change, it's:
> ```ruby
>=C2=A0 =C2=A0Tempfile.open 'test' do |io|
>=C2=A0 =C2=A0 =C2=A0io.write "..."
>=C2=A0 =C2=A0 =C2=A0io.close # or io.flush
>=C2=A0 =C2=A0 =C2=A0# use tf.path
>=C2=A0 =C2=A0end
> ```
>
> If we want to deprecate then we'd deprecate Tempfile.open with a b= lock (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 behav= ior.

Also, Tempfile.create has advantages to the proposed Tempfile.open change:<= br> - It is available for all maintained Ruby versions.=C2=A0 (It is available = since Ruby 2.1. Zero-argument call is permitted since Ruby 2.4.) Tempfile.c= reate is usable without worrying Ruby version dependencies.
- The proposed change doesn't eliminate all curiousness of Tempfile.ope= n. It still use Tempfile class for delegation which is eliminated in Tempfi= le.create.

As far as I understand the advantage of the proposed Tempfile.open change o= ver Tempfile.create is just a method name which is consistent with other cl= asses.



----------------------------------------
Bug #17144: Tempfile.open { ... } does not unlink the file
https://bugs.ruby-lang.org/issues/17144#change-8= 7441

* 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 =3D f.pa= th }; p File.exist?($path)'
true
```
but it should be `false`.

This means even after the block finishes to execute the file still exists o= n 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 usag= es of `SomeClass.open { ... }`.
There are more than 10 SomeClass.open in core+stdlib, and AFAIK all these m= ethods 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 blo= ck:
https://docs.ruby-lang.org/en/2.7.0/T= empfile.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/fa21985a7a2f8= f52a8bd82bd12a724e9dca74934).

However it can cause some incompatibility, if an existing usage relied on t= he 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 expecte= d 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 r= everted because such libraries probably want to keep compat with older Rubi= es)

In all of ruby/ruby I found only [2 usages](https://github.com/ruby/ruby/commit/3beecafc2cae86290a1= 91c1e841be13f5b08795d) depending on the file to still be on disk after = the block.

@shugo brought to my attention that `Tempfile.create { ... }` unlinks the f= ile (Tempfile.create seems to exist since 2.1 yet very few seem to know abo= ut it).
I think the semantics of `Tempfile.create { ... }` is what the vast majorit= y 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 `T= empfile.open { ... }` to avoid creating needlessly a finalizer.

Found by https://github.= com/ruby/spec/commit/d347e89ef6c817e469a1c25985dbd729c52b80fd and the l= eak detector.

From https://github.com/ruby/tempfile/issues/2

So, OK to keep this change and make `Tempfile.open { ... }` do what most us= ages expect,
even if we have to update a few usages that relied on the file to still exi= st after the block?



--
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:ruby-core-request@ruby-lang.org?subject=3Dunsubscribe= >
<http://lists.ruby-lang.org/cgi-bin/m= ailman/options/ruby-core>


--
Austin Ziegler =E2=80=A2 halostatue@gmail.com =E2=80=A2 austin@halostatue.ca
http://www.ha= lostatue.ca/ =E2=80=A2 http://twitter.com/halostatue
--000000000000fae75205ae7e9c0f-- --===============1286644039== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline Unsubscribe: --===============1286644039==--