Project

General

Profile

Actions

Bug #13930

closed

Exception is caught in rescue above ensure

Added by msauter (Michael Sauter) over 3 years ago. Updated 17 days ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
2.4.1p111 (2017-03-22 revision 58053) [x86_64-linux]
[ruby-core:82936]

Description

Given the following code:

def foo
  i = 0
  3.times do |n|
    begin
      puts "a"
      i += 1
      next if i > 1
      puts "b"
    rescue => e
      puts "rescue in foo, caught #{e}"
    ensure
      puts "ensure in foo, yield from foo: #{n}"
      yield n
    end
  end
end

begin
  x = 0
  foo do |o|
    puts o
    x += 1
    raise "test" if x > 1
    puts "done yielding"
  end
rescue => e
  puts "rescue outside, caught #{e}"
ensure
  puts "ensure outside"
end

The output is:

a
b
ensure in foo, yield from foo: 0
0
done yielding
a
ensure in foo, yield from foo: 1
1
rescue in foo, caught test
ensure in foo, yield from foo: 1
1
rescue outside, caught test
ensure outside

So the exception raised within the yielded block is caught in the rescue block above the ensure block which yielded. That sounds wrong to me. Or is it intended? The issue seems to be with the usage of next. Also, exception is caught inside AND outside as it seems that the ensure block ends up being called twice ?!

If I change the code to this:

def foo
  i = 0
  3.times do |n|
    begin
      puts "a"
      i += 1
      # next if i > 1
      puts "b"
    rescue => e
      puts "rescue in foo, caught #{e}"
    ensure
      puts "ensure in foo, yield from foo: #{n}"
      yield n
    end
  end
end

begin
  x = 0
  foo do |o|
    puts o
    x += 1
    raise "test" if x > 1
    puts "done yielding"
  end
rescue => e
  puts "rescue outside, caught #{e}"
ensure
  puts "ensure outside"
end

Then the output is:

a
b
ensure in foo, yield from foo: 0
0
done yielding
a
b
ensure in foo, yield from foo: 1
1
rescue outside, caught test
ensure outside

I would have expected this output also when using next as above.


Files

bug13930.disasm (3.62 KB) bug13930.disasm wanabe (_ wanabe), 10/28/2018 08:44 AM
bug13930.patch (492 Bytes) bug13930.patch wanabe (_ wanabe), 10/28/2018 09:00 AM

Related issues

Related to Ruby master - Bug #16618: Ensure called twice when raise in ensureClosedActions
Actions #1

Updated by msauter (Michael Sauter) over 3 years ago

  • Description updated (diff)
Actions #2

Updated by msauter (Michael Sauter) over 3 years ago

  • Description updated (diff)
Actions #3

Updated by msauter (Michael Sauter) over 3 years ago

  • Description updated (diff)
Actions #4

Updated by shyouhei (Shyouhei Urabe) over 3 years ago

  • Description updated (diff)

Updated by wanabe (_ wanabe) over 2 years ago

I think it's due to compile_next() and add_ensure_iseq().

The following is a reduced script result.

$ ruby -e 'iseq = RubyVM::InstructionSequence.compile("1.times do; begin; p :loop; next; rescue; p :NG_rescue; ensure; raise \"test\"; end; end rescue p $!"); File.write("bug13930.disasm", iseq.disasm); iseq.eval'
:loop
:NG_rescue
#<RuntimeError: test>

Attached "bug13930.disasm" is the disasm result.
"catch type: rescue st: 0001 ed: 0019" may be wrong and it might be a good to be "ed: 0006" or "ed: 0008".
Because "0009 putself" is the receiver of "0012 opt_send_without_block " in ensure clause.

I wrote "bug13930.patch" but this is incorrect because:

  1. The patch has a performance issue because of throw instruction.
    • I haven't measured benchmark yet and I don't know how.
  2. "next in while loop" pattern should be also fixed.
  3. compile_break() / compile_redo() / compile_return() will have same issue because they use add_ensure_iseq().
    • But I can't fix them because of "break from proc-closure" error.
Actions #6

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Tags set to wip-patch, core
Actions #7

Updated by jeremyevans0 (Jeremy Evans) about 1 year ago

  • Related to Bug #16618: Ensure called twice when raise in ensure added

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

  • Assignee set to ko1 (Koichi Sasada)
  • Status changed from Open to Assigned

Thanks to wanabe (_ wanabe)'s insight that this issue is related to the scope of the rescue catch entry, I came up with a pull request that avoids the issues wanabe (_ wanabe) identified in his patch: https://github.com/ruby/ruby/pull/4291. Instead of trying to fix things cleanly (which I don't know how to do), just adjust the scope of the rescue catch entry so it doesn't cover the ensure block. This approach fixes the original example and wanabe (_ wanabe)'s simplified example in this issue, as well as all failing examples in #16618. The heuristic this approach uses may not fix all issues, and it may cause issues, but with my limited knowledge of the compiler I cannot be certain of either. However, the pull request does pass all existing tests.

Actions #9

Updated by ko1 (Koichi Sasada) 17 days ago

  • Status changed from Assigned to Closed

Applied in changeset git|609de71f043e8ba34f22b9993e444e2e5bb05709.


fix raise in exception with jump

add_ensure_iseq() adds ensure block to the end of
jump such as next/redo/return. However, if the rescue
cause are in the body, this rescue catches the exception
in ensure clause.

iter do
next
rescue
R
ensure
raise
end

In this case, R should not be executed, but executed without this patch.

Fixes [Bug #13930]
Fixes [Bug #16618]

A part of tests are written by @jeremyevans https://github.com/ruby/ruby/pull/4291

Actions

Also available in: Atom PDF