https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112019-01-26T09:51:26ZRuby Issue Tracking SystemRuby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765232019-01-26T09:51:26ZEregon (Benoit Daloze)
<ul></ul><p>I think the workaround using a variable set after <code>yield</code> (<code>success = true</code>) is not too bad, and clear enough. And the performance would be fine in this case (i.e. there would be no overhead if the JIT profiles the branch and only one branch is taken, like TruffleRuby).</p>
<p><code>Ensure</code> should always be executed no matter the circumstances IMHO, so I don't like to make it conditional with extra syntax.<br>
Not executing <code>ensure</code> when using <code>break</code>, etc sounds like a bug in the vast majority of cases.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765242019-01-26T10:59:48Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p><a class="user active user-mention" href="https://bugs.ruby-lang.org/users/772">@Eregon (Benoit Daloze)</a> - thanks for your response.</p>
<blockquote>
<p><code>Ensure</code> should always be executed no matter the circumstances IMHO, so I don't like to make it conditional with extra syntax.</p>
</blockquote>
<p>This isn't about not executing <code>ensure</code>, but allowing user to handle situations like "returned normally" vs "non-standard flow control".</p>
<blockquote>
<p>I think the workaround using a variable set after <code>yield</code> (<code>success = true</code>) is not too bad, and clear enough. And the performance would be fine in this case (i.e. there would be no overhead if the JIT profiles the branch and only one branch is taken, like TruffleRuby).</p>
</blockquote>
<p>Personally, I think it's ugly, and I also think it is inefficient. I also don't think it's clearly conveying what the user is trying to do.</p>
<p>The problem is, people write code like this:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">begin</span>
<span class="o">..</span>
<span class="k">ensure</span>
<span class="n">abnormal_path</span> <span class="k">if</span> <span class="vg">$!</span>
<span class="k">end</span>
</code></pre>
<p>This actually wrong and fails in the case of <code>throw</code> wrapped in <code>catch</code> as given in the original examples.</p>
<p>It's actually not clear from the code if this is what the user wanted or not. Because you can't express clearly what you are actually interested in.</p>
<p>There should be no overhead when exiting normally in the following situation:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">begin</span>
<span class="k">yield</span>
<span class="k">ensure</span> <span class="k">when</span> <span class="ow">not</span> <span class="k">return</span>
<span class="k">return</span> <span class="ss">:abnormal</span>
<span class="k">end</span>
</code></pre>
<p>As soon as you write code like:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">begin</span>
<span class="k">yield</span>
<span class="n">success</span> <span class="o">=</span> <span class="kp">true</span>
<span class="k">ensure</span>
<span class="k">return</span> <span class="ss">:abnormal</span> <span class="k">unless</span> <span class="n">success</span>
<span class="k">end</span>
</code></pre>
<p>you guarantee that the code must pass through the <code>ensure</code> block. But your intention is, only execute this code if the code didn't return normally (or some other flow control).</p>
<p>So, I don't think the argument about always executing <code>ensure</code> holds up - this isn't about changing the semantics of <code>ensure</code> but extending it to handle explicitly what people are already doing, albeit probably incorrectly and inefficiently.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765272019-01-26T11:02:54Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul><li><strong>Target version</strong> deleted (<del><i>2.7</i></del>)</li></ul> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765312019-01-26T12:03:23Zshevegen (Robert A. Heiler)shevegen@gmail.com
<ul></ul><p>I was about to write a long comment, but then I noticed that my reply would be too long, so I shortened<br>
this. So just a few key statements:</p>
<p>I don't like the proposed syntax suggestion here in particular:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">ensure</span> <span class="k">when</span> <span class="ow">not</span> <span class="k">return</span>
</code></pre>
<p>The reason why I dislike it is partially due to the syntax verbosity; but also because, more significantly,<br>
because it appears to do conditional checks in very different means than have existed before in ruby.<br>
I would expect "<code>when</code>" usually in a case-structure.</p>
<p>There was this other recent suggestion to also add "<code>when</code>", to allow for chaining after the addition of<br>
the alias "<code>then</code>" to "<code>yield_self</code>", leading to chains of <code>.when .then .when .then</code>; and I also am not a huge fan<br>
of using "<code>when</code>" that way. Of course there is more than one way to do something, but my own personal<br>
preference would be to keep "<code>when</code>" reserved for case-when layouts.</p>
<p>As for <code>catch</code>/<code>throw</code>: I think it is a lot more common to see <code>begin</code>/<code>rescue</code> than <code>catch</code>/<code>throw</code>. Even if this<br>
may be slower (e. g. some people doing control flow logic with <code>begin</code>/<code>rescue</code>), I think the intent and<br>
clarity of intent is much clearer with control flows based on <code>begin</code>/<code>rescue</code> (and ensure) style layout<br>
of code. So to conclude and finish my comment, I think it would be better to not introduce a more<br>
complicated ensure conditional clause as proposed in the suggestion here. IMO you can re-structure<br>
your code to keep <code>ensure</code> simple, so it should not be necessary to add additional conditionals into<br>
<code>ensure</code>, even more so when these are unusual in regards to syntax - it would be the first time that I<br>
would see "<code>ensure when</code>" and I don't quite like that syntax suggestion in this context.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765402019-01-26T23:26:54Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p><a class="user active user-mention" href="https://bugs.ruby-lang.org/users/3414">@shevegen (Robert A. Heiler)</a> - thanks for your feedback.</p>
<blockquote>
<p>The reason why I dislike it is partially due to the syntax verbosity</p>
</blockquote>
<p>With regards to syntax, it should use existing keywords, otherwise it can cause more problems. The reason for <code>not</code> is already explained, because otherwise user may have to enumerate all other tags which is inconvenient and may break if new tags are introduced in the future. If you can propose better syntax, please do.</p>
<blockquote>
<p>but also because, more significantly, because it appears to do conditional checks in very different means than have existed before in ruby.</p>
</blockquote>
<p>This isn't really a conditional check like <code>if</code>, and it's not even a big change internally, it simply exposes a bit more of the exception handling mechanisms. It should be more efficient, because <code>ensure when not return</code> can be handled as part of exceptional flow control.</p>
<p>It's more similar to <code>rescue</code> than <code>ensure</code> in it's overhead. In MRI, <code>rescue</code> has some overhead, but it's possible to implement zero-cost exception handling.</p>
<p>The reason why I think this is a good idea is because Ruby provides a lot of flow control statements, but handling these abnormal flow control is very tricky and easy to get wrong. Statements like <code>ensure ... unless $!</code> is actually incorrect as well as inefficient. <code>rescue Exception</code> doesn't handle all ways a function can exit.</p>
<p>I think the key things to think about would be:</p>
<ul>
<li>Is there a better syntax for this proposal?</li>
<li>Does introducing such feature have any practical downside?
<ul>
<li>Implementation cost.</li>
<li>Maintenance cost.</li>
<li>Does it introduce technical debt?</li>
</ul>
</li>
<li>Does it solve a problem that can't be solved some other way?
<ul>
<li>
<code>ensure when not return</code> can be partly solved some other way but with some overhead.</li>
<li>
<code>ensure when raise</code> is similar to <code>rescue Exception</code> but doesn't require <code>raise</code> to resume default exception handling.</li>
<li>
<code>ensure when retry</code> and other formulations (<code>throw</code>, <code>return</code>, <code>next</code>, <code>break</code>, <code>redo</code>, <code>retry</code>) have no equivalent right now.</li>
<li>
<code>ensure when ..., ..., ...</code> is practically impossible, each case needs to be handled independently if possible.</li>
</ul>
</li>
<li>Even if there are existing solutions, does this proposed solution communicate more clearly the intention of the programmer?</li>
<li>Do we want to expose this level of behaviour to user code?</li>
<li>Could we limit what tags you can match? I suspect the most important one would be <code>ensure when not return</code>, but I can also imagine it could be pretty useful to catch things like <code>retry</code> and <code>redo</code>.</li>
</ul> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765412019-01-26T23:30:50Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>One more thing to bear in mind is that you can avoid this feature if you don't like it. Because it's for some very specific situations. It doesn't affect existing code in any way. But for situations like mine where I have a handful of use cases, and efficiency is a concern (to an extent) as well as readability and correctness, I would welcome such a feature. If you don't need it you won't be affected by it, but if you need it, there is actually no other practical solution in the language right now.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765452019-01-27T02:42:10Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>ioquatix (Samuel Williams) wrote:</p>
<blockquote>
<p>One more thing to bear in mind is that you can avoid this feature if you don't like it. Because it's for some very specific situations. It doesn't affect existing code in any way. But for situations like mine where I have a handful of use cases, and efficiency is a concern (to an extent) as well as readability and correctness, I would welcome such a feature. If you don't need it you won't be affected by it, but if you need it, there is actually no other practical solution in the language right now.</p>
</blockquote>
<p>In the very rare cases where you want to treat a non-local, non-exception exit differently from a normal exit, as Eregon mentioned, there is a simple existing practical solution that is unlikely to be significantly less efficient:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">doot</span>
<span class="n">ret</span> <span class="o">=</span> <span class="k">yield</span>
<span class="n">normal_exit</span> <span class="o">=</span> <span class="kp">true</span>
<span class="n">ret</span>
<span class="k">ensure</span>
<span class="c1"># Did the block return normally</span>
<span class="k">return</span> <span class="s2">"abnormal"</span> <span class="k">if</span> <span class="vg">$!</span> <span class="o">||</span> <span class="o">!</span><span class="n">normal_exit</span>
<span class="k">end</span>
</code></pre>
<p>I think we should definitely not add syntax in an attempt to make it easier to treat a non-local, non-exception exit differently than a normal exit, as doing so is usually a mistake.</p>
<p>You may not consider the above approach a simple and practical solution. However, considering you have already conceded that your proposal is for "very specific situations" and you only have "handful of use cases", even if you consider it neither simple nor practical, it's still easy and possible. New syntax is not necessary to support what you want, and I do not think new syntax should be added for "very specific situations" with only a "handful of use cases" when it is already possible to get the behavior you want with existing syntax.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765722019-01-29T21:21:32Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>So, <a class="user active user-mention" href="https://bugs.ruby-lang.org/users/1604">@jeremyevans0 (Jeremy Evans)</a>, I agree with everything you say.</p>
<p>That being said, this proposal goes beyond just that single case. For example, there is no easy way to capture <code>retry</code> or <code>redo</code> exiting a block.</p>
<p>Looking at your example, e.g.</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">doot</span>
<span class="n">ret</span> <span class="o">=</span> <span class="k">yield</span>
<span class="n">normal_exit</span> <span class="o">=</span> <span class="kp">true</span>
<span class="n">ret</span>
<span class="k">ensure</span>
<span class="c1"># Did the block return normally</span>
<span class="k">return</span> <span class="s2">"abnormal"</span> <span class="k">if</span> <span class="vg">$!</span> <span class="o">||</span> <span class="o">!</span><span class="n">normal_exit</span>
<span class="k">end</span>
</code></pre>
<p>I certainly prefer</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">doot</span>
<span class="k">yield</span>
<span class="k">ensure</span> <span class="k">when</span> <span class="ow">not</span> <span class="k">return</span>
<span class="k">return</span> <span class="s2">"abnormal"</span>
<span class="k">end</span>
</code></pre>
<p>It's clearer what's going on (especially if function is more complex than 1 line of actual code), it should be more efficiently implemented by VM since it only affects non-normal flow control and there is no conditional checks required.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765892019-01-30T12:22:59ZEregon (Benoit Daloze)
<ul></ul><p>ioquatix (Samuel Williams) wrote:</p>
<blockquote>
<p>For example, there is no easy way to capture <code>retry</code> or <code>redo</code> exiting a block.</p>
</blockquote>
<p>Which is intended, it would break the semantics of those if they were caught, or it would be very surprising if some ensure blocks are not run in these cases even though they exit the scope.</p>
<p>I think there is < 1% case where it's sensible to not run ensure in this kind of conditions, and the workaround seems good enough for these rare cases.</p>
<blockquote>
<p>it should be more efficiently implemented by VM since it only affects non-normal flow control and there is no conditional checks required.</p>
</blockquote>
<p>Please don't assume that. A check will be needed too in the general case, and it can already optimize very well with the local variable (e.g., zero overhead on TruffleRuby if only using the normal path, or only the exception path).</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=765912019-01-30T12:25:14ZEregon (Benoit Daloze)
<ul></ul><p><a class="user active user-mention" href="https://bugs.ruby-lang.org/users/3344">@ioquatix (Samuel Williams)</a> Could you give one or two real-world examples where this would be useful?</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=767172019-02-07T07:08:51Zmatz (Yukihiro Matsumoto)matz@ruby.or.jp
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Rejected</i></li></ul><p>The language feature similar to <code>ensure</code> can be observed in many languages (<code>unwind-protocol</code> in Lisp, <code>defer</code> in Go, <code>finally</code> in Java and others), but neither of them considers the situation. That indicates there's no real-world use-case for it. So currently I believe there's no benefit enough the implementation cost and making the language more complex.</p>
<p>Matz.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=767712019-02-12T01:16:56Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>Okay, thanks for your time and input. That makes sense.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=922362021-05-27T11:10:04Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>I revisited this situation again.</p>
<p>This behaviour is very confusing:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">transaction</span>
<span class="k">begin</span>
<span class="nb">puts</span> <span class="s2">"Begin Transaction"</span>
<span class="k">yield</span>
<span class="k">rescue</span>
<span class="nb">puts</span> <span class="s2">"Abort Transaction"</span>
<span class="k">else</span>
<span class="nb">puts</span> <span class="s2">"Commit Transaction"</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="kp">catch</span><span class="p">(</span><span class="ss">:ball</span><span class="p">)</span> <span class="k">do</span>
<span class="n">transaction</span> <span class="k">do</span>
<span class="kp">throw</span> <span class="ss">:ball</span>
<span class="k">end</span>
<span class="k">end</span>
</code></pre>
<p>In particular, trying to handle this by using <code>ensure ... unless $!</code> can fail in some situations due to scope of $!</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">transaction</span>
<span class="k">begin</span>
<span class="nb">puts</span> <span class="s2">"Begin Transaction"</span>
<span class="k">yield</span>
<span class="k">rescue</span>
<span class="nb">puts</span> <span class="s2">"Abort Transaction"</span>
<span class="k">ensure</span>
<span class="nb">puts</span> <span class="s2">"Commit Transaction"</span> <span class="k">unless</span> <span class="vg">$!</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="kp">catch</span><span class="p">(</span><span class="ss">:ball</span><span class="p">)</span> <span class="k">do</span>
<span class="k">begin</span>
<span class="k">raise</span> <span class="s2">"Problem"</span>
<span class="k">rescue</span>
<span class="n">transaction</span> <span class="k">do</span>
<span class="kp">throw</span> <span class="ss">:ball</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="k">end</span>
</code></pre>
<p>It seems like we must involve some elaborate state tracking in order to get the correct behaviour (calling either abort or commit predictably). So, I believe we should reconsider this feature, or some evolution of it.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=922372021-05-27T11:13:41Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>Here is working example:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">transaction</span>
<span class="n">failed</span> <span class="o">=</span> <span class="kp">false</span>
<span class="k">begin</span>
<span class="nb">puts</span> <span class="s2">"Begin Transaction"</span>
<span class="k">yield</span>
<span class="k">rescue</span> <span class="no">Exception</span>
<span class="nb">puts</span> <span class="s2">"Abort Transaction"</span>
<span class="n">failed</span> <span class="o">=</span> <span class="kp">true</span>
<span class="k">raise</span>
<span class="k">ensure</span>
<span class="nb">puts</span> <span class="s2">"Commit Transaction"</span> <span class="k">unless</span> <span class="n">failed</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="kp">catch</span><span class="p">(</span><span class="ss">:ball</span><span class="p">)</span> <span class="k">do</span>
<span class="k">begin</span>
<span class="k">raise</span> <span class="s2">"Problem"</span>
<span class="k">rescue</span>
<span class="n">transaction</span> <span class="k">do</span>
<span class="kp">throw</span> <span class="ss">:ball</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="k">end</span>
</code></pre>
<p>This seems overly complex to me personally.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=922382021-05-27T13:38:21Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>ioquatix (Samuel Williams) wrote in <a href="#note-14">#note-14</a>:</p>
<blockquote>
<p>Here is working example:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">transaction</span>
<span class="n">failed</span> <span class="o">=</span> <span class="kp">false</span>
<span class="k">begin</span>
<span class="nb">puts</span> <span class="s2">"Begin Transaction"</span>
<span class="k">yield</span>
<span class="k">rescue</span> <span class="no">Exception</span>
<span class="nb">puts</span> <span class="s2">"Abort Transaction"</span>
<span class="n">failed</span> <span class="o">=</span> <span class="kp">true</span>
<span class="k">raise</span>
<span class="k">ensure</span>
<span class="nb">puts</span> <span class="s2">"Commit Transaction"</span> <span class="k">unless</span> <span class="n">failed</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="kp">catch</span><span class="p">(</span><span class="ss">:ball</span><span class="p">)</span> <span class="k">do</span>
<span class="k">begin</span>
<span class="k">raise</span> <span class="s2">"Problem"</span>
<span class="k">rescue</span>
<span class="n">transaction</span> <span class="k">do</span>
<span class="kp">throw</span> <span class="ss">:ball</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="k">end</span>
</code></pre>
<p>This seems overly complex to me personally.</p>
</blockquote>
<p>You can simplify this slightly using a local variable:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">transaction</span>
<span class="n">failed</span> <span class="o">=</span> <span class="kp">false</span>
<span class="k">begin</span>
<span class="nb">puts</span> <span class="s2">"Begin Transaction"</span>
<span class="k">yield</span>
<span class="k">rescue</span> <span class="no">Exception</span> <span class="o">=></span> <span class="n">exc</span>
<span class="nb">puts</span> <span class="s2">"Abort Transaction"</span>
<span class="k">raise</span>
<span class="k">ensure</span>
<span class="nb">puts</span> <span class="s2">"Commit Transaction"</span> <span class="k">unless</span> <span class="n">exc</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="kp">catch</span><span class="p">(</span><span class="ss">:ball</span><span class="p">)</span> <span class="k">do</span>
<span class="k">begin</span>
<span class="k">raise</span> <span class="s2">"Problem"</span>
<span class="k">rescue</span>
<span class="n">transaction</span> <span class="k">do</span>
<span class="kp">throw</span> <span class="ss">:ball</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="k">end</span>
</code></pre>
<p>I think such code makes sense. Exceptions abort the transaction, but should be reraised and not swallowed, so the <code>raise</code> doesn't feel out of place.</p>
<p>Looking at my earlier example:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">doot</span>
<span class="n">ret</span> <span class="o">=</span> <span class="k">yield</span>
<span class="n">normal_exit</span> <span class="o">=</span> <span class="kp">true</span>
<span class="n">ret</span>
<span class="k">ensure</span>
<span class="c1"># Did the block return normally</span>
<span class="k">return</span> <span class="s2">"abnormal"</span> <span class="k">if</span> <span class="vg">$!</span> <span class="o">||</span> <span class="o">!</span><span class="n">normal_exit</span>
<span class="k">end</span>
</code></pre>
<p>This is a case that wouldn't work correctly in the transaction scenario, where it is called inside an existing <code>rescue</code> block. So you would currently have to use:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">doot</span>
<span class="n">ret</span> <span class="o">=</span> <span class="k">yield</span>
<span class="n">normal_exit</span> <span class="o">=</span> <span class="kp">true</span>
<span class="n">ret</span>
<span class="k">rescue</span> <span class="no">Exception</span> <span class="o">=></span> <span class="n">exc</span>
<span class="k">raise</span>
<span class="k">ensure</span>
<span class="c1"># Did the block return normally</span>
<span class="k">return</span> <span class="s2">"abnormal"</span> <span class="k">if</span> <span class="n">exc</span> <span class="o">||</span> <span class="o">!</span><span class="n">normal_exit</span>
<span class="k">end</span>
</code></pre>
<p>About the only way I can think to make that easier would be for <code>ensure</code> to support <code>=></code>:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">doot</span>
<span class="n">ret</span> <span class="o">=</span> <span class="k">yield</span>
<span class="n">normal_exit</span> <span class="o">=</span> <span class="kp">true</span>
<span class="n">ret</span>
<span class="k">ensure</span> <span class="o">=></span> <span class="n">exc</span>
<span class="c1"># Did the block return normally</span>
<span class="k">return</span> <span class="s2">"abnormal"</span> <span class="k">if</span> <span class="n">exc</span> <span class="o">||</span> <span class="o">!</span><span class="n">normal_exit</span>
<span class="k">end</span>
</code></pre>
<p>In this case <code>exc</code> would be <code>nil</code> if the code before ensure did not raise an exception, and the exception instance if it did.</p>
<p>However, I think the cases where this actually matters (<code>ensure</code> without <code>rescue</code> but that cares about whether an exception has been raised) are so few that we shouldn't consider language modifications to support them.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=922392021-05-27T15:47:43ZEregon (Benoit Daloze)
<ul></ul><p>What's wrong with this, which works fine?</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">transaction</span>
<span class="k">begin</span>
<span class="nb">puts</span> <span class="s2">"Begin Transaction"</span>
<span class="n">result</span> <span class="o">=</span> <span class="k">yield</span>
<span class="n">success</span> <span class="o">=</span> <span class="kp">true</span>
<span class="n">result</span>
<span class="k">ensure</span>
<span class="k">if</span> <span class="n">success</span>
<span class="nb">puts</span> <span class="s2">"Commit Transaction"</span>
<span class="k">else</span>
<span class="nb">puts</span> <span class="s2">"Abort Transaction"</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="kp">catch</span><span class="p">(</span><span class="ss">:ball</span><span class="p">)</span> <span class="k">do</span>
<span class="n">transaction</span> <span class="k">do</span>
<span class="kp">throw</span> <span class="ss">:ball</span>
<span class="k">end</span>
<span class="k">end</span>
</code></pre>
<p>IMHO it's clear and explicit.<br>
What we want to check here is ultimately if the block was successful (no error of any sort) and the local variable is pretty clear.<br>
In fact, you might even want to allow the block to return <code>:abort</code> or so, and then it would simply be <code>if success && result != :abort</code>.</p>
<p>FWIW I think there are very few use cases where <code>throw</code>/<code>catch</code> are a good idea, but this applies to <code>break</code> and others.<br>
Short answer: use <code>ensure</code> if you want to be sure something runs, <code>rescue</code> only captures <code>Exception</code>, as expected.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923032021-06-01T23:15:25Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>I suspect that there is existing buggy code which has ensure blocks and checks <code>$!</code> which fails in the case I mentioned.</p>
<p>I think the ensure block should be able to capture the current exception without using a global.</p>
<p>I like the proposed syntax:</p>
<pre><code>begin
...
ensure => exception
if exception
abort
else
commit
end
end
</code></pre>
<p>It allows existing code that uses <code>if $!</code> to be retrofitted to do the correct thing.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923042021-06-01T23:36:29Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p><a class="user active user-mention" href="https://bugs.ruby-lang.org/users/772">@Eregon (Benoit Daloze)</a> <code>throw</code> should not abort the transaction, it should be committed.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923152021-06-02T11:34:27ZEregon (Benoit Daloze)
<ul></ul><blockquote>
<p>I suspect that there is existing buggy code which has ensure blocks and checks $! which fails in the case I mentioned.</p>
</blockquote>
<p>Typically that's an anti-pattern.<br>
In 99% cases, the ensure code should do its logic regardless whether there is an ongoing exception/unwind or not.</p>
<p>Seems Jeremy agrees in comment 7:</p>
<blockquote>
<p>I think we should definitely not add syntax in an attempt to make it easier to treat a non-local, non-exception exit differently than a normal exit, as doing so is usually a mistake.</p>
</blockquote>
<hr>
<blockquote>
<p>Eregon (Benoit Daloze) throw should not abort the transaction, it should be committed.</p>
</blockquote>
<p>That is surprising to me.<br>
If <code>throw</code> is performed in the middle of the transaction block then maybe only half the operations are done, committing in that case seems wrong.</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="n">transaction</span> <span class="k">do</span>
<span class="n">update1</span>
<span class="n">some_call_that_ends_up_in_throw</span>
<span class="n">update2</span>
<span class="k">end</span>
</code></pre> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923182021-06-02T14:51:54Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>Eregon (Benoit Daloze) wrote in <a href="#note-19">#note-19</a>:</p>
<blockquote>
<blockquote>
<p>Eregon (Benoit Daloze) throw should not abort the transaction, it should be committed.</p>
</blockquote>
<p>That is surprising to me.<br>
If <code>throw</code> is performed in the middle of the transaction block then maybe only half the operations are done, committing in that case seems wrong.</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="n">transaction</span> <span class="k">do</span>
<span class="n">update1</span>
<span class="n">some_call_that_ends_up_in_throw</span>
<span class="n">update2</span>
<span class="k">end</span>
</code></pre>
</blockquote>
<p>Rails 7 is going to start treating non-local, non-exception exits as rolling back the transaction, the same as exception exits. I believe this is to work around the fact that Timeout uses throw (<a href="https://github.com/rails/rails/pull/29333" class="external">https://github.com/rails/rails/pull/29333</a>).</p>
<p>Sequel won't make the same change. I think that non-local, non-exception exits should commit the transaction, and only exceptions should roll the transaction back. Changing the behavior just because of Timeout's design (even if Timeout is not used) seems to be a mistake to me. I certainly have a lot of code that uses throw to exit transactions and expects the transaction will be committed. Sinatra and Roda both use throw for returning responses to web requests, for example:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="no">DB</span><span class="p">.</span><span class="nf">transaction</span> <span class="k">do</span>
<span class="no">DB</span><span class="p">[</span><span class="ss">:table</span><span class="p">].</span><span class="nf">insert</span>
<span class="k">unless</span> <span class="n">update_related_table?</span>
<span class="n">redirect</span> <span class="s1">'/some-path'</span> <span class="c1"># throw</span>
<span class="k">end</span>
<span class="no">DB</span><span class="p">[</span><span class="ss">:related_table</span><span class="p">].</span><span class="nf">update</span>
<span class="k">end</span>
</code></pre> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923212021-06-02T16:59:27ZEregon (Benoit Daloze)
<ul></ul><p>Thanks for the link.</p>
<p>Is <code>throw</code> used for other things than <code>redirect</code>?</p>
<p>If <code>redirect</code> would use a regular exception, and for instance pass <code>[]</code> as the backtrace, it would probably be as efficient, but then gives the possibility to differentiate for example with <code>rescue => e; e&.should_commit?</code> or so.<br>
<code>throw</code>/<code>catch</code> has AFAIK no way to communicate any information, so there is no way to know if the <code>throw</code> is meant as a convenience return like for <code>redirect</code> or as some kind of bailout which should not commit the transaction.<br>
It can't even be differentiated from return/break so it seems a bad idea to use <code>throw</code> in the first place.</p>
<p>It feels weird that Timeout uses <code>throw</code> and not <code>raise</code>, and it seems counter-intuitive (everyone knows <code>Timeout.timeout {}</code> raises Timeout::Error, and exception, <code>throw</code> is unexpected)<br>
It also adds quite some complexity: <a href="https://github.com/ruby/timeout/blob/4893cde0eda321448a1a86487ac9b571f6c35727/lib/timeout.rb#L29-L50" class="external">https://github.com/ruby/timeout/blob/4893cde0eda321448a1a86487ac9b571f6c35727/lib/timeout.rb#L29-L50</a><br>
Does anyone know what's the point of that?</p>
<p>What I could find is <a href="https://github.com/ruby/timeout/commit/238c003c921e6e555760f8e96968562a622a99c4" class="external">https://github.com/ruby/timeout/commit/238c003c921e6e555760f8e96968562a622a99c4</a><br>
and <a href="https://bugs.ruby-lang.org/issues/8730" class="external">https://bugs.ruby-lang.org/issues/8730</a></p>
<p>IMHO <code>rescue Exception</code> without re-raise is always the bug.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923222021-06-02T17:39:58Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>Eregon (Benoit Daloze) wrote in <a href="#note-21">#note-21</a>:</p>
<blockquote>
<p>Thanks for the link.</p>
<p>Is <code>throw</code> used for other things than <code>redirect</code>?</p>
</blockquote>
<p>Yes, it's used anytime you want to return a response immediately. I just used redirect as an example.</p>
<blockquote>
<p>If <code>redirect</code> would use a regular exception, and for instance pass <code>[]</code> as the backtrace, it would probably be as efficient, but then gives the possibility to differentiate for example with <code>rescue => e; e&.should_commit?</code> or so.<br>
<code>throw</code>/<code>catch</code> has AFAIK no way to communicate any information, so there is no way to know if the <code>throw</code> is meant as a convenience return like for <code>redirect</code> or as some kind of bailout which should not commit the transaction.<br>
It can't even be differentiated from return/break so it seems a bad idea to use <code>throw</code> in the first place.</p>
</blockquote>
<p>Changing a throw (which doesn't indicate error) to an exception (which indicates error) for a case that is not an error makes no sense to me. The problem is that Timeout uses throw instead of an exception for error cases (timeouts), not code that uses throw for non-error cases.</p>
<blockquote>
<p>It feels weird that Timeout uses <code>throw</code> and not <code>raise</code>, and it seems counter-intuitive (everyone knows <code>Timeout.timeout {}</code> raises Timeout::Error, and exception, <code>throw</code> is unexpected)<br>
It also adds quite some complexity: <a href="https://github.com/ruby/timeout/blob/4893cde0eda321448a1a86487ac9b571f6c35727/lib/timeout.rb#L29-L50" class="external">https://github.com/ruby/timeout/blob/4893cde0eda321448a1a86487ac9b571f6c35727/lib/timeout.rb#L29-L50</a><br>
Does anyone know what's the point of that?</p>
</blockquote>
<p>I believe this is because exception handling inside the Timeout.timeout block doesn't interfere with timeout handling.</p>
<blockquote>
<p>What I could find is <a href="https://github.com/ruby/timeout/commit/238c003c921e6e555760f8e96968562a622a99c4" class="external">https://github.com/ruby/timeout/commit/238c003c921e6e555760f8e96968562a622a99c4</a><br>
and <a href="https://bugs.ruby-lang.org/issues/8730" class="external">https://bugs.ruby-lang.org/issues/8730</a></p>
</blockquote>
<p>Yep, that pretty much confirms it. I don't agree with the tradeoff. Using throw for error cases is wrong, IMO. It would be better to have cases where Timeout didn't work due to improper rescuing, than not being able to use throw correctly because of Timeout's implementation. There are already places where Timeout doesn't work correctly, after all. A timeout error in my mind is more similar to a TERM signal, and Ruby handles that using an exception.</p>
<p>I should point out that this is not just a throw issue, it occurs for any non local exit. Here's an example using return. We have to use a transaction around the initial retrieval of the record due to the use of FOR UPDATE, because in certain conditions we want to update the record later, and don't want any modifications to the record in between. We need the transaction to commit and not rollback on exit, otherwise we lose the update to the associated object.</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">foo</span>
<span class="n">record</span> <span class="o">=</span> <span class="kp">nil</span>
<span class="n">transaction</span> <span class="k">do</span>
<span class="k">return</span> <span class="ss">:a</span> <span class="k">unless</span> <span class="n">record</span> <span class="o">=</span> <span class="n">dataset</span><span class="p">.</span><span class="nf">for_update</span><span class="p">.</span><span class="nf">first</span>
<span class="n">record</span><span class="p">.</span><span class="nf">associated_object</span><span class="p">.</span><span class="nf">update</span><span class="p">()</span>
<span class="k">return</span> <span class="ss">:b</span> <span class="k">if</span> <span class="n">record</span><span class="p">.</span><span class="nf">some_condition?</span>
<span class="n">record</span><span class="p">.</span><span class="nf">update</span><span class="p">()</span> <span class="k">if</span> <span class="n">record</span><span class="p">.</span><span class="nf">some_other_condition?</span>
<span class="k">end</span>
<span class="n">record</span>
<span class="k">end</span>
</code></pre>
<p>While you can generally rewrite code to avoid non-local exits like this, often the non-local exit approach is simplest.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923232021-06-02T20:54:08ZEregon (Benoit Daloze)
<ul></ul><blockquote>
<p>A timeout error in my mind is more similar to a TERM signal, and Ruby handles that using an exception.</p>
</blockquote>
<p>I think that's a good point.<br>
I agree for similar things like Ctrl+C/SIGINT which is an <code>Interrupt</code> exception (< SignalException < Exception), and <code>Kernel#exit</code> which is a <code>SystemExit</code> exception (< Exception).</p>
<p>Nothing can guarantee to bubble through user code without stopping in between (it's always possible to hang/loop via <code>ensure</code>),<br>
except a fatal error that would immediately end the process or skip even <code>ensure</code> blocks and always reach the C main().<br>
So proper exception handling (e.g. no <code>rescue Exception; # ignore</code>, <code>ensure</code> code must be bounded in time/steps) is always needed for correctness, and if Ctrl+C/exit work with an exception, so should Timeout.timeout.<br>
Maybe we should open a new issue so that Timeout.timeout uses an Exception and not <code>throw</code>?</p>
<hr>
<p>I'm not sure if <code>throw</code> is always used as non-local control flow, and not as some sort of exception (e.g., to indicate some 403).</p>
<p>BTW I guess we could replace throw with a non-local <code>return</code> using <code>handle_request(proc { return })</code> instead of <code>catch</code>, but it wouldn't change anything related to this issue, both are considered non-local exits and non-distinguishable.<br>
That's an argument for treating <code>throw</code> as control flow much like non-local <code>return</code> and not as exceptional though.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923242021-06-02T22:47:01Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>Thanks for the discussion. I'll try to summarise my position briefly.</p>
<blockquote>
<p>Maybe we should open a new issue so that Timeout.timeout uses an Exception and not throw?</p>
</blockquote>
<p>I agree Timeout should raise an exception internally and we should change this. Happy to work on a PR with you all. I am very strongly in favour of this. If we decide to do this, we should let Rails team know about our intended change so they don't end up working themselves into a corner.</p>
<blockquote>
<p>Yes, it's used anytime you want to return a response immediately. I just used redirect as an example.</p>
</blockquote>
<p>Yes, I also do this and agree with Jeremy's position here. While I do respect everyone's input, to me, exception = abort transaction, throw/break/next = commit.</p>
<p>Therefore, I do feel strongly that we need some precise way to capture the exception as we go through the ensure block. The previous suggestion makes sense to me, as in:</p>
<pre><code>begin
raise "Boom"
ensure => exception
end
</code></pre>
<p>This actually makes it very easy to do the correct thing. The alternatives are tricky to get right:</p>
<pre><code>failed = false
begin
rescue Exception # "Exception" usually missing... so buggy.
# error
failed = true
raise
ensure
if failed
abort
else
commit
end
end
</code></pre>
<p>I'm pretty sure I've got bugs in my code due to this, due to mis-understanding the flow control. So, I think we should make this easier for developers to get right.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923252021-06-02T22:47:36Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul><li><strong>Status</strong> changed from <i>Rejected</i> to <i>Open</i></li></ul> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923352021-06-04T04:22:36Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>Here are some examples of situations where <code>$!</code> is used inside <code>ensure</code> block. In every case, there is a potential buggy behaviour.</p>
<p><a href="https://github.com/drbrain/net-http-persistent/blob/75574f2546a08aa2663b06a2e005bcf2ee304f13/lib/net/http/persistent.rb#L230" class="external">https://github.com/drbrain/net-http-persistent/blob/75574f2546a08aa2663b06a2e005bcf2ee304f13/lib/net/http/persistent.rb#L230</a></p>
<p><a href="https://github.com/copiousfreetime/launchy/blob/80e723ebf2d9e73d35ae5de31a73268eb6c46ecc/lib/launchy.rb#L36-L40" class="external">https://github.com/copiousfreetime/launchy/blob/80e723ebf2d9e73d35ae5de31a73268eb6c46ecc/lib/launchy.rb#L36-L40</a></p>
<p><a href="https://github.com/rails/spring/blob/577cf01f232bb6dbd0ade7df2df2ac209697e741/lib/spring/application.rb#L296-L301" class="external">https://github.com/rails/spring/blob/577cf01f232bb6dbd0ade7df2df2ac209697e741/lib/spring/application.rb#L296-L301</a></p>
<p><a href="https://github.com/ahoward/open4/blob/2bc378285dcec9c88613fd4def57a58606c0680b/lib/open4.rb#L189-L192" class="external">https://github.com/ahoward/open4/blob/2bc378285dcec9c88613fd4def57a58606c0680b/lib/open4.rb#L189-L192</a></p>
<p>I can find more, but I think it's sufficient to show the problem.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=923362021-06-04T04:56:18Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>Also there is a problem with the proposed work around:</p>
<pre><code>begin
rescue Exception => exception
raise
ensure
if exception
abort
else
commit
end
end
</code></pre>
<p>This only works provided you don't have any other <code>rescue</code> blocks, which you can see in some of the above examples may be an issue, or at least require careful implementation.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=933222021-08-18T03:00:10Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Rejected</i></li></ul><p>I will make a more limited proposal.</p> Ruby master - Feature #15567: Allow ensure to match specific situationshttps://bugs.ruby-lang.org/issues/15567?journal_id=933382021-08-18T17:32:22ZEregon (Benoit Daloze)
<ul><li><strong>Related to</strong> <i><a class="issue tracker-2 status-1 priority-4 priority-default" href="/issues/18083">Feature #18083</a>: Capture error in ensure block.</i> added</li></ul>