New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

if a PyException wraps a Java Exception object, then the PyException's getCause() method should return that Java Exception object #54

Open
jason-s opened this Issue Feb 1, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@jason-s

jason-s commented Feb 1, 2017

If I do this in Jython: (tried with both 2.5.3 and 2.7.0 standalone)

import java.lang.Exception
for k in xrange(10):
   print k
if k > 3: raise java.lang.Exception("Hey")

it works fine; Jython raises a wrapped instance of java.lang.Exception. It appears to be an org.python.core.PyException. Unfortunately this class uses the default constructor of its superclass.

Why does this matter? If the object raised within Jython is a Jython object, rather than a Java exception class, it doesn't matter. But if the object raised within Jython is some Java exception object E, and Jython is embedded within a larger Java program P, then Jython doesn't allow the larger program P to use the standard Throwable.getCause() method to access the Java exception object E: it's been wrapped up opaquely, and unless the program P has special logic that knows about org.python.core.PyException and can somehow access E, it can't benefit from the information contained in E.

I would suggest changing the main constructor of PyException from this

public PyException(PyObject type, PyObject value, PyTraceback traceback) {
        this.type = type;
        this.value = value;
        ...

to this:

public PyException(PyObject type, PyObject value, PyTraceback traceback) {
        super(getJavaExceptionCauseIfPresent(value))
        this.type = type;
        this.value = value;
        ...

where getJavaExceptionCauseIfPresent(value) (or a suitably-named alternative) either returns the wrapped Java object that is inside the PyObject value, if there is one, or null otherwise. This would allow Java programs that embed Jython to access the getCause() method without having to know that the exception thrown by Jython is, in fact, a PyException.

Need to handle these test cases:

    import java.lang.RuntimeException
    def testCase(n, message):
       if n == 0:
         raise java.lang.RuntimeException(message)
       else:
         class MyException(java.lang.RuntimeException):
           pass
         raise MyException(message)
@Stewori

This comment has been minimized.

Show comment
Hide comment
@Stewori

Stewori Feb 2, 2017

Member

Changing the PyException constructor would break Py.JavaError(), which calls initCause on the PyException. According to javadoc of Throwable, initCause would throw an IllegalStateException, if the cause was already given in the constructor. We could modify Py.JavaError(), but I'd suggest to apply your requested modification at the end of PyException.doRaise() as follows:

...

PyException res = new PyException(type, value, (PyTraceback)traceback);
if (value.getJavaProxy() != null) {
    Object javaError = value.__tojava__(Throwable.class);
    if (javaError != Py.NoConversion) {
       try {
    	    res.initCause((Throwable) javaError);
        } catch (Exception e) {}
    }
}
return res;

I already checked that this would pass regrtests (at least doesn't make them worse). It would be ideal if you could assert that this change would solve the problem you describe. E.g. note that from Python perspective you wouldn't notice:

from java.lang import RuntimeException

def test_exc():
    raise RuntimeException("Hey")

try:
    test_exc()
except RuntimeException as err:
    print err # is the RuntimeException itself
    print err.getCause() #still prints None, because already unwrapped

So here it's the same behavior as before the change.

Member

Stewori commented Feb 2, 2017

Changing the PyException constructor would break Py.JavaError(), which calls initCause on the PyException. According to javadoc of Throwable, initCause would throw an IllegalStateException, if the cause was already given in the constructor. We could modify Py.JavaError(), but I'd suggest to apply your requested modification at the end of PyException.doRaise() as follows:

...

PyException res = new PyException(type, value, (PyTraceback)traceback);
if (value.getJavaProxy() != null) {
    Object javaError = value.__tojava__(Throwable.class);
    if (javaError != Py.NoConversion) {
       try {
    	    res.initCause((Throwable) javaError);
        } catch (Exception e) {}
    }
}
return res;

I already checked that this would pass regrtests (at least doesn't make them worse). It would be ideal if you could assert that this change would solve the problem you describe. E.g. note that from Python perspective you wouldn't notice:

from java.lang import RuntimeException

def test_exc():
    raise RuntimeException("Hey")

try:
    test_exc()
except RuntimeException as err:
    print err # is the RuntimeException itself
    print err.getCause() #still prints None, because already unwrapped

So here it's the same behavior as before the change.

@Stewori

This comment has been minimized.

Show comment
Hide comment
@Stewori

Stewori Feb 2, 2017

Member

Also: Would be good if you could file an issue at bugs.jython.org so we can better keep track. (As change-notes in NEWS file etc usually refer to numbering style from there)

Member

Stewori commented Feb 2, 2017

Also: Would be good if you could file an issue at bugs.jython.org so we can better keep track. (As change-notes in NEWS file etc usually refer to numbering style from there)

@jason-s

This comment has been minimized.

Show comment
Hide comment
@jason-s

jason-s Feb 2, 2017

Would be good if you could file an issue at bugs.jython.org so we can better keep track.

Will do! Thanks for the quick response. If I were a bit more adept at understanding how this all works, I would file a PR myself, but it sounds like there's some real subtleties here.

edit: done -- http://bugs.jython.org/issue2547

jason-s commented Feb 2, 2017

Would be good if you could file an issue at bugs.jython.org so we can better keep track.

Will do! Thanks for the quick response. If I were a bit more adept at understanding how this all works, I would file a PR myself, but it sounds like there's some real subtleties here.

edit: done -- http://bugs.jython.org/issue2547

@jason-s

This comment has been minimized.

Show comment
Hide comment
@jason-s

jason-s Feb 2, 2017

It would be ideal if you could assert that this change would solve the problem you describe.

initCause() should work too. I don't really understand the subtleties of some of the things that raise does in Jython, but the goal of this issue is really to make sure that:

  • if within Java an exception ex1 is caught of type PyException
  • ex1 was constructed in Jython to wrap a Java exception object ex0
  • and within Java you call ex1.getCause(), you should get back ex0.

jason-s commented Feb 2, 2017

It would be ideal if you could assert that this change would solve the problem you describe.

initCause() should work too. I don't really understand the subtleties of some of the things that raise does in Jython, but the goal of this issue is really to make sure that:

  • if within Java an exception ex1 is caught of type PyException
  • ex1 was constructed in Jython to wrap a Java exception object ex0
  • and within Java you call ex1.getCause(), you should get back ex0.

@jason-s jason-s changed the title from PyException only uses default constructor; doesn't initialize the cause to if a PyException wraps a Java Exception object, then the `PyException`'s `getCause()` method should return that Java Exception object Feb 2, 2017

@jason-s jason-s changed the title from if a PyException wraps a Java Exception object, then the `PyException`'s `getCause()` method should return that Java Exception object to if a PyException wraps a Java Exception object, then the PyException's getCause() method should return that Java Exception object Feb 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment