https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112020-09-03T09:32:02ZRuby Issue Tracking SystemRuby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=873962020-09-03T09:32:02ZEregon (Benoit Daloze)
<ul></ul><p>Out of these 2 usages that relied on the file to still exist, 1 was incorrect:<br>
<a href="https://github.com/ruby/ruby/blob/e8c3872555fc85640505974e6b1c39d315572689/lib/reline/line_editor.rb#L2085-L2087" class="external">https://github.com/ruby/ruby/blob/e8c3872555fc85640505974e6b1c39d315572689/lib/reline/line_editor.rb#L2085-L2087</a><br>
The GC can trigger anywhere between these 3 lines and it would break.</p>
<p>The other usage seemed correct, but was IMHO quite ugly (a resource block + an <code>ensure</code>, needed to workaround this issue):<br>
<a href="https://github.com/ruby/ruby/blob/e8c3872555fc85640505974e6b1c39d315572689/test/openssl/test_x509store.rb#L36-L49" class="external">https://github.com/ruby/ruby/blob/e8c3872555fc85640505974e6b1c39d315572689/test/openssl/test_x509store.rb#L36-L49</a></p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=873982020-09-03T12:58:36Zakr (Akira Tanaka)akr@fsij.org
<ul></ul><p>I sympathize with this issue.<br>
I wish Tempfile.open had worked like Tempfile.create from the beginning.</p>
<p>But changing it is incompatible.<br>
It is not our (current) practice that introducing an incompatible change without prior notice.</p>
<p>It would be possible to change it with some migration period (if matz approves).<br>
But I doubt that the benefits are not greater than the pain of its incompatibility because desired functionality, "block exit removes the temporary file", is already provided in Tempfile.create.</p>
<p>Anyway, it would be good to improve the document of Tempfile.open to recommend Tempfile.create.</p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=873992020-09-03T13:15:03Zakr (Akira Tanaka)akr@fsij.org
<ul></ul><p>One idea: creating tmpfile library and<br>
Tmpfile.open calls Tempfile.create.</p>
<p>This also resolves an inconsistency between tempfile.rb and tmpdir.rb.<br>
For this consistency, File.tmpfile may be better than Tmpfile.open<br>
because it is similar to Dir.tmpdir.</p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=874002020-09-03T13:44:35ZEregon (Benoit Daloze)
<ul></ul><p>It seems hard to deprecate here without changing behavior. Any idea?<br>
OTOH, when an usage relied on the file to still exist it should be quite clear what happens (the file will not be there, so an exception).<br>
It took me seconds to find out when looking at the failing tests.</p>
<p>The change is already part of NEWS, I could mark it as experimental if desired.</p>
<p>I think very few cases will break, so it seems reasonable to change to me.<br>
I'd think most cases which want the file to still be on disk either have all the logic inside the block, or don't use a block (so those are unaffected).<br>
Ruby 3.0 has other incompatible changes of course, IMHO we need to evolve the old APIs, not be stuck forever with strange and non-intuitive APIs.</p>
<p>Many gems test against ruby-head, maybe let's simply wait a bit and see if they report some breakage?<br>
From experience, that seems a practical way to estimate incompatibility.</p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=874092020-09-03T15:05:13ZGlass_saga (Masaki Matsushita)glass.saga@gmail.com
<ul><li><strong>Related to</strong> <i><a class="issue tracker-2 status-2 priority-4 priority-default" href="/issues/7148">Feature #7148</a>: Improved Tempfile w/o DelegateClass</i> added</li></ul> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=874102020-09-03T15:14:15ZDan0042 (Daniel DeLorme)
<ul></ul><p>-1 for breaking compatibility with no deprecation, just for the sake of perceived consistency.</p>
<p>But then again it's important to note that <code>Tempfile.open{ }</code> returns nil, so the only way to cause an incompatibility is if the blocks "leaks" the Tempfile reference, like <code>Tempfile.open{ @f = _1 }</code> or <code>Tempfile.open{ return _1 }</code></p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=874112020-09-03T15:15:24ZGlass_saga (Masaki Matsushita)glass.saga@gmail.com
<ul></ul><p>If we could allow incompatible changes of Tempfile in 3.0 or later, it is a good chance to reimplement it as a subclass of File without delegate.rb.<br>
<a class="issue tracker-2 status-2 priority-4 priority-default" title="Feature: Improved Tempfile w/o DelegateClass (Assigned)" href="https://bugs.ruby-lang.org/issues/7148">#7148</a></p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=874122020-09-03T15:46:30ZDan0042 (Daniel DeLorme)
<ul></ul><blockquote>
<p><code>Tempfile.open{ }</code> returns nil</p>
</blockquote>
<p>I apologize for this brain fart. <code>Tempfile.open{ }</code> returns the result of the block. So it's entirely likely that someone would use<br>
<code>tmp = Tempfile.open{ |f| f.write(data); f }</code><br>
instead of<br>
<code>tmp = Tempfile.open; tmp.write(data); tmp.close</code></p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=874172020-09-03T18:56:35ZEregon (Benoit Daloze)
<ul></ul><p>Dan0042 (Daniel DeLorme) wrote in <a href="#note-6">#note-6</a>:</p>
<blockquote>
<p>-1 for breaking compatibility with no deprecation, just for the sake of perceived consistency.</p>
</blockquote>
<p>Not "the sake of perceived consistency".<br>
It's leaking a resource (a file on the disk) outside of a resource block.<br>
Seems a severe issue to me.</p>
<p>And people must be annoyed all the time by this bug/surprising behavior.<br>
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 <code>Tempfile.open { ... }; ensure</code> or discover the non-standard-named method <code>Tempfile.create { ... }</code>).</p>
<p>Regarding the pattern, Tempfile.open requires this complete anti-pattern to correctly release all resources:<br>
(from <a href="https://github.com/ruby/ruby/commit/e8c3872555fc85640505974e6b1c39d315572689" class="external">https://github.com/ruby/ruby/commit/e8c3872555fc85640505974e6b1c39d315572689</a>)</p>
<pre><code class="ruby syntaxhl" data-language="ruby"> <span class="k">begin</span>
<span class="n">tf</span> <span class="o">=</span> <span class="no">Tempfile</span><span class="p">.</span><span class="nf">open</span> <span class="s1">'test'</span> <span class="k">do</span> <span class="o">|</span><span class="n">io</span><span class="o">|</span> <span class="c1"># yet another variable name for the same thing</span>
<span class="n">io</span><span class="p">.</span><span class="nf">write</span> <span class="s2">"..."</span>
<span class="n">io</span> <span class="c1"># leaking the argument of the resource block, that feels very hacky</span>
<span class="k">end</span>
<span class="c1"># use tf.path</span>
<span class="k">ensure</span>
<span class="n">tf</span><span class="p">.</span><span class="nf">close!</span> <span class="k">if</span> <span class="n">tf</span> <span class="c1"># Am I using Ruby or Go? The block should deal with this.</span>
<span class="k">end</span>
</code></pre>
<p>Do we want to perpetuate this anti-pattern?</p>
<p>With this change, it's:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"> <span class="no">Tempfile</span><span class="p">.</span><span class="nf">open</span> <span class="s1">'test'</span> <span class="k">do</span> <span class="o">|</span><span class="n">io</span><span class="o">|</span>
<span class="n">io</span><span class="p">.</span><span class="nf">write</span> <span class="s2">"..."</span>
<span class="n">io</span><span class="p">.</span><span class="nf">close</span> <span class="c1"># or io.flush</span>
<span class="c1"># use tf.path</span>
<span class="k">end</span>
</code></pre>
<p>If we want to deprecate then we'd deprecate Tempfile.open with a block (and use Tempfile.create instead).<br>
That's for sure causing more pain than the change I did.</p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=874412020-09-04T01:18:22Zakr (Akira Tanaka)akr@fsij.org
<ul></ul><p>Eregon (Benoit Daloze) wrote in <a href="#note-9">#note-9</a>:</p>
<blockquote>
<p>Dan0042 (Daniel DeLorme) wrote in <a href="#note-6">#note-6</a>:</p>
<blockquote>
<p>-1 for breaking compatibility with no deprecation, just for the sake of perceived consistency.</p>
</blockquote>
<p>Not "the sake of perceived consistency".<br>
It's leaking a resource (a file on the disk) outside of a resource block.<br>
Seems a severe issue to me.</p>
</blockquote>
<p>It is compared to wrong use of Tempfile.open</p>
<p>I understand "perceived consistency" is compared to Tempfile.create.</p>
<blockquote>
<p>And people must be annoyed all the time by this bug/surprising behavior.<br>
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 <code>Tempfile.open { ... }; ensure</code> or discover the non-standard-named method <code>Tempfile.create { ... }</code>).</p>
</blockquote>
<p>I feel that "create" is the second standard choice of factory method.</p>
<blockquote>
<p>Regarding the pattern, Tempfile.open requires this complete anti-pattern to correctly release all resources:<br>
(from <a href="https://github.com/ruby/ruby/commit/e8c3872555fc85640505974e6b1c39d315572689" class="external">https://github.com/ruby/ruby/commit/e8c3872555fc85640505974e6b1c39d315572689</a>)</p>
<pre><code class="ruby syntaxhl" data-language="ruby"> <span class="k">begin</span>
<span class="n">tf</span> <span class="o">=</span> <span class="no">Tempfile</span><span class="p">.</span><span class="nf">open</span> <span class="s1">'test'</span> <span class="k">do</span> <span class="o">|</span><span class="n">io</span><span class="o">|</span> <span class="c1"># yet another variable name for the same thing</span>
<span class="n">io</span><span class="p">.</span><span class="nf">write</span> <span class="s2">"..."</span>
<span class="n">io</span> <span class="c1"># leaking the argument of the resource block, that feels very hacky</span>
<span class="k">end</span>
<span class="c1"># use tf.path</span>
<span class="k">ensure</span>
<span class="n">tf</span><span class="p">.</span><span class="nf">close!</span> <span class="k">if</span> <span class="n">tf</span> <span class="c1"># Am I using Ruby or Go? The block should deal with this.</span>
<span class="k">end</span>
</code></pre>
<p>Do we want to perpetuate this anti-pattern?</p>
<p>With this change, it's:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"> <span class="no">Tempfile</span><span class="p">.</span><span class="nf">open</span> <span class="s1">'test'</span> <span class="k">do</span> <span class="o">|</span><span class="n">io</span><span class="o">|</span>
<span class="n">io</span><span class="p">.</span><span class="nf">write</span> <span class="s2">"..."</span>
<span class="n">io</span><span class="p">.</span><span class="nf">close</span> <span class="c1"># or io.flush</span>
<span class="c1"># use tf.path</span>
<span class="k">end</span>
</code></pre>
<p>If we want to deprecate then we'd deprecate Tempfile.open with a block (and use Tempfile.create instead).<br>
That's for sure causing more pain than the change I did.</p>
</blockquote>
<p>We can use Tempfile.create.<br>
There is no incompatibility pain if we don't change Tempfile.open behavior.</p>
<p>Also, Tempfile.create has advantages to the proposed Tempfile.open change:</p>
<ul>
<li>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.</li>
<li>The proposed change doesn't eliminate all curiousness of Tempfile.open. It still use Tempfile class for delegation which is eliminated in Tempfile.create.</li>
</ul>
<p>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.</p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=874852020-09-06T00:55:51ZDan0042 (Daniel DeLorme)
<ul></ul><p>Austin Ziegler wrote on mailing list:</p>
<blockquote>
<p>If we don’t change the behaviour, could we at least modify the documentation for <code>Tempfile.open</code> to recommend most people use <code>Tempfile.create</code>, since I don’t think that I’ve ever used it and reach for <code>Tempfile.open</code> most of the time, because of its similarity to <code>File.open</code> (and I’ve been using Ruby since 2002).</p>
</blockquote>
<p>That would be good, and in fact I believe improving the documentation is the best way to address this issue.</p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=876862020-09-25T04:45:36Zmatz (Yukihiro Matsumoto)matz@ruby.or.jp
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul><p>To keep compatibility, we are not going to change the behavior of <code>Tempfile.open</code>. But to reduce the confusion, documentation was updated.<br>
In the future, maybe something like <code>RuboCop</code> warning will be convienient.</p>
<p>Matz.</p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=877022020-09-25T09:30:49ZEregon (Benoit Daloze)
<ul></ul><p>OK, <a href="https://github.com/ruby/tempfile/pull/4" class="external">https://github.com/ruby/tempfile/pull/4</a> is the PR that reverts it and already applied to ruby master.</p>
<p>I will try to improve the documentation further, something like<br>
"Tempfile.open is basically deprecated but kept for compatibility, please use Tempfile.create instead whenever possible".</p> Ruby master - Bug #17144: Tempfile.open { ... } does not unlink the filehttps://bugs.ruby-lang.org/issues/17144?journal_id=877262020-09-25T16:24:00Zakr (Akira Tanaka)akr@fsij.org
<ul></ul><p>Tempfile.open would be useful when an user want to remove the file by GC.<br>
Tempfile.create doesn't provide the feature, intentionally.</p>
<p>So, as far as an user needs temporary file removal by GC, we should not deprecate Tempfile.open<br>
(unless we provide an alternative).</p>