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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a9cae4
n-api: Handle fatal exception in async callback
jasongin May 4, 2017
c355a15
Address review feedback
jasongin May 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 0 additions & 7 deletions doc/api/n-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2855,13 +2855,6 @@ will be invoked with a status value of `napi_cancelled`.
The work should not be deleted before the `complete`
callback invocation, even when it was cancelled.

**Note:** As mentioned in the section on memory management, if
the code to be run in the callbacks will create N-API values, then
N-API handle scope functions must be used to create/destroy a
`napi_handle_scope` such that the scope is active when
objects can be created.


### napi_create_async_work
<!-- YAML
added: v8.0.0
Expand Down
21 changes: 20 additions & 1 deletion src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2766,7 +2766,26 @@ class Work {
Work* work = static_cast<Work*>(req->data);

if (work->_complete != nullptr) {
work->_complete(work->_env, ConvertUVErrorCode(status), work->_data);
napi_env env = work->_env;

// Establish a handle scope here so that every callback doesn't have to.
// Also it is needed for the exception-handling below.
v8::HandleScope scope(env->isolate);

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


// 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,
Copy link
Member

Choose a reason for hiding this comment

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

typo: unhandled

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

// report it as a fatal exception. (There is no JavaScript on the
// callstack that can possibly handle it.)
if (!env->last_exception.IsEmpty()) {
v8::TryCatch try_catch;
env->isolate->ThrowException(
v8::Local<v8::Value>::New(env->isolate, env->last_exception));
node::FatalException(env->isolate, try_catch);
}
}
}

Expand Down
18 changes: 18 additions & 0 deletions test/addons-napi/test_async/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,30 @@
'use strict';
const common = require('../../common');
const assert = require('assert');
const child_process = require('child_process');
const test_async = require(`./build/${common.buildType}/test_async`);

const testException = 'test_async_cb_exception';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.


// Exception thrown from async completion callback.
// (Tested in a spawned process because the exception is fatal.)
if (process.argv[2] === testException) {
test_async.Test(1, common.mustCall(function(err, val) {
throw new Error(testException);
}));
return;
}
const p = child_process.spawnSync(
process.execPath, [ '--napi-modules', __filename, testException ]);
assert.ifError(p.error);
assert.ok(p.stderr.toString().includes(testException));

// Successful async execution and completion callback.
test_async.Test(5, common.mustCall(function(err, val) {
assert.strictEqual(err, null);
assert.strictEqual(val, 10);
process.nextTick(common.mustCall());
}));

// Async work item cancellation with callback.
test_async.TestCancel(common.mustCall());
19 changes: 0 additions & 19 deletions test/addons-napi/test_async/test_async.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,6 @@ typedef struct {
carrier the_carrier;
carrier async_carrier[MAX_CANCEL_THREADS];

struct AutoHandleScope {
explicit AutoHandleScope(napi_env env)
: _env(env),
_scope(nullptr) {
napi_open_handle_scope(_env, &_scope);
}
~AutoHandleScope() {
napi_close_handle_scope(_env, _scope);
}
private:
AutoHandleScope() { }

napi_env _env;
napi_handle_scope _scope;
};

void Execute(napi_env env, void* data) {
#if defined _WIN32
Sleep(1000);
Expand All @@ -53,7 +37,6 @@ void Execute(napi_env env, void* data) {
}

void Complete(napi_env env, napi_status status, void* data) {
AutoHandleScope scope(env);
carrier* c = static_cast<carrier*>(data);

if (c != &the_carrier) {
Expand Down Expand Up @@ -116,13 +99,11 @@ napi_value Test(napi_env env, napi_callback_info info) {
}

void BusyCancelComplete(napi_env env, napi_status status, void* data) {
AutoHandleScope scope(env);
carrier* c = static_cast<carrier*>(data);
NAPI_CALL_RETURN_VOID(env, napi_delete_async_work(env, c->_request));
}

void CancelComplete(napi_env env, napi_status status, void* data) {
AutoHandleScope scope(env);
carrier* c = static_cast<carrier*>(data);

if (status == napi_cancelled) {
Expand Down