Fix RubyThread.join to set "$!" in the calling thread when the target

subbuss · subbuss · commit 20632af6e1fa · 2012-05-08T17:11:08.000-05:00
thread exits with an exception.  This lets us gets rid of a hack in both
AST and IR interpreters where on catching an exception, "$!" is set
before passing control to the rescue handlers.  This in turn makes the
exception handlers in IR interpreter loop identical for a couple
scenarios which can then let us compact exception tables for the IR JIT.
diff --git a/src/org/jruby/RubyThread.java b/src/org/jruby/RubyThread.java
@@ -612,6 +612,8 @@ public IRubyObject join(IRubyObject[] args) {
         }
 
         if (exitingException != null) {
+            // Set $! in the current thread before exiting
+            getRuntime().getGlobalVariables().set("$!", (IRubyObject)exitingException.getException());
             throw exitingException;
         }
 
diff --git a/src/org/jruby/ast/RescueNode.java b/src/org/jruby/ast/RescueNode.java
@@ -170,11 +170,6 @@ private IRubyObject interpretWithJavaExceptions(Ruby runtime, ThreadContext cont
 
     private IRubyObject handleException(Ruby runtime, ThreadContext context, IRubyObject self, Block aBlock, RaiseException raiseJump) {
         RubyException raisedException = raiseJump.getException();
-        // TODO: Rubicon TestKernel dies without this line.  A cursory glance implies we
-        // falsely set $! to nil and this sets it back to something valid.  This should 
-        // get fixed at the same time we address bug #1296484.
-        runtime.getGlobalVariables().set("$!", raisedException);
-
         RescueBodyNode cRescueNode = rescueNode;
 
         while (cRescueNode != null) {
diff --git a/src/org/jruby/ir/interpreter/Interpreter.java b/src/org/jruby/ir/interpreter/Interpreter.java
@@ -400,7 +400,7 @@ private static IRubyObject interpret(ThreadContext context, IRubyObject self,
                     switch(operation) {
                     case PUSH_BINDING: {
                         // SSS FIXME: Blocks are a headache -- so, these instrs. are only added to IRMethods
-                        // They have more complicated logic for pushing a dynamic scope (see InterpretedIRBlockBody)
+                        // Blocks have more complicated logic for pushing a dynamic scope (see InterpretedIRBlockBody)
                         currDynScope = DynamicScope.newDynamicScope(scope.getStaticScope());
                         context.pushScope(currDynScope);
                         ipc++;
@@ -620,11 +620,6 @@ private static IRubyObject interpret(ThreadContext context, IRubyObject self,
                 if (ipc == -1) throw re; // No one rescued exception, pass it on!
 
                 exception = re.getException();
-                // SSS: Copied this comment and line from ast/RescueNode.java:handleException
-                // TODO: Rubicon TestKernel dies without this line.  A cursory glance implies we
-                // falsely set $! to nil and this sets it back to something valid.  This should 
-                // get fixed at the same time we address bug #1296484.
-                runtime.getGlobalVariables().set("$!", (IRubyObject)exception);
             } catch (Throwable t) {
                 if (t instanceof Unrescuable) {
                     // ThreadKill, RubyContinuation, MainExitException, etc.
@@ -660,6 +655,15 @@ private static void handleNonLocalReturn(ThreadContext context, IRScope scope, R
 
         if (inClosure) {
             if (methodToReturnFrom == null) {
+                // SSS FIXME: As Tom correctly pointed out, this is not correct.  The example that breaks this code is:
+                //
+                //      jruby -X-CIR -e "Thread.new { Proc.new { return }.call }.join"
+                //
+                // This should report a LocalJumpError, not a ThreadError.
+                //
+                // The right fix would involve checking the closure to see who it is associated with.
+                // If it is a thread-body, it would be a ThreadError.  If not, it would be a local-jump-error
+                // This requires having access to the block -- same requirement as in handleBreakJump.
                 if (context.getThread() == context.getRuntime().getThreadService().getMainThread()) {
                     throw IRException.RETURN_LocalJumpError.getException(context.getRuntime());
                 } else {

Original file line number	Diff line number	Diff line change
`@@ -612,6 +612,8 @@ public IRubyObject join(IRubyObject[] args) {`
`612`	`612`	`}`
`613`	`613`
`614`	`614`	`if (exitingException != null) {`
	`615`	`+ // Set $! in the current thread before exiting`
	`616`	`+ getRuntime().getGlobalVariables().set("$!", (IRubyObject)exitingException.getException());`
`615`	`617`	`throw exitingException;`
`616`	`618`	`}`
`617`	`619`