ruby / tempfile

A utility class for managing temporary files.
Other
29 stars 10 forks source link

Tempfile.open { ... } does not unlink the file #2

Closed eregon closed 4 years ago

eregon commented 4 years ago
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 e.g. File.open.

Since the block creates the file, it should also delete it, so there are no leftovers after the block.

How about using close! instead of close in https://github.com/ruby/tempfile/blob/ec3dd03f94cb6b0a187dfee146680a0d0b444e42/lib/tempfile.rb#L293 ?

Found by https://github.com/ruby/spec/commit/d347e89ef6c817e469a1c25985dbd729c52b80fd and the leak detector.

shugo commented 4 years ago

@eregon Tempfile was originally designed as GC dependent, and it was an expected behavior that Tempfile.open {} doesn't unlink the file. I'm not against this change, but there may be applications depending on the old behavior, so I think we should discuss this issue at bugs.ruby-lang.org.

shugo commented 4 years ago

FYI, Tempfile.create was introduced instead of Tempfile.open to avoid breaking change.

znz commented 4 years ago

Japanese reference manual has an example that use path after open with block. This change breaks such usage.

https://docs.ruby-lang.org/ja/2.7.0/method/Tempfile/s/new.html

require 'tempfile'

tf = Tempfile.open("temp"){|fp|
  fp.puts "hoge"
  fp
}
# テンポラリファイルへのパスを表示
p tf.path
p File.read(tf.path) #=> "hoge\n"
eregon commented 4 years ago

OK, I'll create an issue on bugs.ruby-lang.org.

I think it's very strange that open doesn't fully release the resources it created. So IMHO still good to have. Also 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

In all of ruby/ruby I found only 2 usages depending on that, and 10+ usages which had to close! explicitly due to these surprising semantics, or forgot to.

hsbt commented 4 years ago

@eregon I and @ko1 discuss about the right place for discussing the feature change. Let's move to bugs.ruby-lang.org.

And I will disable the issue feature with unmaintained libraries on https://github.com/ruby/ruby/blob/master/doc/maintainers.rdoc.

eregon commented 4 years ago

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