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

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

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
6 participants
@jasongin
Copy link
Contributor

commented May 4, 2017

  • 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

n-api: Handle fatal exception in async callback
 - 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.

@jasongin jasongin referenced this pull request May 4, 2017

Merged

Test and fix AsyncWorker #30

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

Rethrow async cb error as JS exception
After nodejs/node#12838 is merged, the exception will be properly reported.
// 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,

This comment has been minimized.

Copy link
@addaleax

addaleax May 4, 2017

Member

typo: unhandled

This comment has been minimized.

Copy link
@jasongin

jasongin May 4, 2017

Author Contributor

Fixed.

// Also it is needed for the exception-handling below.
v8::HandleScope scope(env->isolate);

work->_complete(env, ConvertUVErrorCode(status), work->_data);

This comment has been minimized.

Copy link
@addaleax

addaleax May 4, 2017

Member

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…)

This comment has been minimized.

Copy link
@jasongin

jasongin May 4, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@addaleax

addaleax May 4, 2017

Member

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).

This comment has been minimized.

Copy link
@jasongin

jasongin May 4, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@mhdawson

mhdawson May 7, 2017

Member

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.

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

const testException = 'test_async_cb_exception';

This comment has been minimized.

Copy link
@addaleax

addaleax May 4, 2017

Member

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

This comment has been minimized.

Copy link
@jasongin

jasongin May 4, 2017

Author Contributor

Fixed.

@mhdawson
Copy link
Member

left a comment

LGTM

@mhdawson

This comment has been minimized.

Copy link
Member

commented May 7, 2017

@jasnell

jasnell approved these changes May 9, 2017

@mhdawson

This comment has been minimized.

Copy link
Member

commented May 9, 2017

CI run good landing

@mhdawson

This comment has been minimized.

Copy link
Member

commented May 9, 2017

Landed as 2e3fef7

@mhdawson mhdawson closed this May 9, 2017

mhdawson added a commit that referenced this pull request May 9, 2017

n-api: Handle fatal exception in async callback
 - 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>

anchnk added a commit to anchnk/node that referenced this pull request May 19, 2017

n-api: Handle fatal exception in async callback
 - 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>

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 12, 2018

n-api: Handle fatal exception in async callback
 - 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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018

n-api: Handle fatal exception in async callback
 - 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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018

n-api: Handle fatal exception in async callback
 - 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>

MylesBorins added a commit that referenced this pull request Apr 16, 2018

n-api: Handle fatal exception in async callback
 - 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>

@MylesBorins MylesBorins referenced this pull request Apr 16, 2018

Merged

v6.14.2 proposal #19996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.