Skip to content

Commit 2e3fef7

Browse files
jasonginmhdawson
authored andcommitted
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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 2bbabb1 commit 2e3fef7

File tree

4 files changed

+38
-27
lines changed

4 files changed

+38
-27
lines changed

doc/api/n-api.md

-7
Original file line numberDiff line numberDiff line change
@@ -2855,13 +2855,6 @@ will be invoked with a status value of `napi_cancelled`.
28552855
The work should not be deleted before the `complete`
28562856
callback invocation, even when it was cancelled.
28572857

2858-
**Note:** As mentioned in the section on memory management, if
2859-
the code to be run in the callbacks will create N-API values, then
2860-
N-API handle scope functions must be used to create/destroy a
2861-
`napi_handle_scope` such that the scope is active when
2862-
objects can be created.
2863-
2864-
28652858
### napi_create_async_work
28662859
<!-- YAML
28672860
added: v8.0.0

src/node_api.cc

+20-1
Original file line numberDiff line numberDiff line change
@@ -2766,7 +2766,26 @@ class Work {
27662766
Work* work = static_cast<Work*>(req->data);
27672767

27682768
if (work->_complete != nullptr) {
2769-
work->_complete(work->_env, ConvertUVErrorCode(status), work->_data);
2769+
napi_env env = work->_env;
2770+
2771+
// Establish a handle scope here so that every callback doesn't have to.
2772+
// Also it is needed for the exception-handling below.
2773+
v8::HandleScope scope(env->isolate);
2774+
2775+
work->_complete(env, ConvertUVErrorCode(status), work->_data);
2776+
2777+
// Note: Don't access `work` after this point because it was
2778+
// likely deleted by the complete callback.
2779+
2780+
// If there was an unhandled exception in the complete callback,
2781+
// report it as a fatal exception. (There is no JavaScript on the
2782+
// callstack that can possibly handle it.)
2783+
if (!env->last_exception.IsEmpty()) {
2784+
v8::TryCatch try_catch;
2785+
env->isolate->ThrowException(
2786+
v8::Local<v8::Value>::New(env->isolate, env->last_exception));
2787+
node::FatalException(env->isolate, try_catch);
2788+
}
27702789
}
27712790
}
27722791

test/addons-napi/test_async/test.js

+18
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,30 @@
11
'use strict';
22
const common = require('../../common');
33
const assert = require('assert');
4+
const child_process = require('child_process');
45
const test_async = require(`./build/${common.buildType}/test_async`);
56

7+
const testException = 'test_async_cb_exception';
8+
9+
// Exception thrown from async completion callback.
10+
// (Tested in a spawned process because the exception is fatal.)
11+
if (process.argv[2] === 'child') {
12+
test_async.Test(1, common.mustCall(function(err, val) {
13+
throw new Error(testException);
14+
}));
15+
return;
16+
}
17+
const p = child_process.spawnSync(
18+
process.execPath, [ '--napi-modules', __filename, 'child' ]);
19+
assert.ifError(p.error);
20+
assert.ok(p.stderr.toString().includes(testException));
21+
22+
// Successful async execution and completion callback.
623
test_async.Test(5, common.mustCall(function(err, val) {
724
assert.strictEqual(err, null);
825
assert.strictEqual(val, 10);
926
process.nextTick(common.mustCall());
1027
}));
1128

29+
// Async work item cancellation with callback.
1230
test_async.TestCancel(common.mustCall());

test/addons-napi/test_async/test_async.cc

-19
Original file line numberDiff line numberDiff line change
@@ -20,22 +20,6 @@ typedef struct {
2020
carrier the_carrier;
2121
carrier async_carrier[MAX_CANCEL_THREADS];
2222

23-
struct AutoHandleScope {
24-
explicit AutoHandleScope(napi_env env)
25-
: _env(env),
26-
_scope(nullptr) {
27-
napi_open_handle_scope(_env, &_scope);
28-
}
29-
~AutoHandleScope() {
30-
napi_close_handle_scope(_env, _scope);
31-
}
32-
private:
33-
AutoHandleScope() { }
34-
35-
napi_env _env;
36-
napi_handle_scope _scope;
37-
};
38-
3923
void Execute(napi_env env, void* data) {
4024
#if defined _WIN32
4125
Sleep(1000);
@@ -53,7 +37,6 @@ void Execute(napi_env env, void* data) {
5337
}
5438

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

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

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

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

128109
if (status == napi_cancelled) {

0 commit comments

Comments
 (0)