Fix for JRUBY-4677: Java exceptions can't be rescued with "rescue Exc…

…eption" This fixes the problem by modifying all rescue blocks to handle Java exceptions when they're rescueing "Exception" as well as handling Ruby exceptions. It feels like the way it should work from the Ruby side, but I worry about it from the Java side. I had to introduce a new Unrescuable marker to allow some internal exceptions to propagate. All tests pass, specs pass the same.
prathamesh-sonpatki · Apr 6, 2010 · 25e40db · 25e40db
1 parent 1dfd560
commit 25e40db
Show file tree

Hide file tree

Showing 8 changed files with 110 additions and 48 deletions.
diff --git a/lib/ruby/site_ruby/shared/builtin/java/java.lang.rb b/lib/ruby/site_ruby/shared/builtin/java/java.lang.rb
@@ -30,8 +30,13 @@ def <=>(a)
   end
 end
 
-class java::lang::Exception
+class << java::lang::Exception
-  def self.===(rhs)
+  alias :old_eqq :===
-    (NativeException == rhs.class) && (java_class.assignable_from?(rhs.cause.java_class))
+  def ===(rhs)
+    if (NativeException == rhs.class) && (java_class.assignable_from?(rhs.cause.java_class))
+      true
+    else
+      old_eqq(rhs)
+    end
   end
 end
diff --git a/src/org/jruby/RubyException.java b/src/org/jruby/RubyException.java
@@ -42,6 +42,8 @@
 
 import org.jruby.anno.JRubyClass;
 import org.jruby.anno.JRubyMethod;
+import org.jruby.exceptions.JumpException.FlowControlException;
+import org.jruby.java.proxies.ConcreteJavaProxy;
 import org.jruby.runtime.Block;
 import org.jruby.runtime.ClassIndex;
 import org.jruby.runtime.ObjectAllocator;
@@ -278,6 +280,20 @@ public IRubyObject inspect(ThreadContext context) {
         return getRuntime().newString(sb.toString());
     }
 
+    @JRubyMethod(name = "===", meta = true)
+    public static IRubyObject op_eqq(ThreadContext context, IRubyObject recv, IRubyObject other) {
+        Ruby runtime = context.getRuntime();
+        // special case non-FlowControlException Java exceptions so they'll be caught by rescue Exception
+        if (recv == runtime.getException() && other instanceof ConcreteJavaProxy) {
+            Object object = ((ConcreteJavaProxy)other).getObject();
+            if (object instanceof Throwable && !(object instanceof FlowControlException)) {
+                return context.getRuntime().getTrue();
+            }
+        }
+        // fall back on default logic
+        return ((RubyClass)recv).op_eqq(context, other);
+    }
+
     @Override
     public void copySpecialInstanceVariables(IRubyObject clone) {
         RubyException exception = (RubyException)clone;

diff --git a/src/org/jruby/ast/RescueNode.java b/src/org/jruby/ast/RescueNode.java
@@ -41,6 +41,7 @@
 import org.jruby.evaluator.ASTInterpreter;
 import org.jruby.exceptions.JumpException;
 import org.jruby.exceptions.RaiseException;
+import org.jruby.exceptions.Unrescuable;
 import org.jruby.javasupport.JavaUtil;
 import org.jruby.javasupport.util.RuntimeHelpers;
 import org.jruby.lexer.yacc.ISourcePosition;
@@ -106,37 +107,7 @@ public List<Node> childNodes() {
 
     @Override
     public IRubyObject interpret(Ruby runtime, ThreadContext context, IRubyObject self, Block aBlock) {
-        if (runtime.getJavaSupport().isActive() && UnsafeFactory.getUnsafe() != null) {
+        return interpretWithJavaExceptions(runtime, context, self, aBlock);
-            return interpretWithJavaExceptions(runtime, context, self, aBlock);
-        } else {
-            return interpretWithoutJavaExceptions(runtime, context, self, aBlock);
-        }
-    }
-
-    private IRubyObject interpretWithoutJavaExceptions(Ruby runtime, ThreadContext context, IRubyObject self, Block aBlock) {
-        RescuedBlock : while (true) {
-            IRubyObject globalExceptionState = runtime.getGlobalVariables().get("$!");
-            boolean anotherExceptionRaised = false;
-            try {
-                return executeBody(runtime, context, self, aBlock);
-            } catch (RaiseException raiseJump) {
-                try {
-                    return handleException(runtime, context, self, aBlock, raiseJump);
-                } catch (JumpException.RetryJump rj) {
-                    // let RescuedBlock continue
-                } catch (RaiseException je) {
-                    anotherExceptionRaised = true;
-                    throw je;
-                }
-            } catch (JumpException.FlowControlException flow) {
-                // just rethrow
-                throw flow;
-            } finally {
-                // clear exception when handled or retried
-                if (!anotherExceptionRaised)
-                    runtime.getGlobalVariables().set("$!", globalExceptionState);
-            }
-        }
     }
 
     private IRubyObject interpretWithJavaExceptions(Ruby runtime, ThreadContext context, IRubyObject self, Block aBlock) {
@@ -158,6 +129,9 @@ private IRubyObject interpretWithJavaExceptions(Ruby runtime, ThreadContext cont
                 // just rethrow
                 throw flow;
             } catch (Throwable t) {
+                if (t instanceof Unrescuable) {
+                    UnsafeFactory.getUnsafe().throwException(t);
+                }
                 try {
                     return handleJavaException(runtime, context, self, aBlock, t);
                 } catch (JumpException.RetryJump rj) {

diff --git a/src/org/jruby/exceptions/JumpException.java b/src/org/jruby/exceptions/JumpException.java
@@ -42,7 +42,7 @@
 public class JumpException extends RuntimeException {
     private static final long serialVersionUID = -228162532535826617L;
 
-    public static class FlowControlException extends JumpException {
+    public static class FlowControlException extends JumpException implements Unrescuable {
         protected int target;
         protected Object value;
 

diff --git a/src/org/jruby/exceptions/MainExitException.java b/src/org/jruby/exceptions/MainExitException.java
@@ -26,7 +26,7 @@
  ***** END LICENSE BLOCK *****/
 package org.jruby.exceptions;
 
-public class MainExitException extends RuntimeException {
+public class MainExitException extends RuntimeException implements Unrescuable {
     private static final long serialVersionUID = -8585821821150293755L;
     boolean usageError;
     int status;

diff --git a/src/org/jruby/exceptions/ThreadKill.java b/src/org/jruby/exceptions/ThreadKill.java
@@ -27,6 +27,6 @@
  ***** END LICENSE BLOCK *****/
 package org.jruby.exceptions;
 
-public class ThreadKill extends RuntimeException {
+public class ThreadKill extends RuntimeException implements Unrescuable {
 	private static final long serialVersionUID = -6885888060743175327L;
 }
diff --git a/src/org/jruby/exceptions/Unrescuable.java b/src/org/jruby/exceptions/Unrescuable.java
@@ -0,0 +1,38 @@
+/***** BEGIN LICENSE BLOCK *****
+ * Version: CPL 1.0/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Common Public
+ * License Version 1.0 (the "License"); you may not use this file
+ * except in compliance with the License. You may obtain a copy of
+ * the License at http://www.eclipse.org/legal/cpl-v10.html
+ *
+ * Software distributed under the License is distributed on an "AS
+ * IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * rights and limitations under the License.
+ *
+ * Copyright (C) 2010 Charles O Nutter <headius@headius.com>
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either of the GNU General Public License Version 2 or later (the "GPL"),
+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the CPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the CPL, the GPL or the LGPL.
+ ***** END LICENSE BLOCK *****/
+package org.jruby.exceptions;
+
+/**
+ * This marker interface is for JRuby internal exceptions that can't be caught
+ * by Ruby exception handling.
+ * 
+ * @author headius
+ */
+public interface Unrescuable {
+
+}
diff --git a/src/org/jruby/javasupport/util/RuntimeHelpers.java b/src/org/jruby/javasupport/util/RuntimeHelpers.java
@@ -26,6 +26,7 @@
 import org.jruby.evaluator.ASTInterpreter;
 import org.jruby.exceptions.JumpException;
 import org.jruby.exceptions.RaiseException;
+import org.jruby.exceptions.Unrescuable;
 import org.jruby.internal.runtime.methods.CallConfiguration;
 import org.jruby.internal.runtime.methods.DynamicMethod;
 import org.jruby.internal.runtime.methods.UndefinedMethod;
@@ -59,6 +60,7 @@
 import org.jruby.runtime.builtin.IRubyObject;
 import org.jruby.util.ByteList;
 import org.jruby.util.TypeConverter;
+import org.jruby.util.unsafe.UnsafeFactory;
 
 /**
  * Helper methods which are called by the compiler.  Note: These will show no consumers, but
@@ -877,6 +879,10 @@ public static IRubyObject isExceptionHandled(RubyException currentException, IRu
     }
 
     public static IRubyObject isExceptionHandled(RubyException currentException, IRubyObject exception, ThreadContext context) {
+        return isExceptionHandled((IRubyObject)currentException, exception, context);
+    }
+
+    public static IRubyObject isExceptionHandled(IRubyObject currentException, IRubyObject exception, ThreadContext context) {
         Ruby runtime = context.getRuntime();
         if (!runtime.getModule().isInstance(exception)) {
             throw runtime.newTypeError("class or module required for rescue clause");
@@ -898,13 +904,20 @@ public static IRubyObject isExceptionHandled(RubyException currentException, IRu
         return isExceptionHandled(currentException, exception1, exception2, context);
     }
 
-    private static boolean checkJavaException(Throwable currentThrowable, IRubyObject throwable) {
+    private static boolean checkJavaException(Throwable throwable, IRubyObject catchable, ThreadContext context) {
-        if (throwable instanceof RubyClass) {
+        if (context.getRuntime().getException().op_ge(catchable).isTrue()) {
-            RubyClass rubyClass = (RubyClass)throwable;
+            if (throwable instanceof RaiseException) {
+                return isExceptionHandled(((RaiseException)throwable).getException(), catchable, context).isTrue();
+            }
+            // let Ruby exceptions decide if they handle it
+            return isExceptionHandled(JavaUtil.convertJavaToUsableRubyObject(context.getRuntime(), throwable), catchable, context).isTrue();
+        }
+        if (catchable instanceof RubyClass && catchable.getInstanceVariables().hasInstanceVariable("@java_class")) {
+            RubyClass rubyClass = (RubyClass)catchable;
             JavaClass javaClass = (JavaClass)rubyClass.fastGetInstanceVariable("@java_class");
             if (javaClass != null) {
                 Class cls = javaClass.javaClass();
-                if (cls.isInstance(currentThrowable)) {
+                if (cls.isInstance(throwable)) {
                     return true;
                 }
             }
@@ -913,11 +926,15 @@ private static boolean checkJavaException(Throwable currentThrowable, IRubyObjec
     }
 
     public static IRubyObject isJavaExceptionHandled(Throwable currentThrowable, IRubyObject[] throwables, ThreadContext context) {
+        if (currentThrowable instanceof Unrescuable) {
+            UnsafeFactory.getUnsafe().throwException(currentThrowable);
+        }
+
         if (currentThrowable instanceof RaiseException) {
             return isExceptionHandled(((RaiseException)currentThrowable).getException(), throwables, context);
         } else {
             for (int i = 0; i < throwables.length; i++) {
-                if (checkJavaException(currentThrowable, throwables[0])) {
+                if (checkJavaException(currentThrowable, throwables[0], context)) {
                     return context.getRuntime().getTrue();
                 }
             }
@@ -927,10 +944,14 @@ public static IRubyObject isJavaExceptionHandled(Throwable currentThrowable, IRu
     }
 
     public static IRubyObject isJavaExceptionHandled(Throwable currentThrowable, IRubyObject throwable, ThreadContext context) {
+        if (currentThrowable instanceof Unrescuable) {
+            UnsafeFactory.getUnsafe().throwException(currentThrowable);
+        }
+
         if (currentThrowable instanceof RaiseException) {
             return isExceptionHandled(((RaiseException)currentThrowable).getException(), throwable, context);
         } else {
-            if (checkJavaException(currentThrowable, throwable)) {
+            if (checkJavaException(currentThrowable, throwable, context)) {
                 return context.getRuntime().getTrue();
             }
 
@@ -939,13 +960,17 @@ public static IRubyObject isJavaExceptionHandled(Throwable currentThrowable, IRu
     }
 
     public static IRubyObject isJavaExceptionHandled(Throwable currentThrowable, IRubyObject throwable0, IRubyObject throwable1, ThreadContext context) {
+        if (currentThrowable instanceof Unrescuable) {
+            UnsafeFactory.getUnsafe().throwException(currentThrowable);
+        }
+
         if (currentThrowable instanceof RaiseException) {
             return isExceptionHandled(((RaiseException)currentThrowable).getException(), throwable0, throwable1, context);
         } else {
-            if (checkJavaException(currentThrowable, throwable0)) {
+            if (checkJavaException(currentThrowable, throwable0, context)) {
                 return context.getRuntime().getTrue();
             }
-            if (checkJavaException(currentThrowable, throwable1)) {
+            if (checkJavaException(currentThrowable, throwable1, context)) {
                 return context.getRuntime().getTrue();
             }
 
@@ -954,16 +979,20 @@ public static IRubyObject isJavaExceptionHandled(Throwable currentThrowable, IRu
     }
 
     public static IRubyObject isJavaExceptionHandled(Throwable currentThrowable, IRubyObject throwable0, IRubyObject throwable1, IRubyObject throwable2, ThreadContext context) {
+        if (currentThrowable instanceof Unrescuable) {
+            UnsafeFactory.getUnsafe().throwException(currentThrowable);
+        }
+
         if (currentThrowable instanceof RaiseException) {
             return isExceptionHandled(((RaiseException)currentThrowable).getException(), throwable0, throwable1, throwable2, context);
         } else {
-            if (checkJavaException(currentThrowable, throwable0)) {
+            if (checkJavaException(currentThrowable, throwable0, context)) {
                 return context.getRuntime().getTrue();
             }
-            if (checkJavaException(currentThrowable, throwable1)) {
+            if (checkJavaException(currentThrowable, throwable1, context)) {
                 return context.getRuntime().getTrue();
             }
-            if (checkJavaException(currentThrowable, throwable2)) {
+            if (checkJavaException(currentThrowable, throwable2, context)) {
                 return context.getRuntime().getTrue();
             }