n-api: Handle fatal exception in async callback #12838

jasongin · 2017-05-04T22:46:27Z

Create a handle scope before invoking the async completion callback, because it is basically always needed, easy for user code to forget, and this makes it more consistent with ordinary N-API function callbacks that already have a scope established.
Check for an unhandled JS exception after invoking an async completion callback, and report it via node::FatalException().
Add a corresponding test case for an exception in async callback.

Previously, any unhandled JS exception thrown from a napi_async_complete_callback would be silently ignored. Among other things this meant assertions in some test cases could be undetected.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

n-api

- Create a handle scope before invoking the async completion callback, because it is basically always needed, easy for user code to forget, and this makes it more consistent with ordinary N-API function callbacks. - Check for an unhandled JS exception after invoking an async completion callback, and report it via `node::FatalException()`. - Add a corresponding test case for an exception in async callback. Previously, any unhandled JS exception thrown from a `napi_async_complete_callback` would be silently ignored. Among other things this meant assertions in some test cases could be undetected.

After nodejs/node#12838 is merged, the exception will be properly reported.

addaleax · 2017-05-04T22:54:20Z

src/node_api.cc

+      // Note: Don't access `work` after this point because it was
+      // likely deleted by the complete callback.
+
+      // If there was an unhnadled exception in the complete callback,


typo: unhandled

addaleax · 2017-05-04T23:09:04Z

src/node_api.cc

+      // Also it is needed for the exception-handling below.
+      v8::HandleScope scope(env->isolate);
+
+      work->_complete(env, ConvertUVErrorCode(status), work->_data);


So … I’ve given this a bit of thought before. Semantically and technically it would make the most sense if _complete ran inside a MakeCallback scope. Unfortunately, right now that would mean that _complete would need to be specified as a JS function (which would probably even be fine except that nobody would remember to call napi_delete_async_work inside it).

Once #11883 lands, I would like to look into splitting the enter and exit parts of MakeCallback into their own parts (like what https://github.com/matthewloring/node/commit/dd178336a60c2698619b96a62267bd528c09ae0d does for Promises), and ideally using them here to give the users an easier API.

Do you think that’s a good idea? I think it makes sense but it might require some weird hacks in N-API for the time being (like wrapping the _complete callback inside a JS function just to pass it to MakeCallback…)

Semantically and technically it would make the most sense if _complete ran inside a MakeCallback scope.

Conceptually that makes sense to me. But would the extra call to JS and back be a performance issue? And I'd prefer to avoid depending on any new APIs (refactoring MakeCallback() into enter and exit parts) because that would make back-porting the N-API code more difficult.

But anyway, that would not address the unhandled-exception issue. Based on my experimentation, MakeCallback() doesn't do anything to report unhandled exceptions from the JavaScript function that it invokes. Should that be considered a bug?

Conceptually that makes sense to me. But would the extra call to JS and back be a performance issue? And I'd prefer to avoid depending on any new APIs (refactoring MakeCallback() into enter and exit parts) because that would make back-porting the N-API code more difficult.

Sorry, maybe I didn’t express myself well – my thought was that we’d use a JS wrapper for now, and for backported APIs, and them move from that to the new APIs once they exist in Node core (and only for the Node core implementation).

The extra call to JS and back probably has a slight performance impact, but I’d guess that’s okay when compared to the general overhead of doing async work.

But anyway, that would not address the unhandled-exception issue.

Yes, it’s orthogonal to this PR and won’t block it in any way. :)

Based on my experimentation, MakeCallback() doesn't do anything to report unhandled exceptions from the JavaScript function that it invokes. Should that be considered a bug?

I don’t think so – it should probably leave the caller the chance to clean up for themselves when an error occurred (also, implementing it in another way would probably be weird with nested MakeCallbacks).

So the main benefit then would be so that user C++ callback code doesn't have to use napi_make_callback() to call JS code; it could use the regular napi_call_function() instead?

We'd still need to keep the napi_make_callback() API to support advanced async scenarios though.

If I understand correctly it would be less about convenience and more about safety. If javascript is execute is should be in the state/context that MakeCallback provides. If we enforce that, it is safer than documenting and asking the implementer to make sure they do that.

addaleax · 2017-05-04T23:10:06Z

test/addons-napi/test_async/test.js

 const test_async = require(`./build/${common.buildType}/test_async`);

+const testException = 'test_async_cb_exception';


we typically just say child and check process.argv[2] === 'child' directly ;)

mhdawson

LGTM

mhdawson · 2017-05-07T21:20:05Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/7938/

mhdawson · 2017-05-09T19:16:06Z

CI run good landing

mhdawson · 2017-05-09T19:24:28Z

Landed as 2e3fef7

- Create a handle scope before invoking the async completion callback, because it is basically always needed, easy for user code to forget, and this makes it more consistent with ordinary N-API function callbacks. - Check for an unhandled JS exception after invoking an async completion callback, and report it via `node::FatalException()`. - Add a corresponding test case for an exception in async callback. Previously, any unhandled JS exception thrown from a `napi_async_complete_callback` would be silently ignored. Among other things this meant assertions in some test cases could be undetected. PR-URL: #12838 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

- Create a handle scope before invoking the async completion callback, because it is basically always needed, easy for user code to forget, and this makes it more consistent with ordinary N-API function callbacks. - Check for an unhandled JS exception after invoking an async completion callback, and report it via `node::FatalException()`. - Add a corresponding test case for an exception in async callback. Previously, any unhandled JS exception thrown from a `napi_async_complete_callback` would be silently ignored. Among other things this meant assertions in some test cases could be undetected. PR-URL: nodejs#12838 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

- Create a handle scope before invoking the async completion callback, because it is basically always needed, easy for user code to forget, and this makes it more consistent with ordinary N-API function callbacks. - Check for an unhandled JS exception after invoking an async completion callback, and report it via `node::FatalException()`. - Add a corresponding test case for an exception in async callback. Previously, any unhandled JS exception thrown from a `napi_async_complete_callback` would be silently ignored. Among other things this meant assertions in some test cases could be undetected. Backport-PR-URL: #19447 PR-URL: #12838 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels May 4, 2017

jasongin mentioned this pull request May 4, 2017

Test and fix AsyncWorker nodejs/node-addon-api#30

Merged

addaleax added dont-land-on-v4.x labels May 4, 2017

jasongin added a commit to jasongin/node-addon-api that referenced this pull request May 4, 2017

Rethrow async cb error as JS exception

cb46bc5

After nodejs/node#12838 is merged, the exception will be properly reported.

addaleax approved these changes May 4, 2017

View reviewed changes

benjamingr approved these changes May 4, 2017

View reviewed changes

Address review feedback

c355a15

mhdawson approved these changes May 7, 2017

View reviewed changes

jasnell approved these changes May 9, 2017

View reviewed changes

mhdawson closed this May 9, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: Handle fatal exception in async callback #12838

n-api: Handle fatal exception in async callback #12838

jasongin commented May 4, 2017

addaleax May 4, 2017

jasongin May 4, 2017

addaleax May 4, 2017

jasongin May 4, 2017

addaleax May 4, 2017

jasongin May 4, 2017

mhdawson May 7, 2017

addaleax May 4, 2017

jasongin May 4, 2017

mhdawson left a comment

mhdawson commented May 7, 2017

mhdawson commented May 9, 2017

mhdawson commented May 9, 2017

		const test_async = require(`./build/${common.buildType}/test_async`);

		const testException = 'test_async_cb_exception';

n-api: Handle fatal exception in async callback #12838

n-api: Handle fatal exception in async callback #12838

Conversation

jasongin commented May 4, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented May 7, 2017

mhdawson commented May 9, 2017

mhdawson commented May 9, 2017