ruby-core

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:[email protected]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>

Thread

Prev Next

In This Thread

Prev Next