Attach PyObject to exceptions, fix memory leak when constructing Java StackTraceElements

[magicmq]

Changes

The below changes are non-breaking.

Minor changes to exception handling

This PR changes exception handling within an interpreter context, for the following scenarios:

• If a pure Python exception is raised from Python (such as ValueError, SyntaxError, ArithmeticError, etc.), a PythonException object is instantiated and thrown (as long as the exception isn't caught in Python). The underlying Python exception object is attached to the exception (as a PyObject) and is obtainable via PythonException.getPythonException(). For example:

JepConfig config = new JepConfig().addIncludePaths(".");
try (Interpreter interp = new SubInterpreter(config)) {
    try {
        interp.eval("raise ValueError('Test traceback')");
    } catch (PythonException e) {
        PyObject pyExc = e.getPythonException();
        // Do something with the pyExc...
    }
}

• If a Java exception is surfaced as a result of a Python call, a JepException object is instantiated and thrown. For example:

JepConfig config = new JepConfig().addIncludePaths(".");
try (Interpreter interp = new SubInterpreter(config)) {
    interp.eval("from java.util import ArrayList");
    try {
        interp.eval("ArrayList().get(0)");
    } catch (PythonException e) {
        // This will NOT be caught
    } catch (JepException e) {
        // This WILL be caught
        Throwable cause = e.getCause();
        // cause will be an instance of IndexOutOfBoundsException
    }
}

The following changes were also made:

• A getPythonTraceback() method was added to JepException: a traceback may be available for both of the above scenarios, while a Python exception object will only be available for PythonException.
• A process_python_traceback function was added to jep_exceptions.c to provide the formatted Python traceback String and the frames for subsequent processing into Java StackTraceElements.
• The TestExceptionCause test was merged into a new test, TestJepException, which performs more granular testing on JepException (and PythonException) in a variety of possible scenarios.

Commit 4e547e9 makes the above changes/additions.

Quick Aside

I think the above changes could potentially create confusion, particularly with the second scenario: users may expect all exceptions that "came from" Python code to be a PythonException, even if the exception originated in Java. I can't think of a better way to solve this without making breaking changes to the existing code, but nevertheless, I attempted to make this as clear as possible in the JavaDocs.

Although this PR solves the issues outlined in #319 and #633, I think it would make a lot more organizational sense to do the following:

• Reserve JepException for JEP-related exceptions that occur outside of the scope of an interpreter (for example, creating a JepConfig after creating a SharedInterpreter, or throwing to signal invalid thread access).
• Create a new class called InterpreterException, which becomes a new superclass for any exceptions thrown inside the interpreter context (either pure Python exceptions or Java exceptions which surface as a result of Python calls). I think it would be useful to attach a formatted Python traceback String to this superclass.
• Make PythonException a subclass of InterpreterException, and throw this subtype for pure Python exceptions only. It would cache the underlying Python exception as a PyObject.
• Create JavaException, a subclass of InterpreterException, and throw this subtype for Java exceptions that occur as a result of a Python call. The underlying Java exception would be obtainable via getCause(), as before. Although the cause was a Java exception, it may still be useful to have a Python traceback, so that's obtainable via the superclass' InterpreterException.getPythonTraceback() method.

I didn't do this because it would introduce breaking changes, since JepException currently encompasses exceptions thrown in a Python context.

Patch for memory leak when handling exceptions

This PR also fixes a memory leak in jep_exceptions.c. When looping over the Python traceback to build Java StackTraceElements, PySequence_GetItem creates a new reference, but these new references are immediately passed to other functions, without decrefing:

stackEntry = PyList_GetItem(pystack, i);
// java order is classname, method name, filename, line number
// python order is filename, line number, function name, line
charPyFile = PyUnicode_AsUTF8(
                 PySequence_GetItem(stackEntry, 0));
pyLineNum = (int) PyLong_AsLongLong(
                PySequence_GetItem(stackEntry, 1));
charPyFunc = PyUnicode_AsUTF8(
                 PySequence_GetItem(stackEntry, 2));
pyLine = PySequence_GetItem(stackEntry, 3);

In addition, pyLine is never decrefed. This leaks up to 4 Python objects per stack frame.

This is fixed by saving all four references as named variables, and explicitly calling decref on them when apporpriate.

Commit 6ea9217 fixes this memory leak.

[bsteffensmeier]

I think the above changes could potentially create confusion, particularly with the second scenario: users may expect all exceptions that "came from" Python code to be a PythonException, even if the exception originated in Java. I can't think of a better way to solve this without making breaking changes to the existing code, but nevertheless, I attempted to make this as clear as possible in the JavaDocs.

I have started looking at this and overall it looks good so far. I will need more time to fully review it but for now I keep coming back to this point. Your documentation and explanation here make me feel like I could comfortably use the API correctly but I don't understand why we need to structure it that way. Is there a technical or compatibility reason why a Python Exception that was originally a Java Exception can't be a PythonException instead of a JepException? I think we have a python exception object and it might not be as interesting as the original java cause but it should still technically be enough to satisfy the requirements to make a PythonException.

Patch for memory leak when handling exceptions

Awesome!

[magicmq]

Is there a technical or compatibility reason why a Python Exception that was originally a Java Exception can't be a PythonException instead of a JepException? I think we have a python exception object and it might not be as interesting as the original java cause but it should still technically be enough to satisfy the requirements to make a PythonException.

I didn't necessarily make it that way because of a technical or compatibility issue, but a Python exception that was originally a Java exception wouldn't have a Python exception object, would it? I know JEP converts a few select Java exceptions into equivalent Python exceptions (and in those cases, Python exception objects would be available), but my understanding was that this was done solely to facilitate catching those Java exceptions in Python, not necessarily because the Python exception object adds any value or additional information to the exception itself.

I suppose that a PythonException could be thrown as well for Java exceptions that surface from Python, but that seemed even more misleading than the current design, since the exception wasn't really a Python exception at all, it was a Java exception that only happened to surface as a result of a Python call.

[bsteffensmeier]

I didn't necessarily make it that way because of a technical or compatibility issue, but a Python exception that was originally a Java exception wouldn't have a Python exception object, would it?

In process_py_exception() there is always a python exception object, the only time that wasn't the case is when we weren't calling PyErr_NormalizeException(), but with that addition we will always have a Python exception, whether the original cause was Python related or from Java.

In fact, I just realized that with this new code, jpyException will always be a Java PyObject wrapping that python exception, even when the exception originated in java. For the case where you currently create a JepException we create jpyException without ever using it.

I know JEP converts a few select Java exceptions into equivalent Python exceptions (and in those cases, Python exception objects would be available), but my understanding was that this was done solely to facilitate catching those Java exceptions in Python, not necessarily because the Python exception object adds any value or additional information to the exception itself.

Even in the case where there is no equivalent Python exception we create a Python RuntimeError so the lack of an equivalent exception type doesn't prevent the creation of a Python exception. I agree that the original intent was to make it easier to catch Java Exceptions in Python and there isn't much value in the Python Exception. But it seems to me this feature proves there is some value in the Python exception. The Python traceback is an attribute of a Python exception, you want access to this traceback even when an exception originated in Java so in this use case there is some value in the Python exception even if it originated from a Java exception.

You find the formatted traceback string to be useful but I can imagine there might be use cases where the traceback objects themselves might be useful for similar reasons. I see including the whole PyObject in the PythonException as a convenient way to just include all the context we have around the exception from Python in case anyone catching it wants to use it. I don't see any reason to ever include the formatted traceback and not give access to the whole exception.

I suppose that a PythonException could be thrown as well for Java exceptions that surface from Python, but that seemed even more misleading than the current design, since the exception wasn't really a Python exception at all, it was a Java exception that only happened to surface as a result of a Python call.

This seems less confusing to me. If there was ever a Python exception then I think Jep should throw PythonException, even in the case where that PythonException is just triggered by a Java exception. I think this is similar to when a Java Exception has a long chain of CausedBy, often this is confusing and some of those layers tend to be meaningless but trying to cut out layers of causes from java exception is only going to make it more confusing.