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.

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

Conversation

qwenger
Copy link
Collaborator

@qwenger qwenger commented Oct 27, 2021

Fix #63, fix #66.

In contrast to JS, Python exceptions are global. So to support the case where an exception is raised in an asynchronous segment, one needs to store away the exception and unset the error indicator.

This stored exception can then be raised if it is returned on the JS side, or as part of a HostPromiseRejectionTracker.

Note: that mechanism is also needed to properly handle nested Python -> JS -> Python -> JS -> Python cases:

import quickjs
ctx = quickjs.Context()

def testa():
    ctx.eval("testb()")

def testb():
    raise Exception

ctx.add_callable("testa", testa)
ctx.add_callable("testb", testb)
ctx.eval("testa()")

This also supersedes #64.

@PetterS
Copy link
Owner

PetterS commented Oct 28, 2021

Thanks for this patch! Let me see if I have time to review it this weekend....

(I no longer work on this small library as part of my day-to-day work)

In the meantime, could you also add. a unit test demonstrating the bug?

@qwenger
Copy link
Collaborator Author

qwenger commented Oct 30, 2021

I added a unit test.

Please note that this PR and #65 step on each other's toes, so once you merge one of them (assuming you do :) ) I'll update the other one to make it mergeable.

Also, I'm a little bit rusty when it comes to the C-API, so my implementation may be a bit lacking stylistically - suggestions for improvement are welcome.

Once both PRs are merged, I'll probably submit a PR for implementing a host promise rejection tracker.

Copy link
Owner

@PetterS PetterS left a comment

Choose a reason for hiding this comment

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

Are you able to run the check_memory script to check whether all of the reference counting is correct?

@@ -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

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

@qwenger
Copy link
Collaborator Author

qwenger commented Nov 14, 2021

Are you able to run the check_memory script to check whether all of the reference counting is correct?

I'm not really sure about what to look for in the memory logs...

@qwenger
Copy link
Collaborator Author

qwenger commented Nov 17, 2021

Ok. Now I've more or less understood how check_memory.py works and I have proposed a couple enhancements (#75, #76, #78).

I also agree that the current implementation based on the error string is not ideal. Furthermore, saving the error globally in the context does not seem good either. Better would be - if doable - to encapsulate the Python error in the JS error, using a new JS error type (a subclass of InternalError?).

I'll rework this PR once the other ones (#65 plus the aforementioned ones about memory leaks) are integrated.

@PetterS
Copy link
Owner

PetterS commented Nov 23, 2021

Sounds good! Thanks for the improvements to the memory script.

qwenger added a commit to qwenger/quickjs that referenced this pull request Apr 8, 2022
…ns into custom InternalPythonError object extending InternalError.
@qwenger
Copy link
Collaborator Author

qwenger commented Apr 8, 2022

Superseded by #79 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in awaited Python call More verbose exceptions in callables
2 participants