Skip to content
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.

Sign up for GitHub

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

Fix #66: Fetch and store Python exceptions while in JS code #67

Closed
wants to merge 4 commits into from
Closed

Fix #66: Fetch and store Python exceptions while in JS code #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a3c51d9
Fetch and store Python exceptions while in JS code.
qwenger Oct 27, 2021
5bf43a2
Only use stored Python exception if handling a Python call JS error.
qwenger Oct 27, 2021
25979f8
Fix traceback.format_exception call if error value or traceback is null.
qwenger Oct 27, 2021
c603994
Add unit test for python exceptions in async environments.
qwenger Oct 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 59 additions & 2 deletions module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ typedef struct {
InterruptData interrupt_data;
// NULL-terminated singly linked list of callable Python objects that we need to keep alive.
PythonCallableNode *python_callables;
PyObject *last_python_error_type;
PyObject *last_python_error_value;
PyObject *last_python_error_traceback;
} ContextData;

// The data of the type _quickjs.Object.
Expand Down Expand Up @@ -259,6 +262,42 @@ static PyObject *quickjs_to_python(ContextData *context_obj, JSValue value) {
JSValue exception = JS_GetException(context);
const char *cstring = JS_ToCString(context, exception);
const char *stack_cstring = NULL;
PyObject *py_stack = NULL;
if (context_obj->last_python_error_value == NULL || cstring == NULL || strstr(cstring, "Python call failed.") == NULL) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check strstr(cstring, "Python call failed.") is quite brittle. We should not rely on the specific human-readable message of exceptions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in principle, but how else to catch the error at

quickjs/module.c

Line 629 in c603994

return JS_ThrowInternalError(ctx, "Python call failed.");
specifically? Use another error type?

py_stack = PyUnicode_FromString("");
} else {
PyObject *tbm = PyImport_ImportModule("traceback");
PyObject *tbfmt = PyObject_GetAttrString(tbm, "format_exception");
PyObject *errv = context_obj->last_python_error_value;
if (errv == NULL) {
errv = Py_None;
}
PyObject *errtb = context_obj->last_python_error_traceback;
if (errtb == NULL) {
errtb = Py_None;
}
Py_INCREF(errv);
Py_INCREF(errtb);
PyObject *tbstrs = PyObject_CallFunctionObjArgs(tbfmt, context_obj->last_python_error_type, errv, errtb, NULL);
PyObject *tbinfo = PyUnicode_FromString(
"\nThe above exception was caused by the following exception:\n\n");
PyList_Insert(tbstrs, 0, tbinfo);
PyObject *sep = PyUnicode_FromString("");
py_stack = PyUnicode_Join(sep, tbstrs);
Py_DECREF(errtb);
Py_DECREF(errv);
Py_DECREF(sep);
Py_DECREF(tbinfo);
Py_DECREF(tbstrs);
Py_DECREF(tbfmt);
Py_DECREF(tbm);
Py_DECREF(context_obj->last_python_error_type);
Py_XDECREF(context_obj->last_python_error_value);
Py_XDECREF(context_obj->last_python_error_traceback);
context_obj->last_python_error_type = NULL;
context_obj->last_python_error_value = NULL;
context_obj->last_python_error_traceback = NULL;
}
if (!JS_IsNull(exception) && !JS_IsUndefined(exception)) {
JSValue stack = JS_GetPropertyStr(context, exception, "stack");
if (!JS_IsException(stack)) {
Expand All @@ -269,9 +308,9 @@ static PyObject *quickjs_to_python(ContextData *context_obj, JSValue value) {
if (cstring != NULL) {
const char *safe_stack_cstring = stack_cstring ? stack_cstring : "";
if (strstr(cstring, "stack overflow") != NULL) {
PyErr_Format(StackOverflow, "%s\n%s", cstring, safe_stack_cstring);
PyErr_Format(StackOverflow, "%s\n%s%U", cstring, safe_stack_cstring, py_stack);
} else {
PyErr_Format(JSException, "%s\n%s", cstring, safe_stack_cstring);
PyErr_Format(JSException, "%s\n%s%U", cstring, safe_stack_cstring, py_stack);
}
} else {
// This has been observed to happen when different threads have used the same QuickJS
Expand All @@ -280,6 +319,7 @@ static PyObject *quickjs_to_python(ContextData *context_obj, JSValue value) {
PyErr_Format(JSException,
"(Failed obtaining QuickJS error string. Concurrency issue?)");
}
Py_DECREF(py_stack);
JS_FreeCString(context, cstring);
JS_FreeCString(context, stack_cstring);
JS_FreeValue(context, exception);
Expand Down Expand Up @@ -330,6 +370,9 @@ static PyObject *context_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
self->time_limit = 0;
self->thread_state = NULL;
self->python_callables = NULL;
self->last_python_error_type = NULL;
self->last_python_error_value = NULL;
self->last_python_error_traceback = NULL;
JS_SetContextOpaque(self->context, self);
}
return (PyObject *)self;
Expand All @@ -347,6 +390,9 @@ static void context_dealloc(ContextData *self) {
Py_DECREF(this->obj);
PyMem_Free(this);
}
Py_XDECREF(self->last_python_error_type);
Py_XDECREF(self->last_python_error_value);
Py_XDECREF(self->last_python_error_traceback);
Py_TYPE(self)->tp_free((PyObject *)self);
}

Expand Down Expand Up @@ -568,6 +614,17 @@ static JSValue js_c_function(
PyObject *result = PyObject_CallObject(node->obj, args);
Py_DECREF(args);
if (!result) {
Py_XDECREF(context->last_python_error_type);
Py_XDECREF(context->last_python_error_value);
Py_XDECREF(context->last_python_error_traceback);
PyErr_Fetch(
&context->last_python_error_type,
&context->last_python_error_value,
&context->last_python_error_traceback);
PyErr_NormalizeException(
&context->last_python_error_type,
&context->last_python_error_value,
&context->last_python_error_traceback);
end_call_python(context);
return JS_ThrowInternalError(ctx, "Python call failed.");
}
Expand Down
14 changes: 14 additions & 0 deletions test_quickjs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,20 @@ def test_list():
# instead of a JS exception.
self.context.eval("test_list()")

def test_python_exception_with_object_return_does_not_raise_system_error(self):
# https://github.com/PetterS/quickjs/issues/66

def python_raise():
raise Exception

self.context.add_callable("python_raise", python_raise)
# When called, `a` should return an object (a promise),
# even though a Python error is generated in the background.
self.context.eval("async function a() {await python_raise();}")
# With incorrect error handling, this raised a SystemError in dev builds,
# and segfaulted in prod builds.
self.assertEqual(self.context.eval("typeof a();"), "object")


class Object(unittest.TestCase):
def setUp(self):
Expand Down