Project

General

Profile

Actions

Feature #18083

open

Capture error in ensure block.

Added by ioquatix (Samuel Williams) 2 months ago. Updated 5 days ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:104960]

Description

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.


Related issues

Related to Ruby master - Feature #15567: Allow ensure to match specific situationsRejectedioquatix (Samuel Williams)Actions

Updated by ioquatix (Samuel Williams) 2 months ago

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

Updated by Eregon (Benoit Daloze) 2 months ago

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?

Updated by Eregon (Benoit Daloze) 2 months ago

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
Actions #4

Updated by Eregon (Benoit Daloze) 2 months ago

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

Updated by ioquatix (Samuel Williams) 2 months ago

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.

Updated by duerst (Martin Dürst) 2 months ago

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.

Updated by ioquatix (Samuel Williams) 2 months ago

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.

Updated by ioquatix (Samuel Williams) 2 months ago

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.

Updated by ioquatix (Samuel Williams) 2 months ago

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

Updated by matz (Yukihiro Matsumoto) 2 months ago

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.

Updated by ioquatix (Samuel Williams) 2 months ago

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.

Updated by Eregon (Benoit Daloze) 2 months ago

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.

Updated by ioquatix (Samuel Williams) 2 months ago

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.

Updated by Eregon (Benoit Daloze) 2 months ago

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.

Updated by ioquatix (Samuel Williams) 2 months ago

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

Updated by Eregon (Benoit Daloze) 2 months ago

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.

Updated by ioquatix (Samuel Williams) 2 months ago

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.

Updated by ko1 (Koichi Sasada) 7 days ago

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

Updated by Eregon (Benoit Daloze) 6 days ago

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.

Updated by ko1 (Koichi Sasada) 5 days ago

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

Updated by matz (Yukihiro Matsumoto) 5 days ago

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 (Koichi Sasada)'s proposal of dynamic scoping or giving a warning for the bad usage), I agree with experimenting with them.

Matz.

Updated by Eregon (Benoit Daloze) 5 days ago

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.

Updated by ioquatix (Samuel Williams) 5 days ago

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.

Updated by ioquatix (Samuel Williams) 5 days ago

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

Updated by nobu (Nobuyoshi Nakada) 5 days ago

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

Surprisingly this does work:

foo rescue $! => error

It is a pattern-matching.

Actions

Also available in: Atom PDF