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

Pass exceptions to JS that are thrown by JS functions in an async context #36402

Closed
ekraihan opened this issue Dec 5, 2020 · 3 comments
Closed
Labels
node-api Issues and PRs related to the Node-API.

Comments

@ekraihan
Copy link

ekraihan commented Dec 5, 2020

Is your feature request related to a problem? Please describe.

When invoking an async function, the only way to notify JS of exceptions thrown by the function is by checking the result of every async JS function call and invoking napi_fatal_exception if needed. It would be great if napi_fatal_exception was called automatically when exceptions propagate out of an async function.

Describe the solution you'd like

I would like the invoker of napi_threadsafe_function_call_js (which is passed into napi_create_threadsafe_function) to detect when exceptions bubble out of the async callback and call napi_fatal_exception(env, error); Currently, the napi_threadsafe_function_call_js is invoked by node_napi_env::CallIntoModule which runs the code

call(this); // invokes the napi_threadsafe_function_call_js
// ...
if (!last_exception.IsEmpty()) {
    handle_exception(this, last_exception.Get(this->isolate));
    last_exception.Reset();
}

where handle_exception by default calls v8::Isolate::ThrowException which does nothing in an async context.

Describe alternatives you've considered

Perhaps the real problem here is that v8::Isolate::ThrowException does not propagate exceptions to JS when invoked in an async context? Perhaps this is a bug with v8?

@targos targos added the node-api Issues and PRs related to the Node-API. label Dec 6, 2020
@targos
Copy link
Member

targos commented Dec 6, 2020

/cc @nodejs/n-api

@mhdawson
Copy link
Member

@legendecas just submitted a PR to uncaught exceptions so he will take a look at this one and comment.

@legendecas
Copy link
Member

I think this is what #36510 intended to fix.

bengl pushed a commit that referenced this issue May 30, 2022
PR-URL: #36510
Fixes: #36402
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Jul 12, 2022
PR-URL: #36510
Fixes: #36402
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Jul 31, 2022
PR-URL: #36510
Fixes: #36402
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
PR-URL: nodejs/node#36510
Fixes: nodejs/node#36402
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants