Project

General

Profile

Actions

Bug #17144

closed

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

Added by Eregon (Benoit Daloze) about 4 years ago. Updated about 4 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:99868]

Description

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 depending on the file to still be on disk after the block.

@shugo (Shugo Maeda) 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?


Related issues 1 (1 open0 closed)

Related to Ruby master - Feature #7148: Improved Tempfile w/o DelegateClassAssignedGlass_saga (Masaki Matsushita)Actions
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0