Feature #18083: Capture error in ensure block.

• Status: Open
• Assignee: -
• Target version: -

[ioquatix (Samuel Williams)]

As discussed in https://bugs.ruby-lang.org/issues/15567 there are some tricky edge cases.

As a general model, something like the following would be incredibly useful:

begin
 ...
ensure => error
  pp "error occurred" if error
end

Currently you can get similar behaviour like this:

begin
  ...
rescue Exception => error
  raise
ensure
  pp "error occurred" if error
end

The limitation of this approach is it only works if you don't need any other rescue clause. Otherwise, it may not work as expected or require extra care. Also, Rubocop will complain about it.

Using $! can be buggy if you call some method from rescue or ensure clause, since it would be set already. It was discussed extensively in https://bugs.ruby-lang.org/issues/15567 if you want more details.

[ioquatix (Samuel Williams)]

By the way, perhaps we should consider deprecating $! since its usage can lead to incorrect behaviour.

[Eregon (Benoit Daloze)]

The second snippet seems clear, the first one much less.
In what cases error is set? If it's a StandardError? If it's an Exception? If it's any kind of "Ruby exception"?

IMHO this pattern is rarely enough needed that the second snippet is good enough.
If you need other rescue, you can add them in a nested begin/end, or in a different method, no?

BTW I think the second snippet only differentiates Exception and put together regular execution and control flow exit, is that intended?

[Eregon (Benoit Daloze)]

If you only need to know if there is an Exception this seems easier:
Using the example from https://bugs.ruby-lang.org/issues/15567#note-27

begin
  ...
rescue Exception => exception
  transaction.abort
  raise exception
ensure
  transaction.commmit unless transaction.aborted?
end

# or

begin
  ...
rescue Exception => exception
  aborted = true
  transaction.abort
  raise exception
ensure
  transaction.commmit unless aborted
end

# or 

begin
  ...
rescue Exception => exception
  transaction.abort
  raise exception
ensure
  transaction.commmit unless exception
end

[Eregon (Benoit Daloze)]

• Related to Feature #15567: Allow ensure to match specific situations added

[ioquatix (Samuel Williams)]

There are lots of ways to achieve this but the reality is there is a lot of code which uses $! in the ensure block which is incorrect for the reasons already explained.

Your proposed ideas are fine, except that RuboCop will warn you against rescue Exception. I agree with the general sentiment around this.

That being said, there are definitely valid use cases where your ensure block wants to know whether it's exiting via a normal code flow or an exceptional one. To answer your question, ensure => error, error would be the equivalent of a local $!, so nothing really changes, we are just providing a mechanism to do correctly and safely what people are already doing with $!. I don't think there is any valid use case for $! if we introduce the proposed feature here.

[duerst (Martin Dürst)]

Eregon (Benoit Daloze) wrote in #note-3:

If you only need to know if there is an Exception this seems easier:
Using the example from https://bugs.ruby-lang.org/issues/15567#note-27

begin
  ...
rescue Exception => exception
  transaction.abort
  raise exception
ensure
  transaction.commmit unless transaction.aborted?
end

What about

begin
  ...
rescue Exception
  transaction.abort
else
  transaction.commit
end

It seems shorter and easier to understand.

Having read through most of #15567, my impression is that:

• Ruby already has too many features for exceptional flow control. Adding more stuff that makes this even more complicated doesn't look like an improvement of the language. (Go To Statement Considered Harmful, anybody?)
• All the documentation/books that I can remember explain Exceptions and catch/throw completely separately (although usually in close proximity, because both features don't really fit anywhere). Some improvement seems in order, even if this is only "don't use them together".
• ensure contains always executed code. Adding conditions to its syntax seems strange. It would be similar to extending the syntax of else (both in if expressions as well as in begin blocks). Specific conditions should be handled where we already have them, on rescue, e.g. like rescue when throw or some such (if at all).
• The argument that Rubocop complains about something isn't really an argument. If Rubocop is wrong, it should be fixed.

[ioquatix (Samuel Williams)]

Ruby already has too many features for exceptional flow control. Adding more stuff that makes this even more complicated doesn't look like an improvement of the language. (Go To Statement Considered Harmful, anybody?)

We are not implementing any additional flow control.

ensure contains always executed code. Adding conditions to its syntax seems strange. It would be similar to extending the syntax of else (both in if expressions as well as in begin blocks). Specific conditions should be handled where we already have them, on rescue, e.g. like rescue when throw or some such (if at all).

Whether or not you agree with it, people already do it: https://bugs.ruby-lang.org/issues/15567#note-26

I want to make this use case safer, as I was bitten by this bug. The original discussion is a bit different but the surrounding context is similar.

The argument that Rubocop complains about something isn't really an argument. If Rubocop is wrong, it should be fixed.

rescue Exception can produce unusual results that users might not expect, so I think RuboCop is correct here and we shouldn't encourage users to write rescue Exception.

[ioquatix (Samuel Williams)]

Your proposed "shorter and easier to understand" implementation is actually incorrect:

def transaction1
  begin
    yield
  rescue Exception => error
  ensure
    if error
      puts "abort"
    else
      puts "commit"
    end
  end
end

def transaction2
  begin
    yield
  rescue Exception
    puts "abort"
  else
    puts "commit"
  end
end

catch(:foo) do
  transaction1 do
    throw :foo
  end
end

Try for transaction1 and transaction2. The flow control is wrong for transaction2.

[ioquatix (Samuel Williams)]

We want to make it easy for people to write robust Ruby programs, even if there is failure. As discussed in https://bugs.ruby-lang.org/issues/15567 there are some tricky edge cases when using $! because it's non-local state. As an example:

def do_work
  begin # $! may be set on entry to `begin`.
    # ... do work ...
  ensure
    if $!
      puts "work failed"
    else
      puts "work success"
    end
  end
end
 
begin
  raise "Boom"
ensure
  # $! is set.
  do_work
end

There are real examples of this kind of code: https://bugs.ruby-lang.org/issues/15567#note-26

There is another kind of error that can occur:

begin
  raise "Boom"
rescue => error
  # Ignore?
ensure
  pp error # <RuntimeError: Boom>
  pp $! # nil
end

As a general model, something like the following would be incredibly useful, and help guide people in the right direction:

begin
 ...
ensure => error
  pp "error occurred" if error
end

Currently you can almost achieve this:

begin
  ...
rescue Exception => error # RuboCop will complain.
  raise
ensure
  pp "error occurred" if error
end

The limitation of this approach is it only works if you don't need any other rescue clause. Otherwise, it may not work as expected or require extra care (e.g. care with rescue ... => error naming. Also, Rubocop will complain about it, which I think is generally correct since we shouldn't encourage users to rescue Exception.

Because $! can be buggy and encourage incorrect code, I also believe we should consider deprecating it. But in order to do so, we need to provide users with a functional alternative, e.g. ensure => error.

Another option is to clear $! when entering begin block (and maybe restore it on exit?), but this may break existing code.

More Examples

Gem: bundler has 1480241823 downloads.
/lib/bundler/vendor/net-http-persistent/lib/net/http/persistent.rb

  ensure
    return sleep_time unless $!

Gem: rubygems-update has 817517436 downloads.
/lib/rubygems/core_ext/kernel_require.rb

  ensure
    if RUBYGEMS_ACTIVATION_MONITOR.respond_to?(:mon_owned?)
      if monitor_owned != (ow = RUBYGEMS_ACTIVATION_MONITOR.mon_owned?)
        STDERR.puts [$$, Thread.current, $!, $!.backtrace].inspect if $!

Gem: launchy has 196850833 downloads.
/lib/launchy.rb

    ensure
      if $! and block_given? then
        yield $!

Gem: spring has 177806450 downloads.
/lib/spring/application.rb

          ensure
            if $!
              lib = File.expand_path("..", __FILE__)
              $!.backtrace.reject! { |line| line.start_with?(lib) }

Gem: net-http-persistent has 108254000 downloads.
/lib/net/http/persistent.rb

  ensure
    return sleep_time unless $!

Gem: open4 has 86007490 downloads.
/lib/open4.rb

            ensure
              killall rescue nil if $!

Gem: net-ssh-gateway has 74452292 downloads.
/lib/net/ssh/gateway.rb

    ensure
      close(local_port) if block || $!

Gem: vault has 48165849 downloads.
/lib/vault/persistent.rb

  ensure
    return sleep_time unless $!

Gem: webrick has 46884726 downloads.
/lib/webrick/httpauth/htgroup.rb

        ensure
          tmp.close
          if $!

Gem: stripe-ruby-mock has 11533268 downloads.
/lib/stripe_mock/api/server.rb

        ensure
          raise e if $! != e

Gem: logstash-input-udp has 8223308 downloads.
/lib/logstash/inputs/udp.rb

  ensure
    if @udp
      @udp.close_read rescue ignore_close_and_log($!)
      @udp.close_write rescue ignore_close_and_log($!)

Gem: shrine has 6332950 downloads.
/lib/shrine/uploaded_file.rb

      ensure
        tempfile.close! if ($! || block_given?) && tempfile

Discussion:

• nobu: $! is thread-local.
• ko1: Your proposal is to make shorthand for the following code?
  • mame: No

begin
 ...
ensure
  error = $!
  ...
end

• ko1: FYI

def foo
  p $! #=> #<RuntimeError: FOO>
  raise SyntaxError
rescue SyntaxError
end

begin
  raise 'FOO'
ensure
  foo; p $! #=> #<RuntimeError: FOO>
  exit
end

• mame: It is surprising to me that $! is not a method-local but a thread-local variable

at_exit do # |exception| ???
  if $! # exception
    puts "Exiting because of error"
  end
end

begin
  raise "Boom"
ensure
  begin
  ensure => error # should be nil - yes correct. This is the difference between ko1's workaround and samuel's proposal
    p $! #=> #<RuntimeError: Boom>
    error = $!
    puts "failed" if $!
  end
end

• knu:

begin
  Integer("foo")
rescue
  begin
    Integer("1")
  ensure => error
    # You can't tell `$!` came from this begin block without being rescued.
    p $!  #=> #<ArgumentError: invalid value for Integer(): "foo">
    p error #=> nil
  end
end

You can almost achieve the ideal situation with this:

begin
  ...
rescue Exception => error # RuboCop will complain.
  raise
ensure
  pp "error occurred" if error
end

[matz (Yukihiro Matsumoto)]

I hesitate to enhance ensure. It's ugly and against the basic concept of ensure.
The ensure behavior was taken from unwind-protect macro of Lisp, and it does not distinguish between error execution and normal execution. If you check the error condition in the ensure clause, especially using $!, I doubt you misuse the ensure concept.

If you really wanted to check the error condition, you may want to use local variables.

begin
  ....
  no_error = true
ensure
  error_handling unless no_error
end

At the same time, I may underestimate the pain of OP. Let me think about it for a while.

Matz.

[ioquatix (Samuel Williams)]

@matz (Yukihiro Matsumoto) I agree with your general feeling completely. But it's too late: We already have "ugly and against the basic concept of ensure" in the form of $!. As demonstrated, people are checking $! and unfortunately, in general code, it is not safe to check $! in an ensure block because it might come from the parent scope (or some other scope). This puts us in a difficult situation.

I think the decision depends on whether you want to support or deprecate the usage of $!. Fundamentally, if you think that ensure => error is ugly, I believe you must agree ensure ... if $! is equally ugly.

Support $!

If we want to support $!, I would propose that we make it safe, by limiting it to the current block. That means it's effectively reset on entering a begin...ensure...end block, so we prevent it carrying state from the parent scope (if any). There are some potential compatibility issues, and it's still fundamentally ugly... but it might be the easiest option.

Deprecate $!

If we want to deprecate $!, I would propose that we consider how we want engineers to approach the problem they are currently depending on $! for.

The most common use case is something like this:

begin
  resource = acquire # get a resource from the pool or allocate one if pool is empty.
  yield resource
ensure
  if $!
    free(resource) # resource is broken, get rid of it.
  else
    release(resource) # resource is okay, return it to the pool.
  end
end

The proposal to use else is a good one, except it doesn't capture all flow controls:

begin
  resource = acquire # get a resource from the pool or allocate one if pool is empty.
  yield resource
rescue Exception
  free(resource) # resource is broken, get rid of it.
else
  release(resource) # resource is okay, return it to the pool.
end

The problem with the above code, is there are some cases where you can exit the block and neither the rescue nor the else block are executed. It will result in a resource leak. Also, some people would argue that rescue Exception is wrong. So, can we make the above better? If we make else the same as "ensure without exception" then it would be sufficient, but right now it's not. It's also not clear whether the following could ever work:

begin
  resource = acquire # get a resource from the pool or allocate one if pool is empty.
  yield resource
rescue
  free(resource) # resource is broken, get rid of it.
else # Does this mean else if there was no exception, or else if the exception was not handled?
  release(resource) # resource is okay, return it to the pool.
end

The next best alternative is something like this:

begin
  resource = acquire # get a resource from the pool or allocate one if pool is empty.
  yield resource
rescue Exception => error
  free(resource) # resource is broken, get rid of it.
  raise
ensure
  unless error
    release(resource) # resource is okay, return it to the pool.
  end
end

So the fundamental problem we have is that we don't have any way to guarantee, AFAIK, on error execute this code, otherwise execute this other code, and it turns out that a lot of code needs this, essentially "ensure if no exception was raised" or "ensure if no rescue block was executed", or something similar.

By the way, your proposed solution also falls short:

catch(:finished) do
  begin
    throw :finished
    no_error = true
  ensure
    p :error_handling unless no_error
  end
end

Will execute error_handling. This is why it's a non-trivial problem. Everyone comes up with the wrong answer (including me). If you look at real code, you see these incorrect patterns repeated over and over.

In summary, I think user code wants the following: Execute some code, if it fails, do this thing, if it succeeds, do this other thing. Right now, we have: Execute some code, if it fails, do this thing. Whether it failed or not, do this other thing. In this case, flow control operations like break, next, throw/catch are NOT considered failures.

[Eregon (Benoit Daloze)]

I think use-cases needing separate cleanups whether there is a Ruby exception or "normal execution or non-local control flow" will always be ugly, fundamentally because they do something hacky and approximative:
the exception that happens inside is not a proof the resource is at fault, it could be anything, including a NoMemoryError or a SystemStackError for instance (which e.g. could be due to a bug in the block, not related to the resource at all).

So in general, there should be one call to e.g. close in ensure and code should not attempt to differentiate (e.g., what happens for File.open {}).

For the rare cases which do need to differentiate, I think rescue Exception => error + if error is fine.
You literally want to test "any Ruby exception" or "any other exit from this block", and then rescue Exception just makes sense.
It's true one should avoid rescue Exception in general, or at the very least make sure it's always re-raised to not swallow important and possibly unrelated errors.
So RuboCop is right to warn for such usages as they should be avoided in general, and it's worth double-checking if one really needs to rescue Exception.
But as every rule, there are exceptions, and this is a good exception to that rule, if you literally want to differentiate "any Ruby exception" or "any other exit from this block", then rescue Exception is exactly what one wants, just go ahead and suppress the RuboCop warning.

[ioquatix (Samuel Williams)]

the exception that happens inside is not a proof the resource is at fault, it could be anything, including a NoMemoryError or a SystemStackError for instance (which e.g. could be due to a bug in the block, not related to the resource at all).

Yes, it's a fair assessment. But the key point is there is a different from normal exit (which can include throw) and exceptional exit (exception was raised during user code). The reality is, as shown by the examples, lots of code does need to make this differentiation.

You literally want to test "any Ruby exception" or "any other exit from this block", and then rescue Exception just makes sense.

Actually, I'm less concerned about the "any Ruby exception" and more concerned with splitting execution between "normal code flow" and "exceptional code flow", and even that is just as an example. Clearly, lots of people are using $! in an unsafe way, and that's the key point I want to fix. My initial proposal is one way to fix it, by introducing explicit ensure => error. However, I agree with Matz, that it can be considered kind of ugly. But if that's true, why don't we get rid of $!, surely every case for $! can be better handled by explicit rescue? If not, why is $! necessary? If we can answer that question, then I think we will know what path to take.

Personally, I'd vote for the path to get rid of $! because I do agree with Matz that its fundamentally pretty ugly both sematically, as well as practically. I looked at a lot of code that uses $! and it's often used as a poor replacement for the implicit exception cause argument, or for implicit passing error information between code where it should really be explicit.

[Eregon (Benoit Daloze)]

lots of code does need to make this differentiation.

A few places, but in relative usages probably 1/1000 of begin/rescue or ensure/end patterns, right?

Agreed $! should be deprecated and then removed.
The only usage of $! I know of which can't be trivially replaced by => exc is p((abc rescue $!)) and that's 1) unpretty (double parens, modifier rescue) 2) only good for debugging / printing an exception in a quick script.
It's easy enough to begin; expr; rescue => exc; p exc; end (or better, on multiple lines).

Changing $! to be thread+frame local would likely be too incompatible so I think deprecating + removing it is better.

[ioquatix (Samuel Williams)]

@matz (Yukihiro Matsumoto) based on the above discussion, do you agree to deprecate and remove $! if possible?

[Eregon (Benoit Daloze)]

BTW, deprecating and removing $! has another advantage:
a raised Ruby exception wouldn't immediately leak to the current thread/fiber and more optimizations might be possible, including skipping to compute the exception backtrace (the most expensive part of Ruby exceptions) if e.g. a JIT can prove the exception or its backtrace is never used.
That analysis would only require tracking usages of exc in rescue => exc, and not any call in the rescue that might use $!.

Of course, the related $@ (which is $?.backtrace) should be deprecated and removed too.

[ioquatix (Samuel Williams)]

A few places, but in relative usages probably 1/1000 of begin/rescue or ensure/end patterns, right?

I scanned top 10,000 gems by download count:

Checking for "$!":
Found 459 gems with this pattern.
Found 2624 instances of the pattern.

Checking for "ensure":
Found 3221 gems with this pattern.
Found 28306 instances of the pattern.

Checking for /(.*ensure(.*\n){0,4}.*\$!.*)/:
Found 35 gems with potential issues.
Found 46 instances of the pattern.

Any usage of $! is potentially suspect, but at least 35 gems out of 10,000 are definitely incorrect.

[ko1 (Koichi Sasada)]

Another proposal is change $! semantics to `begin/rescue/ensure' clause local.
Does it solve the issues?

[Eregon (Benoit Daloze)]

ko1 (Koichi Sasada) wrote in #note-18:

Another proposal is change $! semantics to `begin/rescue/ensure' clause local.
Does it solve the issues?

That seems elegant actually, because

begin
  ...
rescue
  foo($!)
end

would behave like:

begin
  ...
rescue => exc
  foo(exc)
end

And it would avoid escaping the Exception instance to the thread, which mean it can be escaped analyzed.
Also, if the rescue doesn't use => or $!, there is no need to generate a backtrace which is a huge performance gain!
With current semantics that's difficult to know as any call inside the rescue clause might use $!.

I"m not sure regarding compatibility if e.g. $! is accessed inside a method called from the rescue clause. But that's probably rare and seems bad code, so I think fine to try/experiment.

[ko1 (Koichi Sasada)]

Sorry, my proposal is introducing dynamic scope, not syntactical scope like:

def foo
  p $! #=> A
  begin
    # no raise
  ensure
    p $! #=> nil
  end
end

begin
  raise A
ensure
  foo
end

We can implement raise like method:

def my_raise
  # DO SOMETHING
  raise $!
end

[matz (Yukihiro Matsumoto)]

First, I am not going to change the syntax for the ensure clause. The ensure clause is to unwind-protect so it is not recommended to branch depending on exceptions.

I admit the current $! behavior is error-prone. But considering compatibility, I am not sure we can fix the problem. For the workaround at the moment, avoid using $!. If it is possible to address the issue (e.g. @ko1's proposal of dynamic scoping or giving a warning for the bad usage), I agree with experimenting with them.

Matz.

[Eregon (Benoit Daloze)]

ko1 (Koichi Sasada) wrote in #note-20:

Sorry, my proposal is introducing dynamic scope, not syntactical scope like:

Ah, I misunderstood.

Then I think deprecating $! is the right way, since that seems the only way to be able to analyze decently if an exception's backtrace is needed and do the important optimizations I mentioned in earlier comments.
This would help a lot for making Ruby exceptions faster.
I'm pretty sure @headius (Charles Nutter) would agree since he did a similar optimization, but currently quite limited because of $! being accessible inside calls.

And of course it would avoid the problem of $! having confusing semantics, by telling users to use rescue => exc instead which is clear, simple and clean.

If needed maybe we can avoid warning for rescue $! since that's fairly harmless, but probably that's very rarely used in library/app code (i.e., only used in REPL and for trying things).

---

We could also warn whenever $! is not used syntactically in the rescue clause, but IMHO better deprecate it entirely.

[ioquatix (Samuel Williams)]

Proposal to RuboCop to introduce warning: https://github.com/rubocop/rubocop/issues/10204

I support deprecating $! and $@.

One way: for Ruby 3.x we can create deprecation warning, and remove it in Ruby 4.x.

[ioquatix (Samuel Williams)]

Regarding

foo rescue $!

Maybe we can improve this:

foo rescue => error

I'm not sure it's worth the effort...

Surprisingly this does work:

foo rescue $! => error

[nobu (Nobuyoshi Nakada)]

ioquatix (Samuel Williams) wrote in #note-24:

Surprisingly this does work:

foo rescue $! => error

It is a pattern-matching.

[ioquatix (Samuel Williams)]

Here is a good example of why $! is dangerous:

https://github.com/ruby/net-ftp/pull/24

I believe the merged fix is also wrong, since flow control can exit without closing the connection.

[kddnewton (Kevin Newton)]

If you're already using global variables, we could add another global variable that gets set in ensure blocks that determines if an error was raised. That way you could safely check $!. That wouldn't require a syntax change.

[Dan0042 (Daniel DeLorme)]

ko1 (Koichi Sasada) wrote in #note-20:

my proposal is introducing dynamic scope, not syntactical scope like:

I think this is a good idea, definitely an improvement over the current situation. $! is already always correct inside a rescue clause, and that would make it always correct inside the ensure clause. But I want to confirm if this would be the expected behavior:

#1
p $! #=> A
begin
  # no raise
ensure
  p $! #=> nil (unlike A currently)
end
p $! #=> A (restored after previous begin-ensure block?)

#2
p $! #=> A
begin
  raise "B"
rescue 
  p $! #=> B
  #not re-raised
ensure
  p $! #=> B (unlike A currently)
end
p $! #=> A (restored after previous begin-ensure block?)

[ioquatix (Samuel Williams)]

$! is non-trivial to implement - can have performance implications, and dynamic scope might make it more expensive. cc @Eregon (Benoit Daloze) @headius (Charles Nutter) can we get your input?

Giving $! dynamic scope mitigates the problem but still seems to encourage an approach that @matz (Yukihiro Matsumoto) has said he was against. Specifically, he was against ensure => exception and this is just a variant of that approach.

In the first instance, the question we need to answer is: "Is there a valid use case for $! (assuming dynamic scope as proposed) that isn't better addressed using existing techniques?".

If the answer is YES, we should implement it.

If the answer is NO, we should deprecate and remove $!.

[jeremyevans0 (Jeremy Evans)]

ioquatix (Samuel Williams) wrote in #note-29:

In the first instance, the question we need to answer is: "Is there a valid use case for $! (assuming dynamic scope as proposed) that isn't better addressed using existing techniques?".

If the answer is YES, we should implement it.

If the answer is NO, we should deprecate and remove $!.

This perspective ignores backwards compatibility. If the answer is NO, we would still need to determine if the benefit of removing $! exceeds the cost of the loss of backwards compatibility.

[ioquatix (Samuel Williams)]

Giving $! dynamic scope is equally likely to cause subtle bugs, so no matter what we do here, there is a chance to cause compatibility issues. Deprecation is explicit and gives people a chance to fix the code, which in most cases that I reviewed, is buggy.

[Dan0042 (Daniel DeLorme)]

Giving $! dynamic scope mitigates the problem but still seems to encourage an approach that @matz (Yukihiro Matsumoto) has said he was against. Specifically, he was against ensure => exception and this is just a variant of that approach.

I believe Matz said he was against adding extra syntax, and that he was open to experimenting with ko1's proposal.

Giving $! dynamic scope is indeed likely to cause subtle bugs, just less than currently. I think that almost all code currently written with $! in the ensure clause assume the exception is triggered within the same begin-end block. Using a dynamic scope would "magically" fix all of this currently-incorrect code. So it's a net win.

And considering backwards compatibility I think it's unrealistic to consider deprecation. $! is used fairly often in gems. I also use it fairly often in my code, usually in some variants of the form expr rescue $!