Avoid starting up the JVM when logging python errors #330
Conversation
_log_exception's signature suggests that it is only meant to handle Java exceptions, however in many places it is called with Python exceptions. This has the unintended effect of starting up the JVM whenever debug logging is enabled. We should thus be careful to either (a) use _log_exception everywhere but add case logic to avoid starting up the JVM early, or (b) use _log_exception ONLY when providing a Java exception.
ctrueden
left a comment
ctrueden
left a comment
There was a problem hiding this comment.
Good catch, @gselzer. Indeed, _log_exception is in the _java area, and like you said, from the type annotations, clearly meant to be passed only Java exceptions. We should enforce this at runtime with an assert or type check with TypeError to cut down on future programming mistakes like this.
The only gray area is if you somehow have an exception that might be a Java exception, but might not? Are there any cases like that currently in the codebase?
Yeah, I thought about this a little bit but not deeply. I suppose you could check if the JVM is already running (it couldn't be a Java Exception if the JVM hasn't started, right?), and if so whether it's a java exception? Does checking if it's a JPype |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/fiji-friends-weekly-dev-update-thread/103718/109 |
There are many locations in
__init__.pywhere_log_exceptionis used to log debug messages during ImageJ startup.log_exception's signature suggests that it is only meant to handle Java exceptions, however in many places it is called with Python exceptions. This has the unintended effect of starting up the JVM whenever debug logging is enabled.We should thus be careful to either (a) use _log_exception everywhere but add case logic to avoid starting up the JVM early, or (b) use _log_exception ONLY when providing a Java exception.