Cannot rescue from LocalJumpError after wrongly using break in proc

[ivoanjo]

Environment

• JRuby: jruby 9.1.12.0 (2.3.3) 2017-06-15 33c6439 Java HotSpot(TM) 64-Bit Server VM 25.131-b11 on 1.8.0_131-b11 [linux-x86_64]
• Kernel: Linux maruchan 4.11.0-041100-generic #201705041534 SMP Thu May 4 19:36:05 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
• Distro: Ubuntu 17.04

Expected Behavior

The following test case causes a LocalJumpError exception to be raised, and then properly handles it.

block_with_break = proc { break :foo }

begin
  block_with_break.call
rescue Exception => e
  puts "Handled #{e} (#{e.class}) successfully!"
end

puts 'done'

On MRI this works as expected:

$ ruby -v
ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-linux]
$ ruby test.rb 
Handled break from proc-closure (LocalJumpError) successfully!
done

Actual Behavior

Under JRuby, the code does not seem to be able to catch the LocalJumpError:

$ jruby -v
jruby 9.1.12.0 (2.3.3) 2017-06-15 33c6439 Java HotSpot(TM) 64-Bit Server VM 25.131-b11 on 1.8.0_131-b11 +jit [linux-x86_64]
$ jruby test.rb 
LocalJumpError: unexpected break
(vm exits)

The test case is pretty strange; the real use case where I've bumped into this issue is an app that took a block and created a future from it with concurrent-ruby. On a rare code path it would call an erroneous break, and because concurrent-ruby could not rescue and mark the background task as failed, the task would appear like it would never finish.

[headius]

A similar or nearly-identical case was found in ruby/spec and reported by @eregon in #4577 but we had not fixed it because of the lack of any real-world use case. Because the erroneous break should be an error in any case, it had not been a real concern to create the Ruby exception a bit lazily.

However, your case shows that there might be third-party code that doesn't behave, a code path that wasn't tested, or a piece of code that worked ok in one context but does not in another. In these cases, not propagating LJE as soon as possible makes it difficult to perform appropriate cleanup.

I will point out, however, that ensure blocks should always still fire, and anything that should definitely happen before a concurrent-ruby task (for example) finishes executing, it should be in an ensure.

Marking for 9.2, since I think we should tidy this up for many reasons.

[headius]

Here's a naive fix that searches the local thread context for the scope we're supposed to return to. This obviously adds overhead but I'm not sure the best way in IR to figure out that a proc has gone out of scope and needs to start raising LJE.

@subbuss might have some ideas about how to handle this cases, where it's a valid break initially but becomes invalid once uprooted from the runtime state.

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
index 6ccba2f6db..3ddad78aa2 100644
--- a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
+++ b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -166,6 +166,10 @@ public class IRRuntimeHelpers {
 
         IRScopeType scopeType = ensureScopeIsClosure(context, dynScope);
 
+        if (context.isScopeActive(dynScope.getParentScope())) {
+            throw context.runtime.newLocalJumpError(RubyLocalJumpError.Reason.BREAK, breakValue, "unexpected break");
+        }
+
         // Raise a break jump so we can bubble back down the stack to the appropriate place to break from.
         throw IRBreakJump.create(dynScope.getParentScope(), breakValue, scopeType.isEval()); // weirdly evals are impld as closures...yes yes.
     }
diff --git a/core/src/main/java/org/jruby/runtime/ThreadContext.java b/core/src/main/java/org/jruby/runtime/ThreadContext.java
index 4d59343c53..f04f4eeb44 100644
--- a/core/src/main/java/org/jruby/runtime/ThreadContext.java
+++ b/core/src/main/java/org/jruby/runtime/ThreadContext.java
@@ -335,6 +335,13 @@ public final class ThreadContext {
         scopeStack[scopeIndex--] = null;
     }
 
+    public boolean isScopeActive(DynamicScope scope) {
+        for (int i = scopeIndex; i > 0; i--) {
+            if (scopeStack[i] == scope) return true;
+        }
+        return false;
+    }
+
     private void expandScopeStack() {
         int newSize = scopeStack.length * 2;
         DynamicScope[] newScopeStack = new DynamicScope[newSize];

FWIW, simple breaks that don't span a huge number of frames should have pretty low overhead, but ideally we'd have none.

[headius]

Idea: add an "escaped" or "dead" bit to DynamicScope that we set when it is popped. Adds overhead to every pop (field set, virtual call). Unsure if this would be less overall overhead than the search.

I've used the search in the past to look for current block's frame for similar reasons.

[headius]

Apparently that patch breaks a few things too, so it's a bit too deep.

[subbuss]

Maybe check if

jruby/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java

Lines 228 to 232 in 154275c

	/* ---------------------------------------------------------------
	* SSS FIXME: Puzzled .. Why is this not needed?
	} else if (!context.scopeExistsOnCallStack(bj.scopeToReturnTo.getStaticScope())) {
	throw IRException.BREAK_LocalJumpError.getException(context.runtime);
	* --------------------------------------------------------------- */

might handle this ... that is really old code, so some of the types and checks might be old and need adapting.

[subbuss]

Nah, I don't think that will fix this since at the time of the break, the proc is still on the stack, and hence valid.

[subbuss]

Three potential places to handle this:
(a) RubyProc.java where you check if the Proc is the scope the IRBreakJump is supposed to return to .. and if so, throw an LJE there. But adds overhead to every proc since you are wrapping it with a try-catch
(b) Add an check to the interpreter try-catch (InterpreterEngine.java and StartupInterpreterEngine.java where you check for an exception handler) to check if the exception is an IRBreakJump and and if yes, if the target scope exists on the call stack <-- adds overhead to every exception and every break (but, may be acceptable). Have to do equivalent in the JIT too.
(c) I've forgotten some of the ruby semantics now :( but, if this is a valid check ... at the time of setting up the IRBreakJump, if you know the target scope is a Proc, is it always a LJE .. if so, throw there.

If (c) is valid, that is the best. If not, (b) is probably the lower overhead approach.

[eregon]

Note that the LocalJumpError should be thrown at the place of the break to be compatible with MRI:

> proc { begin; break 42; rescue; p :rescued; end }.call
:rescued

[subbuss]

My original code for all this was for ruby 1.8/1.9 and not sure if semantics have changed a bit since then, or if this has always been broken.

But, anyway, eregon's comment indicates that a break in a proc unconditionally throws a LJE?

[eregon]

No, it throws a LocalJumpError only if the block is invoked by proc (which could be aliased of course), as that would break out of the method defining the Proc which is kind of weird.

> def m(&b); b.call; end; m { break 42 }
=> 42

So this is related to break not allowed to go back and breaking from the proc definition:

pr = proc { break 42 }
# The break would return 42 for "proc { break 42 }" and jump back in time if this worked!
pr.call

[subbuss]

Right, I meant: break in a proc not, the generic proc .. but, aliasing. hmm.

[subbuss]

So, @headius your original hunch is right ... this has to be handled in initiateBreak by examining the call stack. IRBreakJump.create(dynScope.getParentScope() ... is what creates the IRBreakJump .. So, that has to check to ensure that dynScope.getParentScope() doesn't occur deeper in the call stack than a 'Proc.call'. If it does, throw an LJE. So, this might even require adding a flag to the ThreadContext stack objects to indicate when the DynamicScope corresponds to a proc.call (after resolving aliases).

[headius]

Since it appears that the LJE is thrown no matter what once the block has escaped, I think we have some better options. We already do mark blocks as "escaped" once they've been captured into a proc object. We just need to have access to that (probably via frame right now) and check at the time we initiate the break whether we should LJE or not.

[subbuss]

WFM.

[headius]

So a bit of a snag here: we're not setting blocks as escaped properly anymore.

It appears to only be done via CachingCallSite.callIter now which isn't used by any call paths I can see. I had assumed we no longer needed this logic because IR was doing the right thing in all cases we cared about.

This patch makes all specs and known examples pass (including @eregon's case) but obviously this only fixes it in the interpreter.

diff --git a/core/src/main/java/org/jruby/ir/instructions/CallBase.java b/core/src/main/java/org/jruby/ir/instructions/CallBase.java
index 0628b32d90..c940645d26 100644
--- a/core/src/main/java/org/jruby/ir/instructions/CallBase.java
+++ b/core/src/main/java/org/jruby/ir/instructions/CallBase.java
@@ -425,6 +425,14 @@ public abstract class CallBase extends NOperandInstr implements ClosureAccepting
         IRubyObject[] values = prepareArguments(context, self, currScope, dynamicScope, temp);
         Block preparedBlock = prepareBlock(context, self, currScope, dynamicScope, temp);
 
+        if (getClosureArg() != null) {
+            try {
+                return callSite.call(context, self, object, values, preparedBlock);
+            } finally {
+                preparedBlock.escape();
+            }
+        }
+
         return callSite.call(context, self, object, values, preparedBlock);
     }
 
diff --git a/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
index 0d606b49fb..730a33bc27 100644
--- a/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
+++ b/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
@@ -433,7 +433,7 @@ public class InterpreterEngine {
                 // This assumes that scopes with break instr. have a frame / dynamic scope
                 // pushed so that we can get to its static scope. For-loops now always have
                 // a dyn-scope pushed onto stack which makes this work in all scenarios.
-                return IRRuntimeHelpers.initiateBreak(context, currDynScope, rv, blockType);
+                return IRRuntimeHelpers.initiateBreak(context, currDynScope, rv, block, blockType);
             }
             case NONLOCAL_RETURN: {
                 NonlocalReturnInstr ri = (NonlocalReturnInstr)instr;
diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
index 6ccba2f6db..fecb9c9eb9 100644
--- a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
+++ b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -159,15 +159,21 @@ public class IRRuntimeHelpers {
     }
 
     // FIXME: When we recompile lambdas we can eliminate this binary code path and we can emit as a NONLOCALRETURN directly.
-    public static IRubyObject initiateBreak(ThreadContext context, DynamicScope dynScope, IRubyObject breakValue, Block.Type blockType) throws RuntimeException {
+    public static IRubyObject initiateBreak(ThreadContext context, DynamicScope dynScope, IRubyObject breakValue, Block block, Block.Type blockType) throws RuntimeException {
         // Wrap the return value in an exception object and push it through the break exception
         // paths so that ensures are run, frames/scopes are popped from runtime stacks, etc.
         if (inLambda(blockType)) throw new IRWrappedLambdaReturnValue(breakValue);
 
         IRScopeType scopeType = ensureScopeIsClosure(context, dynScope);
 
+        DynamicScope parentScope = dynScope.getParentScope();
+
+        if (block.isEscaped()) {
+            throw context.runtime.newLocalJumpError(RubyLocalJumpError.Reason.BREAK, breakValue, "unexpected break");
+        }
+
         // Raise a break jump so we can bubble back down the stack to the appropriate place to break from.
-        throw IRBreakJump.create(dynScope.getParentScope(), breakValue, scopeType.isEval()); // weirdly evals are impld as closures...yes yes.
+        throw IRBreakJump.create(parentScope, breakValue, scopeType.isEval()); // weirdly evals are impld as closures...yes yes.
     }
 
     // Are we within the scope where we want to return the value we are passing down the stack?

Given that we already do have the callIter try/finally logic in CachingCallSite, I'll see what it would take to hook this up there and in indy.

[subbuss]

Is there is a way to detect 'if (block.isEscaped())' without having to set a flag?

[enebo]

Too bad we cannot instantiate a new instance for a block which escapes and then have an intiateBreak() on it. Then we would have a bi-morphic call and no conditional/flag.

The flag by itself makes me wonder if we already dup/clone the Block since I can campture the same block in a proc and non-proc context at the same time.

[enebo]

Poking holes in my own idea...object instantiation itself is not cheap and I don't know how many places we may retain the original reference and whether there being two instances matters or not (if we do retain a reference somewhere).

[headius]

Two commits are in on fix_break_lje branch that set literal closures passed to calls as escaped once the call returns. This fixes all known specs and examples related to this issue.

In the case of indy, it is implemented by wrapping the call in try/finally logic via method handles, which may not optimize well on older JVMs. Java 8 will do better, and 9 has introduced a tryFinally handle we can use.

[headius]

Note this affects super calls, and I have not done any fixes for that. They follow different invocation logic that will need to propagate whether a literal closure needs to be escaped.

[headius]

I have fixed the given cases. The remaining super cases are broken but we will get back to that some other time (perhaps when it is reported as an error, since it's likely to be very rare).

@ivoanjo Please verify master. Thank you!

[ivoanjo]

Thanks so much @headius, @enebo and @subbuss for the great implementation brainstorm and quick fix!

I can confirm that my original test case (and the real use case) using concurrent-ruby works correctly with current master.

But one other test case I produced when I was trying to create the simplest reproduction test case still seems to be broken, so I will open a separate issue for that one.

[headius]

Reopening to backport to 9.1.14.