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

return valid Python Exceptions when js2python is applied to javascript errors #891

Merged
merged 13 commits into from Dec 23, 2020

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Dec 18, 2020

This is intended to resolve #888, necessary for PR #880 to work correctly.
Work in progress.

Currently there is a serious issue: if the Javascript Error was created without new, this will crash the Pyodide runtime. I have absolutely no idea why.

It would be better to implement the `JsException` class
in C to fix `JsException_AsJs`.
Currently `JsException_AsJs` can cause a Python exception
and I don't think it will be handled correctly.

Also there is a serious issue: if the Javascript `Error` was created
without `new`, this will crash the Pyodide runtime.
I have no idea why, I guess I need to carefully investigate the
difference between a `new Error()` and `Error()`.
@hoodmane hoodmane changed the title Implement a wrapper for javascript errors. return valid Python Exceptions when js2python is applied to javascript errors Dec 18, 2020
@hoodmane hoodmane mentioned this pull request Dec 18, 2020
@hoodmane hoodmane force-pushed the jsproxy-exception branch 2 times, most recently from e173e28 to 7b71e97 Compare December 18, 2020 22:22
@hoodmane
Copy link
Member Author

I think this is ready now @oeway.

Copy link
Contributor

@oeway oeway left a comment

Choose a reason for hiding this comment

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

Looks good!

I have two questions related to this: 1) have you figured why it's crashing when using Error without new? 2) if we do a round trip from js to python then back to js, will we get the same error object?

src/type_conversion/jsproxy.c Outdated Show resolved Hide resolved
@hoodmane
Copy link
Member Author

hoodmane commented Dec 19, 2020

  1. have you figured why it's crashing when using Error without new?

Yes I was incorrectly using a C Python API causing undefined behavior. This particular symptom was bad luck. It is fixed.

  1. if we do a round trip from js to python then back to js, will we get the same error object?

Yes. See test here:
https://github.com/iodide-project/pyodide/blob/ad90a07b5ea6003e2947f0486641292cb53ac42a/src/tests/test_pyodide.py#L41

@oeway
Copy link
Contributor

oeway commented Dec 20, 2020

Great, thank you!

@hoodmane
Copy link
Member Author

@rth I think this is ready to be merged unless you have further concerns?

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @hoodmane and thank you for the review @oeway !

Two comments below, otherwise LGTM.

Please also add an entry to docs/changelog.md.

src/tests/test_pyodide.py Outdated Show resolved Hide resolved
src/type_conversion/jsproxy.c Show resolved Hide resolved
@hoodmane
Copy link
Member Author

hoodmane commented Dec 21, 2020

I also added an entry for JsException in the api docs. To be clear, I'm envisioning code like:

from js import js_async_func
from pyodide import JsException
async def wrapper(arg):
   try:
      await js_async_func(arg)
   except JsException:
      raise ValueError(f"Unknown thunk {arg}")

@rth
Copy link
Member

rth commented Dec 22, 2020

Could you please resolve conflicts following #909 ? Otherwise should be good to merge assuming CI passes.

I have a doubt, would this address #636 or not?

@hoodmane
Copy link
Member Author

hoodmane commented Dec 22, 2020

This does not fix #636. To do that we should:

  • Add hiwire_seterr(id) which will do PyErr_SetObject(JsProxy_new_error(id)).
  • Adjust hiwire_call and hiwire_call_member to use try + catch and call hiwire_setexc(hiwire_new(err)) for any caught error.

I can implement that as a separte PR after this is merged, or I could add it to this PR, whichever you prefer.

Error conversion in the other direction is also not very good: currently if python code throws an error, it prints stuff to stdout directly, but it should instead be possible to catch the error in javascript. To get the errors printed to stdout, we should explicitly catch them and call some separate print function.

@rth rth changed the title return valid Python Exceptions when js2python is applied to javascript errors Raise Exceptions when js2python is applied to javascript errors Dec 23, 2020
@rth rth merged commit bb604b3 into pyodide:master Dec 23, 2020
@hoodmane
Copy link
Member Author

I think the title "Raise Exceptions when js2python is applied to javascript errors" is misleading: js2python doesn't raise an error at all, it returns an exception that can be raised. These are very different things:

f raises an error:

def f():
  raise Exception("hi")

f returns an error:

def f():
  return Exception("hi")

@rth
Copy link
Member

rth commented Dec 23, 2020

I had to shorten it as it was too long for a commit message. Didn't mean to rename the issue only the commit. I agree that's not ideal.

@rth rth changed the title Raise Exceptions when js2python is applied to javascript errors return valid Python Exceptions when js2python is applied to javascript errors Dec 23, 2020
@hoodmane
Copy link
Member Author

Thanks!

@hoodmane hoodmane deleted the jsproxy-exception branch December 23, 2020 20:55
joemarshall pushed a commit to joemarshall/pyodide that referenced this pull request Jan 3, 2021
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.

JsProxy does not handle Error correctly
3 participants