new PythonThrowable java class to address deficiency in fix for issue #294

[scottslewis]

A new class PythonThrowable to take the String output from Python traceback.format_exc(), parse it, and represent the python Exception message and stack trace as a PythonThrowable instance.

The constructor for PythonThrowable will parse a string that is the output of Python call to traceback.format_exc. It will then produce either a 'python style' stack output (via PythonThrowable.printStackTracePython) or 'java style' output (via PythonThrowable.printStackTraceJava). By default the Throwable.printStackTrace() call will use Python format, but can default to java via the PythonThrowable(String,boolean) flag (2nd argument).

Also includes java test code (PythonThrowableTest class) and small addition to Py4JException class (additional constructor) to allow the easy usage of PythonThrowable.

[scottslewis]

The circleci build seems to have failed because of some findbugs problem, but when I run gradlew findbugsMain locally it completes successfully. I can't tell by the output what is going wrong with the circleci run.

[scottslewis]

Again the build apparently failed in findbugs...here is the output from the page at 'Details' above

Building 30% > :findbugsMainDownload http://repo1.maven.org/maven2/org/ow2/asm/asm-tree/5.0.2/asm-tree-5.0.2.jar Building 30% > :findbugsMain > 4 KB/28 KB downloaded7 KB/28 KB downloaded11 KB/28 KB downloaded5 KB/28 KB downloaded9 KB/28 KB downloaded23 KB/28 KB downloaded7 KB/28 KB downloaded8 KB/28 KB downloaded Building 30% > :findbugsMainDownload http://repo1.maven.org/maven2/org/ow2/asm/asm/5.0.2/asm-5.0.2.jar Building 30% > :findbugsMain > 4 KB/51 KB downloaded7 KB/51 KB downloaded11 KB/51 KB downloaded5 KB/51 KB downloaded9 KB/51 KB downloaded23 KB/51 KB downloaded7 KB/51 KB downloaded31 KB/51 KB downloaded5 KB/51 KB downloaded9 KB/51 KB downloaded43 KB/51 KB downloaded7 KB/51 KB downloaded51 KB/51 KB downloaded Building 30% > :findbugsMain > Resolving dependencies ':findbugsPlugins':findbugsMain Building 30%FAILED Building 30% Building 30%40% Building 40%FAILURE: Building 40%Build failed with an exception. Building 40% Building 40% Building 40%* What went wrong: Building 40%Execution failed for task ':findbugsMain'. Building 40%> Building 40%FindBugs rule violations were found. See the report at: file:///home/ubuntu/py4j/py4j-java/build/reports/findbugs/main.xml Building 40% Building 40%* Try: Building 40%Run with Building 40%--stacktrace Building 40% option to get the stack trace. Run with Building 40%--info Building 40% or Building 40%--debug Building 40% option to get more log output.

I can't reproduce this locally...if I run gradlew findbugsMain it finishes successfully, so don't know why it's failing.

[bartdag]

Hi Scott, the FindBugs report is here:

https://417-1045976-gh.circle-artifacts.com/1/home/ubuntu/py4j/py4j-java/build/reports/findbugs/main.xml

I'll try to look at the pull request over the weekend. Thanks!

[codecov-io]

Codecov Report

Merging #303 into master will increase coverage by 0.45%. The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #303      +/-   ##
============================================
+ Coverage     81.49%   81.95%   +0.45%     
- Complexity      704      725      +21     
============================================
  Files            84       83       -1     
  Lines          8625     8571      -54     
  Branches        383      398      +15     
============================================
- Hits           7029     7024       -5     
+ Misses         1456     1399      -57     
- Partials        140      148       +8

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/py4j/Py4JException.java	60% <0%> (-15%)	3% <0%> (ø)
src/main/java/py4j/GatewayConnection.java	70.83% <0%> (-12.21%)	13% <0%> (-7%)
src/main/java/py4j/NetworkUtil.java	40.62% <0%> (-5.53%)	6% <0%> (-1%)
src/main/java/py4j/Protocol.java	72.66% <0%> (-0.76%)	73% <0%> (-1%)
src/main/java/py4j/StringUtil.java	93.75% <0%> (-0.7%)	7% <0%> (-1%)
src/py4j/tests/client_server_test.py	94.48% <0%> (-0.16%)	0% <0%> (ø)
src/main/java/py4j/JavaServer.java	33.33% <0%> (ø)	1% <0%> (ø)	:arrow_down:
src/main/java/py4j/ClientServerConnection.java	0% <0%> (ø)	0% <0%> (ø)	:arrow_down:
...rc/main/java/py4j/Py4JAuthenticationException.java
src/main/java/py4j/commands/AuthCommand.java
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update eb3d4f2...e2fd805. Read the comment docs.

[scottslewis]

Hi Barthelemy. I've fixed the findbugs issues and the formatting things...although the py4j formatter messed up the example Python stack traces in the PythonThrowable class java docs.

The code coverage going down apparently has to do with not testing the getters in PythonThrowable and PythonStackTraceElement. These are both immutable classes and the getters are trivial, so I don't think testing the getters directly are actually needed.

[scottslewis]

One more thing: I did not change the code in Protocol.getReturnValue() that combines the python output from traceback.format_exc with a String message to create and throw a Py4JException. What I would suggest is that this be changed to something like: new Py4JException(new PythonThrowable(pythonFormatExc)); or perhaps: new Py4JException("Java side message",new PythonThrowable(pythonFormatExc));

[bartdag]

@scottslewis apologies for the long delay. I looked at the code and before I dive into specific questions or code review comments I have some high-level questions:

1. If I understand correctly, we would pass the exception string received from the Python side and wrap it into a PythonThrowable. Instead of raising a Py4JException, we would raise a PythonThrowable. Is that correct? Are there other usage scenarios?

2. The main advantage over Py4JException is that StackTraceElement will play nicer with most logging infrastructure and IDE? Are there other advantages?

3. Why is PythonThrowable a subclass of Throwable and not of Py4JException?

4. How robust is the parsing of Python stack trace string? For example, Python 3 finally implemented chained exception (https://www.python.org/dev/peps/pep-3134/): is it taken into account? Do you think that sending a structured variable (i.e., stack trace elements already separated using Py4J protocol on the Python side) would be more robust than trying to perform the parsing on the Java side?

5. Do we need an equivalent implementation for the Python side (e.g., splitting Java exception stack trace into stack trace entries)?

[scottslewis]

Hi Barthelemy,

If I understand correctly, we would pass the exception string received from the Python side and wrap it >into a PythonThrowable. Instead of raising a Py4JException, we would raise a PythonThrowable. Is that >correct? Are there other usage scenarios?

Although one could raise a PythonThrowable directly, my suggestion would be to still raise a Py4jException and provide a PythonThrowble as a cause of the Py4jException, so as not to break existing clients.

The main advantage over Py4JException is that StackTraceElement will play nicer with most logging >infrastructure and IDE?

I would say it's more than 'play nicer'...it's more like it would 'play at all' with the large number of java logging and tooling in the java world...making integration much easier and more likely.

Are there other advantages?

It's cleaner and more extensible by design. It also makes it quite easy and natural to inherit from PythonThrowable....for creating java-side exceptions that correspond to Python-side exceptions...if desired...for Py4j directly...or py4j consumers (like me wrt OSGi remote services).

Why is PythonThrowable a subclass of Throwable and not of Py4JException?

It's meant to represent python-side thrown exceptions, not py4j exceptions. With java's nested/caused-by structure (as opposed to only inheritance) this seems simpler to me. And there's nothing in Py4jException that adds to PythonThrowable. And I do think that nesting PythonThrowable as the cause of Py4jException makes more sense than throwing Py4jException xor PythonThrowable.

How robust is the parsing of Python stack trace string? For example, Python 3 finally implemented chained exception (https://www.python.org/dev/peps/pep-3134/): is it taken into account? Do you think that sending a structured variable (i.e., stack trace elements already separated using Py4J protocol on the Python side) would be more robust than trying to perform the parsing on the Java side?

WRT the parsing of the Python stack trace string: I haven't been personally involved in the pepping, but by usage and observation it seems quite stable to me...i.e. 2 and 3 seem to have the same output. I haven't looked at chained exception pep but will do so. I would have no problem with sending a structured variable, but that would mean enhancing the py4j protocol. If you are willing to do this it's fine with me.

I don't think it would be very possible (for backward compatibility reasons) for Python to change the stack trace string significantly at this point, but if they did so IMHO it's likely to be a Python 4 or Python 5 level change and that's likely going to require a new release of Py4j as well.

So although separating on Python side would potentially be more robust, I don't suspect it's going to be an issue in the short term. FWIW one thing that could be done is to wrap the PythonThrowable constructor in a try/catch block, and if it fails for any reason then simply include the String unparsed in Py4jException to be thrown as is done now. This would provide some insurance that the communication wouldn't be broken by having the parsing fail. My suggestion is that this be done.

If PythonThrowable is introduced now, it could be changed in the future (new constructor) to take a String[] or some such when the structured stack trace is introduced...or if the parsing fails due to some other change in the stack trace String format.

Do we need an equivalent implementation for the Python side (e.g., splitting Java exception stack trace into stack trace entries)?

I don't have strong feelings about this. I could see it as an advantage to do so, but the Python world is far less mature about exception handling in API design, and tooling for same, and nesting exceptions... so it doesn't seem like a driving need to me. I would be more inclined to see if it's needed/desired by consumers before doing the necessary work.

[scottslewis]

Hi Barthelemy. Added support for Python chained exceptions. to PythonThrowable, and added test case. I am going to add another chained exception test case but it's going to take a little doing to generate the python test case so it may be a day or so before it appears.

[bartdag]

Hi Scott, thank you very much for your detailed answers. I think I really like using PythonThrowable as a cause of Py4jException. If you could post an example of what the whole stack trace would look like (Java user code calls Python, Python raises an exception, Java prints the stack trace and we see the chained exception Py4JException -> PythonThrowable), I would invite two other contributors to comment, but I am mostly convinced now this is a good addition. The example stack trace may also end up in the documentation considering that this is a common question.

[scottslewis]

Hi Barthelemy.

I will produce additional tests and examples as you describe, but it's going to take a couple of days due to other commitments. Please lmk if that holds things up.

One thought: If PythonThrowable is created and set as a cause of Py4jException then there should probably be a new option on the JavaGateway instance...that determines whether the PythonThrowable stack trace prints out as 'python style'...i.e. top exception last or 'java style'...top exception first. The current default...i.e. PythonThrowable(String) is 'python style' but it would probably be good to allow 'java style' by JavaGateway configuration (and then using PythonThrowable(String,boolean) constructor at the appropriate code location.

Anyway, I'll create example output both ways .

[scottslewis]

BTW...I did try to put an example in the javadocs for PythonThrowable and one of the ci checks (findbugs?) complained about it, so I removed it from javadocs. We can put in other docs, though.

[scottslewis]

Hi Barthelemy. I've just committed some more improvements...i.e. added test code for chained exceptions, made the PythonThrowable string parsing more robust (with new/additional tests) and added toString() implementation that allows .printStackTrace() to produce output like this:

org.osgi.framework.ServiceException: Service exception on remote service proxy rsid=org.eclipse.ecf.remoteservice.RemoteServiceID[containerID=URIID [uri=py4j://127.0.0.1:25334/python];containerRelativeID=4]
	at org.eclipse.ecf.remoteservice.client.AbstractRSAClientService.invoke(AbstractRSAClientService.java:102)
	at com.sun.proxy.$Proxy6.sayHello(Unknown Source)
	at org.eclipse.ecf.examples.protobuf.hello.consumer.HelloConsumer$1.run(HelloConsumer.java:42)
	at java.lang.Thread.run(Unknown Source)
Caused by: org.eclipse.ecf.core.util.ECFException: Could not execute remote call=RemoteCall[method=org.eclipse.ecf.examples.protobuf.hello.IHello.sayHello, parameters=[h: "some other message"
f: "java"
to: "python"
hellomsg: "Hello from java"
x: 1.1
x: 1.2
], timeout=30000]
	at org.eclipse.ecf.provider.direct.protobuf.ProtobufClientContainer$ProtoBufDirectRemoteService.invokeSync(ProtobufClientContainer.java:146)
	at org.eclipse.ecf.remoteservice.client.AbstractRSAClientService.invoke(AbstractRSAClientService.java:97)
	... 3 more
Caused by: py4j.Py4JException: An exception was raised by the Python Proxy
	at py4j.Protocol.getReturnValue(Protocol.java:459)
	at py4j.reflection.PythonProxyHandler.invoke(PythonProxyHandler.java:108)
	at com.sun.proxy.$Proxy1._call_endpoint(Unknown Source)
	at org.eclipse.ecf.provider.direct.protobuf.ProtobufCallableEndpointImpl.call_endpoint(ProtobufCallableEndpointImpl.java:76)
	at org.eclipse.ecf.provider.direct.protobuf.ProtobufCallableEndpointImpl.call_endpoint(ProtobufCallableEndpointImpl.java:92)
	at org.eclipse.ecf.provider.py4j.protobuf.ProtobufPy4jProviderImpl$2$1.call_endpoint(ProtobufPy4jProviderImpl.java:85)
	at org.eclipse.ecf.provider.direct.protobuf.ProtobufClientContainer$ProtoBufDirectRemoteService.invokeSync(ProtobufClientContainer.java:144)
	... 4 more
Caused by: py4j.PythonThrowable: exeception in class A
        at C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py:16 run
            raise Exception('exeception in class A') from exc
        at C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\run.py:45 sayHello
            aa.run()
        at C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py:334 wrapper
            respb = func(args[0],argInst)
        at C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py:337 wrapper
            raise e
        at C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py:155 _raw_bytes_from_java
            return getattr(self,methodName)(serializedArgs)
        at C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\bridge.py:523 _call_endpoint
            return endpoint._raw_bytes_from_java(methodName,serializedArgs)
        at C:\Users\slewis\git\scottslewis.py4j\py4j-python\src\py4j\java_gateway.py:2270 _call_proxy
            return_value = getattr(self.pool[obj_id], method)(*params)
        ---Python Stack---
        Traceback (most recent call last):
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 14, in run
            b.run()
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\b.py", line 9, in run
            1 / 0
        ZeroDivisionError: division by zero
        
        The above exception was the direct cause of the following exception:
        
        Traceback (most recent call last):
          File "C:\Users\slewis\git\scottslewis.py4j\py4j-python\src\py4j\java_gateway.py", line 2270, in _call_proxy
            return_value = getattr(self.pool[obj_id], method)(*params)
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\bridge.py", line 523, in _call_endpoint
            return endpoint._raw_bytes_from_java(methodName,serializedArgs)
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 155, in _raw_bytes_from_java
            return getattr(self,methodName)(serializedArgs)
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 337, in wrapper
            raise e
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 334, in wrapper
            respb = func(args[0],argInst)
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\run.py", line 45, in sayHello
            aa.run()
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 16, in run
            raise Exception('exeception in class A') from exc
        Exception: exeception in class A

Caused by: py4j.PythonThrowable: division by zero
        at C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\b.py:9 run
            1 / 0
        at C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py:14 run
            b.run()
        ---Python Stack---
        Traceback (most recent call last):
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 14, in run
            b.run()
          File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\b.py", line 9, in run
            1 / 0
        ZeroDivisionError: division by zero

---

Explanation: The top of the stack is this exception: org.osgi.framework.ServiceException. OSGi Remote Services spec requires this runtime exception be thrown if an exception occurs in remote service. The cause of the ServiceException is the underlying exception thrown by the distribution provider.

The next level down exception is: Caused by: org.eclipse.ecf.core.util.ECFException. This wrapping exception is thrown if any ECF distribution provider (ECF Remote Services has a pluggable distribution provider approach) throws a runtime exception.

The next level down is Caused by: py4j.Py4JException: An exception was raised by the Python Proxy. This is a result of changing line 458 from this (current):

			throw new Py4JException("An exception was raised by the Python Proxy. Return Message: " + result);

to this:

                        throw new Py4JException("An exception was raised by the Python Proxy",new PythonThrowable((String)result));

Note this passes the python stack trace (in String result) to new PythonThrowable instance for parsing.

The cause of the Py4JException is set to the PythonThrowable as discussed in previous comments.

The next level down in the stack trace is the output of the top-level PythonThrowable.toString()...i.e. line starting with:

Caused by: py4j.PythonThrowable: exeception in class A
...

This includes the java (top to bottom) of the Python stack and after ---Python Stack--- the python version. In Eclipse, both the java and python File lines are automatically parsed and a link is created to the appropriate Python line (assuming .py file is accessible in workspace). I decided for testing to put both the java (1st) and python (2nd) stack trace in output. It's possible to use only one or the other.

In this case the python exception had set as it's cause another exception (chained) and so the underlying python exception is given as

Caused by: py4j.PythonThrowable: division by zero
...

Note if this had been caused by another chained python exception (or raised as a result of an exception as per pep 3134) it would have an additional Caused By. It this case the division by zero is the root exception.

This example output was generated with this code:

try {
     	HelloMsgContent result = helloService.sayHello(createRequest());
} catch (Exception e) {
     e.printStackTrace();
}

if the code had this:

try {
     	HelloMsgContent result = helloService.sayHello(createRequest());
} catch (Exception e) {
     PythonThrowable pt = (PythonThrowable) e.getCause().getCause().getCause();
     pt.printStackTrace();
}

The output is:

Traceback (most recent call last):
  File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 14, in run
    b.run()
  File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\b.py", line 9, in run
    1 / 0
ZeroDivisionError: division by zero

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "C:\Users\slewis\git\scottslewis.py4j\py4j-python\src\py4j\java_gateway.py", line 2270, in _call_proxy
    return_value = getattr(self.pool[obj_id], method)(*params)
  File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\bridge.py", line 523, in _call_endpoint
    return endpoint._raw_bytes_from_java(methodName,serializedArgs)
  File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 155, in _raw_bytes_from_java
    return getattr(self,methodName)(serializedArgs)
  File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 337, in wrapper
    raise e
  File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 334, in wrapper
    respb = func(args[0],argInst)
  File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\run.py", line 45, in sayHello
    aa.run()
  File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 16, in run
    raise Exception('exeception in class A') from exc

[bartdag]

Hi Scott, thanks a lot for the explanation and the examples. I'll invite other contributors to comment, but I believe this is a nice addition.

@JoshRosen @batterseapower @jonahkichwacoders : if you have a few minutes to spare, could you review the example stack trace in the last comment of this issue. You have all been involved in error handling and I would like to have your input on this proposed addition. Essentially, @batterseapower recently added the ability to ship python stack trace to the Java side when an exception occurs in a Python callback. The python stack trace is included in the exception message which is already a major improvement! @scottslewis took that exception trace string and generated a proper stack trace that can be used by IDEs or logging systems.

There were two concerns: (1) complexity and general "brittleness" of such implementation and (2) impedance mismatch between Java and Python exception traces. Before diving into the code, I wanted to have your opinion on this feature addition.

Thanks!

[scottslewis]

Hi Barthelemy,

There were two concerns: (1) complexity and general "brittleness" of such implementation

WRT brittleness: Although I don't think PythonThrowable is likely to be brittle, I would suggest that it be deployed with a try/catch (Exception) wrapper (for the PythonThrowable constructor) for an initial release, in order to improve robustness over time 'in the stack trace wild'. Also, people have 'unusual' Python stack trace output, these can be easily added to the test cases.

and (2) impedance mismatch between Java and Python exception traces.

A word about this: There are basically two main structural differences between Java and Python when it comes to handling stack traces: 1) Java has classes only while Python has functions and classes; 2) by default, java presents the stack trace in 'last exception first' order (Caused By) and Python 'first exception first' order. As with the example in the comment above, the current impl of PythonThrowable.printStackTrace() presents the stack first in Java order, but for PythonThrowable also includes the stack trace (and for caused by) in Python-style/order after ---Python Stack--- To include both with java first was a design choice on my part. It could be presented other ways (e.g. just the java or just the python stack trace rather than both) with this same implementation. Note that PythonThrowable has API to get one, the other, or both programmatically.

[scottslewis]

Are there plans to merge this?

If so, as mentioned in earlier comment, I would suggest the following...so as to prevent a possible PythonThrowable parsing failure (e.g. due to changes in Python format) from breaking things:

FWIW one thing that could be done is to wrap the PythonThrowable constructor in a try/catch block, and if it fails for any reason then simply include the String unparsed in Py4jException to be thrown as is done now. This would provide some insurance that the communication wouldn't be broken by having the parsing fail.