Save/restore $! properly in the JRuby runtime

* Whenever a RaiseException is caught and handled, the $! should be the restored to the previous value of the exception before the try-catch was entered. * This fixes jruby#1601 and jruby#2491 but additional code auditing and discussion will be helpful.
kares · Feb 14, 2015 · 78e968e · 78e968e
1 parent 6b2cc1b
commit 78e968e
Show file tree

Hide file tree

Showing 11 changed files with 25 additions and 13 deletions.
diff --git a/core/src/main/java/org/jruby/Main.java b/core/src/main/java/org/jruby/Main.java
@@ -438,12 +438,14 @@ private boolean checkFileSyntax(Ruby runtime, String filename) {
     }
 
     private boolean checkStreamSyntax(Ruby runtime, InputStream in, String filename) {
+        IRubyObject oldExc = runtime.getGlobalVariables().get("$!");
         try {
             runtime.parseFromMain(in, filename);
             config.getOutput().println("Syntax OK");
             return true;
         } catch (RaiseException re) {
             if (re.getException().getMetaClass().getBaseName().equals("SyntaxError")) {
+                runtime.getGlobalVariables().set("$!", oldExc);
                 config.getError().println("SyntaxError in " + re.getException().message(runtime.getCurrentContext()));
             } else {
                 throw re;

diff --git a/core/src/main/java/org/jruby/Ruby.java b/core/src/main/java/org/jruby/Ruby.java
@@ -3228,6 +3228,7 @@ public void tearDown(boolean systemExit) {
 
         while (!atExitBlocks.empty()) {
             RubyProc proc = atExitBlocks.pop();
+            IRubyObject oldExc = context.runtime.getGlobalVariables().get("$!");
             try {
                 proc.call(getCurrentContext(), IRubyObject.NULL_ARRAY);
             } catch (RaiseException rj) {
@@ -3242,6 +3243,8 @@ public void tearDown(boolean systemExit) {
                         status = RubyNumeric.fix2int(statusObj);
                     }
                 }
+                // Reset $! now that rj has been handled
+                context.runtime.getGlobalVariables().set("$!", oldExc);
             }
         }
 

diff --git a/core/src/main/java/org/jruby/RubyBasicObject.java b/core/src/main/java/org/jruby/RubyBasicObject.java
@@ -1150,8 +1150,7 @@ public IRubyObject op_not_equal(ThreadContext context, IRubyObject other) {
      */
     @Override
     public int compareTo(IRubyObject other) {
-        // SSS FIXME: How do we get access to the runtime here?
-        // IRubyObject oldExc = runtime.getGlobalVariables().get("$!");
+        IRubyObject oldExc = getRuntime().getGlobalVariables().get("$!");
         try {
             IRubyObject cmp = invokedynamic(getRuntime().getCurrentContext(),
                     this, OP_CMP, other);
@@ -1161,7 +1160,7 @@ public int compareTo(IRubyObject other) {
                 return (int) cmp.convertToInteger().getLongValue();
             }
         } catch (RaiseException ex) {
-            // runtime.getGlobalVariables().set("$!", oldExc);
+            getRuntime().getGlobalVariables().set("$!", oldExc);
         }
 
         /* We used to raise an error if two IRubyObject were not comparable, but

diff --git a/core/src/main/java/org/jruby/RubyClass.java b/core/src/main/java/org/jruby/RubyClass.java
@@ -684,11 +684,12 @@ private static IRubyObject checkFuncallMissing(ThreadContext context, RubyClass
             return null;
         }
         else {
+            IRubyObject oldExc = runtime.getGlobalVariables().get("$!");
             try {
                 return checkFuncallExec(context, self, method, args);
             } catch (RaiseException e) {
-                // clear $!
-                context.setErrorInfo(context.nil);
+                // restore $!
+                runtime.getGlobalVariables().set("$!", oldExc);
                 return checkFuncallFailed(context, self, method, runtime.getNoMethodError(), args);
             }
         }

diff --git a/core/src/main/java/org/jruby/RubyComparable.java b/core/src/main/java/org/jruby/RubyComparable.java
@@ -149,7 +149,6 @@ private static IRubyObject callCmpMethod(ThreadContext context, IRubyObject recv
             if (e.getException().kind_of_p(context, runtime.getStandardError()).isTrue()) {
                 // clear error info resulting from failure to compare (JRUBY-3292)
                 runtime.getGlobalVariables().set("$!", savedError);
-                context.setErrorInfo(runtime.getNil()); // SSS FIXME: Is this correct?
                 return returnValueOnError;
             } else {
                 throw e;

diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
@@ -860,11 +860,14 @@ public void undef(ThreadContext context, String name) {
 
         if (name.equals("method_missing")) {
 
+            IRubyObject oldExc = runtime.getGlobalVariables().get("$!");
             try {
                 removeMethod(context, name);
             } catch (RaiseException t) {
-                if(!(t.getException() instanceof RubyNameError)) {
+                if (!(t.getException() instanceof RubyNameError)) {
                     throw t;
+                } else {
+                    runtime.getGlobalVariables().set("$!", oldExc);
                 }
             }
             return;

diff --git a/core/src/main/java/org/jruby/RubyNumeric.java b/core/src/main/java/org/jruby/RubyNumeric.java
@@ -470,9 +470,9 @@ protected final RubyArray doCoerce(ThreadContext context, IRubyObject other, boo
             warnings.warn("in the next release. Return nil in #coerce if the coercion is impossible.");
             if (err) {
                 coerceFailed(context, other);
-            } else {
-                context.runtime.getGlobalVariables().set("$!", savedError);
             }
+            // Restore $!
+            context.runtime.getGlobalVariables().set("$!", savedError);
             return null;
         }
 

diff --git a/core/src/main/java/org/jruby/RubyTime.java b/core/src/main/java/org/jruby/RubyTime.java
@@ -1216,17 +1216,21 @@ protected static RubyTime s_mload(IRubyObject recv, RubyTime time, IRubyObject f
 
         int offset = 0;
         if (offsetVar != null && offsetVar.respondsTo("to_int")) {
+            IRubyObject oldExc = runtime.getGlobalVariables().get("$!");
             try {
                 offset = offsetVar.convertToInteger().getIntValue() * 1000;
             } catch (RaiseException typeError) {
+                runtime.getGlobalVariables().set("$!", oldExc);
             }
         }
 
         String zone = "";
         if (zoneVar != null && zoneVar.respondsTo("to_str")) {
+            IRubyObject oldExc = runtime.getGlobalVariables().get("$!");
             try {
                 zone = zoneVar.convertToString().toString();
             } catch (RaiseException typeError) {
+                runtime.getGlobalVariables().set("$!", oldExc);
             }
         }
 

diff --git a/core/src/main/java/org/jruby/ir/interpreter/BodyInterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/BodyInterpreterEngine.java
@@ -199,10 +199,7 @@ public IRubyObject interpret(ThreadContext context, IRubyObject self, Interprete
                             ipc = instr.interpretAndGetNewIPC(context, currDynScope, currScope, self, temp, ipc);
                         } else {
                             Object result = instr.interpret(context, currScope, currDynScope, self, temp);
-
-                            if (instr instanceof ResultInstr) {
-                                setResult(temp, currDynScope, ((ResultInstr) instr).getResult(), result);
-                            }
+                            setResult(temp, currDynScope, instr, result);
                         }
                 }
             } catch (Throwable t) {

diff --git a/core/src/main/java/org/jruby/javasupport/Java.java b/core/src/main/java/org/jruby/javasupport/Java.java
@@ -953,6 +953,7 @@ private static RubyModule getProxyOrPackageUnderPackage(ThreadContext context, f
             } catch (RaiseException re) { /* expected */
                 RubyException rubyEx = re.getException();
                 if (rubyEx.kind_of_p(context, runtime.getStandardError()).isTrue()) {
+                    // SSS FIXME: Why is this being done conditionally??
                     Helpers.setErrorInfo(runtime, previousErrorInfo);
                 }
             } catch (Exception e) { /* expected */ }
@@ -1019,6 +1020,7 @@ private static RubyModule getTopLevelProxyOrPackage(ThreadContext context, final
             } catch (RaiseException re) { /* not primitive or lc class */
                 RubyException rubyEx = re.getException();
                 if (rubyEx.kind_of_p(context, runtime.getStandardError()).isTrue()) {
+                    // SSS FIXME: Why is this being set to nil??
                     Helpers.setErrorInfo(runtime, runtime.getNil());
                 }
             } catch (Exception e) { /* not primitive or lc class */ }

diff --git a/core/src/main/java/org/jruby/util/SunSignalFacade.java b/core/src/main/java/org/jruby/util/SunSignalFacade.java
@@ -81,6 +81,7 @@ private JRubySignalHandler(Ruby runtime, IRubyObject block, BlockCallback callba
 
         public void handle(Signal signal) {
             ThreadContext context = runtime.getCurrentContext();
+            IRubyObject oldExc = runtime.getGlobalVariables().get("$!");
             try {
                 if (block != null) {
                     block.callMethod(context, "call");
@@ -92,6 +93,7 @@ public void handle(Signal signal) {
                     runtime.getThread().callMethod(context, "main")
                         .callMethod(context, "raise", e.getException());
                 } catch(Exception ignored) {}
+                runtime.getGlobalVariables().set("$!", oldExc);
             } catch (MainExitException mee) {
                 runtime.getThreadService().getMainThread().kill();
             } finally {