Hi Lee,
On Tue, 8 Jul 2014, Lee Skillen wrote:
Andi, thanks for the reply - I've created a github mirror of the pylucene
project for our own use (which I intend to keep synced with your SVN
repository as its official upstream), located at:
https://github.com/lskillen/pylucene
As suggested I have formatted (and attached) a patch of the unfinished code
that we're using for the through-layer exceptions. Alternatively the diff
can be inspected via github by diff'ing between the new
feature-thru-exception branch that I have created and the master branch, as
such:
https://github.com/lskillen/pylucene/compare/feature-thru-exception?expand=1
Although we've run the test suite without issues I realise there may still
be functionality/style/logical issues with the code. I also suspect that
there may not be specific test cases that target regression failures for
exceptions (yet), so confidence isn't high! All comments are welcome and I
realise this will likely require further changes and a repeatable test case
before it is acceptable, but that's fine.
I took a look at your patch and used the main idea to rewrite it so that:
- ref handling is correct (hopefully): you had it backwards when calling
Py_Restore(), it steals what you currently must own.
- the Python error state is now saved as one tuple of (type, value, tb) on
PythonException, not as three strings and three fake Objects (longs).
- ref handling is correct via a finalize() method DECREF'ing the saved
error state tuple on PythonException when that exception is collected.
- getMessage() still works as before but the traceback is extracted on
demand, not at throw time as was done before.
The previous implementation was done so that no Python cross-VM reference
had to be tracked, all the error data was string'ified at error time.
Your change now requires that the error be kept 'alive' accross layers and
refs must be tracked to avoid leaks.
I did _not_ test this as I don't have a system currently running that uses
JCC in reverse like this. Since you're actively using the feature, please
test the attached patch (against trunk) and report back with comments, bugs,
fixes, etc...
Thanks !
Andi..
Thanks,
Lee
On 4 July 2014 18:17, Andi Vajda <va...@apache.org> wrote:
On Jul 4, 2014, at 18:33, Lee Skillen <lskil...@vulcanft.com> wrote:
Hi Andi,
First of all, absolutely fantastic work on the JCC project, we love it -
Second of all, apologies for contacting you out of the blue via email, but
I wasn't sure of the best place to contact you to ask a few questions
(technical-level, not support-level). If it isn't acceptable to email
please let me know and I can move the discussion somewhere else.
Yes, please include pylucene-dev@lucene.apache.org (subscription
required. send mail pylucene-dev-subscribe@ and follow the instructions
in the reply).
A bit of background first of all :-
We've been integrating JCC for the past two weeks on a sizeable
Python/Java/C++ project (where Python is a controller/glue layer) having
migrated from a previous solution to integrate across the languages, and
for the most part JCC has been a lifesaver.
Technically we've only hit one major issue so far and that has been the
translation of exceptions through the layers. Our Java application does a
lot of double-dispatch (visitation) type of execution and we're calling
outwards to Python to implement the concrete visitation logic.
In the case of an exception being thrown it results in JCC collating a
traceback and captured exception at every level of scope, resulting in a
giant JavaError exception which has lost it's original PythonException
context.
I was actually able to fix this by attempting to capture the full context
via PyErr_Fetch() when the first python object is thrown and storing it
within PythonException, and then ensuring that this is carried through each
layer appropriately - This seems to work very well, although admittedly I
haven't considered regressive errors or compatibility yet, but just wanted
to proof-of-concept it first of all then speak with a JCC maintainer.
The net result is being able to catch errors in a pipeline similar to the
following:
Python
-> Java
-> Python
-> Raise MyException
<- Throw PythonException (containing MyException)
<- Inject MyException
Catch MyException
My main questions are to ask your thoughts about :-
- What are your thoughts about through-layer exceptions and a feature
improvement like this?
I agree that losing information during exception throwing is not good. If
you've got an improvement in the form of a patch, do not hesitate in
sending it in.
- JCC is (I think) currently at home with pylucene, are there are plans to
making it separate?
No such plans at the moment.
- Are you happy if I create a public github project and add the patched
code in there for review?
You're welcome to fork the code if you'd like but you don't have to if all
you want is sending a patch. Your call.
Thank you for the kind words !
Andi..
Hope to hear from you!
Cheers,
Lee
TL;DR;
We would like to :-
- Improve JCC's through-layer exception support.
- Establish a github project for JCC alone.
--
Lee Skillen
Vulcan Financial Technologies
1st Floor, 47 Malone Road, Belfast, BT9 6RY
Office: +44 (0)28 95 817888
Mobile: +44 (0)78 41 425152
Web: www.vulcanft.com
--
Lee Skillen
Vulcan Financial Technologies
1st Floor, 47 Malone Road, Belfast, BT9 6RY
Office: +44 (0)28 95 817888
Mobile: +44 (0)78 41 425152
Web: www.vulcanft.com
Index: CHANGES
===================================================================
--- CHANGES (revision 1608439)
+++ CHANGES (working copy)
@@ -1,7 +1,11 @@
+Version 2.20 ->
+--------------------
+ - improved through-layer Python error handling (with Lee Skillen)
+ -
+
Version 2.19 -> 2.20
--------------------
- added support for java varargs by reconstructing array from last parameters
- -
Version 2.18 -> 2.19
--------------------
Index: java/org/apache/jcc/PythonException.java
===================================================================
--- java/org/apache/jcc/PythonException.java (revision 1608439)
+++ java/org/apache/jcc/PythonException.java (working copy)
@@ -18,21 +18,29 @@
public class PythonException extends RuntimeException {
public boolean withTrace = true;
- protected String message, errorName, traceback;
+ protected long py_error_state = 0L;
public PythonException(String message)
{
super(message);
- getErrorInfo(); // sets errorName, message and traceback
+ saveErrorState();
}
+ public void finalize()
+ throws Throwable
+ {
+ pythonDecRef();
+ }
+
public String getMessage(boolean trace)
{
- if (message == null)
- message = super.getMessage();
+ if (py_error_state == 0L)
+ return super.getMessage();
+ String message = getErrorMessage();
+
if (trace)
- return message + "\n" + traceback;
+ return message + "\n" + getErrorTraceback();
return message;
}
@@ -42,16 +50,12 @@
return getMessage(withTrace);
}
- public String getErrorName()
- {
- return errorName;
- }
+ public native void pythonDecRef();
- public String getTraceback()
- {
- return traceback;
- }
+ public native String getErrorName();
+ public native String getErrorMessage();
+ public native String getErrorTraceback();
+ public native void clear();
- protected native void getErrorInfo();
- public native void clear();
+ protected native void saveErrorState();
}
Index: jcc/sources/functions.cpp
===================================================================
--- jcc/sources/functions.cpp (revision 1608440)
+++ jcc/sources/functions.cpp (working copy)
@@ -1355,11 +1355,43 @@
{
JNIEnv *vm_env = env->get_vm_env();
jthrowable throwable = vm_env->ExceptionOccurred();
- PyObject *err;
vm_env->ExceptionClear();
- err = t_Throwable::wrap_Object(Throwable(throwable));
+ jclass pycls = env->getPythonExceptionClass();
+
+ // Support through-layer exceptions by taking the active PythonException
+ // and making the enclosed exception visible to Python again.
+
+ if (vm_env->IsSameObject(vm_env->GetObjectClass(throwable), pycls))
+ {
+ jfieldID fid = vm_env->GetFieldID(pycls, "py_error_state", "J");
+ PyObject *state = (PyObject *) vm_env->GetLongField(throwable, fid);
+
+ if (state != NULL)
+ {
+ PyObject *type = PyTuple_GET_ITEM(state, 0);
+ PyObject *value = PyTuple_GET_ITEM(state, 1);
+ PyObject *tb = PyTuple_GET_ITEM(state, 2);
+
+ Py_INCREF(type);
+ if (value == Py_None)
+ value = NULL;
+ else
+ Py_INCREF(value);
+ if (tb == Py_None)
+ tb = NULL;
+ else
+ Py_INCREF(tb);
+
+ PyErr_Restore(type, value, tb);
+
+ return NULL;
+ }
+ }
+
+ PyObject *err = t_Throwable::wrap_Object(Throwable(throwable));
+
PyErr_SetObject(PyExc_JavaError, err);
Py_DECREF(err);
Index: jcc/sources/jcc.cpp
===================================================================
--- jcc/sources/jcc.cpp (revision 1608439)
+++ jcc/sources/jcc.cpp (working copy)
@@ -750,68 +750,160 @@
}
};
-static void JNICALL _PythonException_getErrorInfo(JNIEnv *vm_env, jobject self)
+static void JNICALL _PythonException_pythonDecRef(JNIEnv *vm_env, jobject self)
{
+ jclass jcls = vm_env->GetObjectClass(self);
+ jfieldID fid = vm_env->GetFieldID(jcls, "py_error_state", "J");
+ PyObject *state = (PyObject *) vm_env->GetLongField(self, fid);
+
+ if (state != NULL)
+ {
+ PythonGIL gil(vm_env);
+
+ Py_DECREF(state);
+ vm_env->SetLongField(self, fid, (jlong) 0);
+ }
+}
+
+static void JNICALL _PythonException_saveErrorState(JNIEnv *vm_env,
+ jobject self)
+{
PythonGIL gil(vm_env);
+ PyObject *type, *value, *tb;
- if (!PyErr_Occurred())
- return;
+ PyErr_Fetch(&type, &value, &tb);
- PyObject *type, *value, *tb, *errorName;
+ if (type != NULL)
+ {
+ PyObject *state = PyTuple_New(3);
+
+ PyErr_NormalizeException(&type, &value, &tb);
+ PyTuple_SET_ITEM(state, 0, type);
+ if (value == NULL)
+ {
+ PyTuple_SET_ITEM(state, 1, Py_None);
+ Py_INCREF(Py_None);
+ }
+ else
+ PyTuple_SET_ITEM(state, 1, value);
+ if (tb == NULL)
+ {
+ PyTuple_SET_ITEM(state, 2, Py_None);
+ Py_INCREF(Py_None);
+ }
+ else
+ PyTuple_SET_ITEM(state, 2, tb);
+
+ jclass jcls = vm_env->GetObjectClass(self);
+ jfieldID fid = vm_env->GetFieldID(jcls, "py_error_state", "J");
+
+ vm_env->SetLongField(self, fid, (jlong) state);
+ }
+}
+
+static jstring JNICALL _PythonException_getErrorName(JNIEnv *vm_env,
+ jobject self)
+{
jclass jcls = vm_env->GetObjectClass(self);
+ jfieldID fid = vm_env->GetFieldID(jcls, "py_error_state", "J");
+ PyObject *state = (PyObject *) vm_env->GetLongField(self, fid);
+
+ if (state == NULL)
+ return NULL;
- PyErr_Fetch(&type, &value, &tb);
+ PythonGIL gil(vm_env);
+ PyObject *errorName =
+ PyObject_GetAttrString(PyTuple_GET_ITEM(state, 0), "__name__");
- errorName = PyObject_GetAttrString(type, "__name__");
if (errorName != NULL)
{
- jfieldID fid =
- vm_env->GetFieldID(jcls, "errorName", "Ljava/lang/String;");
jstring str = env->fromPyString(errorName);
+ Py_DECREF(errorName);
- vm_env->SetObjectField(self, fid, str);
- vm_env->DeleteLocalRef(str);
- Py_DECREF(errorName);
+ return str;
}
- if (value != NULL)
+ return NULL;
+}
+
+static jstring JNICALL _PythonException_getErrorMessage(JNIEnv *vm_env,
+ jobject self)
+{
+ jclass jcls = vm_env->GetObjectClass(self);
+ jfieldID fid = vm_env->GetFieldID(jcls, "py_error_state", "J");
+ PyObject *state = (PyObject *) vm_env->GetLongField(self, fid);
+
+ if (state == NULL)
+ return NULL;
+
+ PythonGIL gil(vm_env);
+ PyObject *value = PyTuple_GET_ITEM(state, 1);
+
+ if (value != Py_None)
{
PyObject *message = PyObject_Str(value);
if (message != NULL)
{
- jfieldID fid =
- vm_env->GetFieldID(jcls, "message", "Ljava/lang/String;");
jstring str = env->fromPyString(message);
+ Py_DECREF(message);
- vm_env->SetObjectField(self, fid, str);
- vm_env->DeleteLocalRef(str);
- Py_DECREF(message);
+ return str;
}
}
+ return NULL;
+}
+
+static jstring JNICALL _PythonException_getErrorTraceback(JNIEnv *vm_env,
+ jobject self)
+{
+ jclass jcls = vm_env->GetObjectClass(self);
+ jfieldID fid = vm_env->GetFieldID(jcls, "py_error_state", "J");
+ PyObject *state = (PyObject *) vm_env->GetLongField(self, fid);
+
+ if (state == NULL)
+ return NULL;
+
+ PythonGIL gil(vm_env);
PyObject *module = NULL, *cls = NULL, *stringIO = NULL, *result = NULL;
PyObject *_stderr = PySys_GetObject("stderr");
+
if (!_stderr)
- goto err;
+ return NULL;
module = PyImport_ImportModule("cStringIO");
if (!module)
- goto err;
+ return NULL;
cls = PyObject_GetAttrString(module, "StringIO");
Py_DECREF(module);
if (!cls)
- goto err;
+ return NULL;
stringIO = PyObject_CallObject(cls, NULL);
Py_DECREF(cls);
if (!stringIO)
- goto err;
+ return NULL;
Py_INCREF(_stderr);
PySys_SetObject("stderr", stringIO);
+ PyObject *type = PyTuple_GET_ITEM(state, 0);
+ PyObject *value = PyTuple_GET_ITEM(state, 1);
+ PyObject *tb = PyTuple_GET_ITEM(state, 2);
+ jstring str = NULL;
+
+ Py_INCREF(type);
+ if (value == Py_None)
+ value = NULL;
+ else
+ Py_INCREF(value);
+ if (tb == Py_None)
+ tb = NULL;
+ else
+ Py_INCREF(tb);
+
PyErr_Restore(type, value, tb);
PyErr_Print();
@@ -820,22 +912,14 @@
if (result != NULL)
{
- jfieldID fid =
- vm_env->GetFieldID(jcls, "traceback", "Ljava/lang/String;");
- jstring str = env->fromPyString(result);
-
- vm_env->SetObjectField(self, fid, str);
- vm_env->DeleteLocalRef(str);
+ str = env->fromPyString(result);
Py_DECREF(result);
}
PySys_SetObject("stderr", _stderr);
Py_DECREF(_stderr);
- return;
-
- err:
- PyErr_Restore(type, value, tb);
+ return str;
}
static void JNICALL _PythonException_clear(JNIEnv *vm_env, jobject self)
@@ -848,7 +932,14 @@
{
jclass cls = vm_env->FindClass("org/apache/jcc/PythonException");
JNINativeMethod methods[] = {
- { "getErrorInfo", "()V", (void *) _PythonException_getErrorInfo },
+ { "pythonDecRef", "()V", (void *) _PythonException_pythonDecRef },
+ { "saveErrorState", "()V", (void *) _PythonException_saveErrorState },
+ { "getErrorName", "()Ljava/lang/String;",
+ (void *) _PythonException_getErrorName },
+ { "getErrorMessage", "()Ljava/lang/String;",
+ (void *) _PythonException_getErrorMessage },
+ { "getErrorTraceback", "()Ljava/lang/String;",
+ (void *) _PythonException_getErrorTraceback },
{ "clear", "()V", (void *) _PythonException_clear },
};
Index: setup.py
===================================================================
--- setup.py (revision 1608439)
+++ setup.py (working copy)
@@ -12,7 +12,7 @@
import os, sys, platform, subprocess
-jcc_ver = '2.19'
+jcc_ver = '2.20'
machine = platform.machine()
if machine.startswith("iPod") or machine.startswith("iPhone"):
