Skip to content

Commit b900bd6

Browse files
committed
n-api: emit uncaught-exception on unhandled tsfn callbacks
1 parent 5248a17 commit b900bd6

File tree

12 files changed

+283
-49
lines changed

12 files changed

+283
-49
lines changed

doc/api/cli.md

+8
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,13 @@ reference. Code may break under this flag.
340340
`--require` runs prior to freezing intrinsics in order to allow polyfills to
341341
be added.
342342

343+
### `--force-node-api-uncaught-exceptions-policy`
344+
<!-- YAML
345+
added: REPLACEME
346+
-->
347+
348+
Enables 'uncaughtException' event on Node API asynchronous callbacks.
349+
343350
### `--heapsnapshot-near-heap-limit=max_count`
344351
<!-- YAML
345352
added: v15.1.0
@@ -1383,6 +1390,7 @@ Node.js options that are allowed are:
13831390
* `--experimental-wasm-modules`
13841391
* `--force-context-aware`
13851392
* `--force-fips`
1393+
* `--force-node-api-uncaught-exceptions-policy`
13861394
* `--frozen-intrinsics`
13871395
* `--heapsnapshot-near-heap-limit`
13881396
* `--heapsnapshot-signal`

src/js_native_api_v8.h

+12-13
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,7 @@ struct napi_env__ {
5757
context_persistent(isolate, context) {
5858
CHECK_EQ(isolate, context->GetIsolate());
5959
}
60-
virtual ~napi_env__() {
61-
// First we must finalize those references that have `napi_finalizer`
62-
// callbacks. The reason is that addons might store other references which
63-
// they delete during their `napi_finalizer` callbacks. If we deleted such
64-
// references here first, they would be doubly deleted when the
65-
// `napi_finalizer` deleted them subsequently.
66-
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
67-
v8impl::RefTracker::FinalizeAll(&reflist);
68-
}
60+
virtual ~napi_env__() { FinalizeAll(); }
6961
v8::Isolate* const isolate; // Shortcut for context()->GetIsolate()
7062
v8impl::Persistent<v8::Context> context_persistent;
7163

@@ -102,10 +94,17 @@ struct napi_env__ {
10294
}
10395

10496
virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
105-
v8::HandleScope handle_scope(isolate);
106-
CallIntoModule([&](napi_env env) {
107-
cb(env, data, hint);
108-
});
97+
// Forward declaration virtual member. Implemented in node_napi_env__.
98+
}
99+
100+
void FinalizeAll() {
101+
// First we must finalize those references that have `napi_finalizer`
102+
// callbacks. The reason is that addons might store other references which
103+
// they delete during their `napi_finalizer` callbacks. If we deleted such
104+
// references here first, they would be doubly deleted when the
105+
// `napi_finalizer` deleted them subsequently.
106+
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
107+
v8impl::RefTracker::FinalizeAll(&reflist);
109108
}
110109

111110
v8impl::Persistent<v8::Value> last_exception;

src/node_api.cc

+61-35
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "node_buffer.h"
99
#include "node_errors.h"
1010
#include "node_internals.h"
11+
#include "node_process.h"
1112
#include "threadpoolwork-inl.h"
1213
#include "tracing/traced_value.h"
1314
#include "util-inl.h"
@@ -21,6 +22,8 @@ struct node_napi_env__ : public napi_env__ {
2122
CHECK_NOT_NULL(node_env());
2223
}
2324

25+
~node_napi_env__() { FinalizeAll(); }
26+
2427
inline node::Environment* node_env() const {
2528
return node::Environment::GetCurrent(context());
2629
}
@@ -37,15 +40,54 @@ struct node_napi_env__ : public napi_env__ {
3740
v8::True(isolate));
3841
}
3942

43+
inline void trigger_fatal_exception(v8::Local<v8::Value> local_err) {
44+
v8::Local<v8::Message> local_msg =
45+
v8::Exception::CreateMessage(isolate, local_err);
46+
node::errors::TriggerUncaughtException(isolate, local_err, local_msg);
47+
}
48+
49+
// option enforceUncaughtExceptionPolicy is added for not breaking existing
50+
// running n-api add-ons, and should be deprecated in the next major Node.js
51+
// release.
52+
template <typename T>
53+
inline void CallbackIntoModule(T&& call,
54+
bool enforceUncaughtExceptionPolicy = false) {
55+
CallIntoModule(
56+
call,
57+
[enforceUncaughtExceptionPolicy](napi_env env_,
58+
v8::Local<v8::Value> local_err) {
59+
node_napi_env__* env = static_cast<node_napi_env__*>(env_);
60+
node::Environment* node_env = env->node_env();
61+
if (!node_env->options()->force_node_api_uncaught_exceptions_policy &&
62+
!enforceUncaughtExceptionPolicy) {
63+
ProcessEmitDeprecationWarning(
64+
node_env,
65+
"Uncaught N-API callback exception detected, please run node "
66+
"with option --force-node-api-uncaught-exceptions-policy to handle "
67+
"those exceptions properly.",
68+
"DEP0XXX");
69+
return;
70+
}
71+
// If there was an unhandled exception in the complete callback,
72+
// report it as a fatal exception. (There is no JavaScript on the
73+
// callstack that can possibly handle it.)
74+
env->trigger_fatal_exception(local_err);
75+
});
76+
}
77+
4078
void CallFinalizer(napi_finalize cb, void* data, void* hint) override {
79+
CallFinalizer(cb, data, hint, true);
80+
}
81+
82+
inline void CallFinalizer(napi_finalize cb,
83+
void* data,
84+
void* hint,
85+
bool enforceUncaughtExceptionPolicy) {
4186
napi_env env = static_cast<napi_env>(this);
42-
node_env()->SetImmediate([=](node::Environment* node_env) {
43-
v8::HandleScope handle_scope(env->isolate);
44-
v8::Context::Scope context_scope(env->context());
45-
env->CallIntoModule([&](napi_env env) {
46-
cb(env, data, hint);
47-
});
48-
});
87+
v8::HandleScope handle_scope(env->isolate);
88+
v8::Context::Scope context_scope(env->context());
89+
CallbackIntoModule([&](napi_env env) { cb(env, data, hint); },
90+
enforceUncaughtExceptionPolicy);
4991
}
5092

5193
const char* GetFilename() const { return filename.c_str(); }
@@ -76,12 +118,9 @@ class BufferFinalizer : private Finalizer {
76118
v8::HandleScope handle_scope(finalizer->_env->isolate);
77119
v8::Context::Scope context_scope(finalizer->_env->context());
78120

79-
finalizer->_env->CallIntoModule([&](napi_env env) {
80-
finalizer->_finalize_callback(
81-
env,
82-
finalizer->_finalize_data,
83-
finalizer->_finalize_hint);
84-
});
121+
finalizer->_env->CallFinalizer(finalizer->_finalize_callback,
122+
finalizer->_finalize_data,
123+
finalizer->_finalize_hint);
85124
});
86125
}
87126

@@ -113,13 +152,6 @@ NewEnv(v8::Local<v8::Context> context, const std::string& module_filename) {
113152
return result;
114153
}
115154

116-
static inline void trigger_fatal_exception(
117-
napi_env env, v8::Local<v8::Value> local_err) {
118-
v8::Local<v8::Message> local_msg =
119-
v8::Exception::CreateMessage(env->isolate, local_err);
120-
node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg);
121-
}
122-
123155
class ThreadSafeFunction : public node::AsyncResource {
124156
public:
125157
ThreadSafeFunction(v8::Local<v8::Function> func,
@@ -318,19 +350,16 @@ class ThreadSafeFunction : public node::AsyncResource {
318350
v8::Local<v8::Function>::New(env->isolate, ref);
319351
js_callback = v8impl::JsValueFromV8LocalValue(js_cb);
320352
}
321-
env->CallIntoModule([&](napi_env env) {
322-
call_js_cb(env, js_callback, context, data);
323-
});
353+
env->CallbackIntoModule(
354+
[&](napi_env env) { call_js_cb(env, js_callback, context, data); });
324355
}
325356
}
326357

327358
void Finalize() {
328359
v8::HandleScope scope(env->isolate);
329360
if (finalize_cb) {
330361
CallbackScope cb_scope(this);
331-
env->CallIntoModule([&](napi_env env) {
332-
finalize_cb(env, finalize_data, context);
333-
});
362+
env->CallFinalizer(finalize_cb, finalize_data, context, false);
334363
}
335364
EmptyQueueAndDelete();
336365
}
@@ -718,7 +747,7 @@ napi_status napi_fatal_exception(napi_env env, napi_value err) {
718747
CHECK_ARG(env, err);
719748

720749
v8::Local<v8::Value> local_err = v8impl::V8LocalValueFromJsValue(err);
721-
v8impl::trigger_fatal_exception(env, local_err);
750+
static_cast<node_napi_env>(env)->trigger_fatal_exception(local_err);
722751

723752
return napi_clear_last_error(env);
724753
}
@@ -1068,14 +1097,11 @@ class Work : public node::AsyncResource, public node::ThreadPoolWork {
10681097

10691098
CallbackScope callback_scope(this);
10701099

1071-
_env->CallIntoModule([&](napi_env env) {
1072-
_complete(env, ConvertUVErrorCode(status), _data);
1073-
}, [](napi_env env, v8::Local<v8::Value> local_err) {
1074-
// If there was an unhandled exception in the complete callback,
1075-
// report it as a fatal exception. (There is no JavaScript on the
1076-
// callstack that can possibly handle it.)
1077-
v8impl::trigger_fatal_exception(env, local_err);
1078-
});
1100+
_env->CallbackIntoModule(
1101+
[&](napi_env env) {
1102+
_complete(env, ConvertUVErrorCode(status), _data);
1103+
},
1104+
true);
10791105

10801106
// Note: Don't access `work` after this point because it was
10811107
// likely deleted by the complete callback.

src/node_options.cc

+4
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
391391
"disable checks for async_hooks",
392392
&EnvironmentOptions::no_force_async_hooks_checks,
393393
kAllowedInEnvironment);
394+
AddOption("--force-node-api-uncaught-exceptions-policy",
395+
"disable 'uncaughtException' event on Node API asynchronous callbacks",
396+
&EnvironmentOptions::force_node_api_uncaught_exceptions_policy,
397+
kAllowedInEnvironment);
394398
AddOption("--no-warnings",
395399
"silence all process warnings",
396400
&EnvironmentOptions::no_warnings,

src/node_options.h

+1
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ class EnvironmentOptions : public Options {
114114
bool experimental_repl_await = false;
115115
bool experimental_vm_modules = false;
116116
bool expose_internals = false;
117+
bool force_node_api_uncaught_exceptions_policy = false;
117118
bool frozen_intrinsics = false;
118119
int64_t heap_snapshot_near_heap_limit = 0;
119120
std::string heap_snapshot_signal;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
'use strict';
2+
// Flags: --expose-gc --no-concurrent-array-buffer-freeing --no-concurrent-array-buffer-sweeping
3+
4+
const common = require('../../common');
5+
const test_reference = require(`./build/${common.buildType}/test_reference`);
6+
const assert = require('assert');
7+
8+
process.on('uncaughtException', common.mustCall((err) => {
9+
assert.throws(() => { throw err; }, /finalizer error/);
10+
}));
11+
12+
(async function() {
13+
{
14+
test_reference.createExternalWithJsFinalize(
15+
common.mustCall(() => {
16+
throw new Error('finalizer error');
17+
}));
18+
}
19+
global.gc();
20+
})().then(common.mustCall());

test/js-native-api/test_reference/test_reference.c

+42-1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,20 @@ static void FinalizeExternal(napi_env env, void* data, void* hint) {
2020
finalize_count++;
2121
}
2222

23+
static void FinalizeExternalCallJs(napi_env env, void* data, void* hint) {
24+
int *actual_value = data;
25+
NODE_API_ASSERT_RETURN_VOID(env, actual_value == &test_value,
26+
"The correct pointer was passed to the finalizer");
27+
28+
napi_ref finalizer_ref = (napi_ref)hint;
29+
napi_value js_finalizer;
30+
napi_value recv;
31+
NODE_API_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
32+
NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
33+
NODE_API_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
34+
NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
35+
}
36+
2337
static napi_value CreateExternal(napi_env env, napi_callback_info info) {
2438
int* data = &test_value;
2539

@@ -49,6 +63,31 @@ CreateExternalWithFinalize(napi_env env, napi_callback_info info) {
4963
return result;
5064
}
5165

66+
static napi_value
67+
CreateExternalWithJsFinalize(napi_env env, napi_callback_info info) {
68+
size_t argc = 1;
69+
napi_value args[1];
70+
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
71+
NODE_API_ASSERT(env, argc == 1, "Wrong number of arguments");
72+
napi_value finalizer = args[0];
73+
napi_valuetype finalizer_valuetype;
74+
NODE_API_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
75+
NODE_API_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
76+
napi_ref finalizer_ref;
77+
NODE_API_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
78+
79+
napi_value result;
80+
NODE_API_CALL(env,
81+
napi_create_external(env,
82+
&test_value,
83+
FinalizeExternalCallJs,
84+
finalizer_ref, /* finalize_hint */
85+
&result));
86+
87+
finalize_count = 0;
88+
return result;
89+
}
90+
5291
static napi_value CheckExternal(napi_env env, napi_callback_info info) {
5392
size_t argc = 1;
5493
napi_value arg;
@@ -173,14 +212,16 @@ napi_value Init(napi_env env, napi_value exports) {
173212
DECLARE_NODE_API_PROPERTY("createExternal", CreateExternal),
174213
DECLARE_NODE_API_PROPERTY("createExternalWithFinalize",
175214
CreateExternalWithFinalize),
215+
DECLARE_NODE_API_PROPERTY("createExternalWithJsFinalize",
216+
CreateExternalWithJsFinalize),
176217
DECLARE_NODE_API_PROPERTY("checkExternal", CheckExternal),
177218
DECLARE_NODE_API_PROPERTY("createReference", CreateReference),
178219
DECLARE_NODE_API_PROPERTY("deleteReference", DeleteReference),
179220
DECLARE_NODE_API_PROPERTY("incrementRefcount", IncrementRefcount),
180221
DECLARE_NODE_API_PROPERTY("decrementRefcount", DecrementRefcount),
181222
DECLARE_NODE_API_GETTER("referenceValue", GetReferenceValue),
182223
DECLARE_NODE_API_PROPERTY("validateDeleteBeforeFinalize",
183-
ValidateDeleteBeforeFinalize),
224+
ValidateDeleteBeforeFinalize),
184225
};
185226

186227
NODE_API_CALL(env, napi_define_properties(

test/node-api/test_buffer/test_buffer.c

+36
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,17 @@ static void noopDeleter(napi_env env, void* data, void* finalize_hint) {
2222
deleterCallCount++;
2323
}
2424

25+
static void malignDeleter(napi_env env, void* data, void* finalize_hint) {
26+
NODE_API_ASSERT_RETURN_VOID(env, data != NULL && strcmp(data, theText) == 0, "invalid data");
27+
napi_ref finalizer_ref = (napi_ref)finalize_hint;
28+
napi_value js_finalizer;
29+
napi_value recv;
30+
NODE_API_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
31+
NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
32+
NODE_API_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
33+
NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
34+
}
35+
2536
static napi_value newBuffer(napi_env env, napi_callback_info info) {
2637
napi_value theBuffer;
2738
char* theCopy;
@@ -107,6 +118,30 @@ static napi_value staticBuffer(napi_env env, napi_callback_info info) {
107118
return theBuffer;
108119
}
109120

121+
static napi_value malignFinalizerBuffer(napi_env env, napi_callback_info info) {
122+
size_t argc = 1;
123+
napi_value args[1];
124+
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
125+
NODE_API_ASSERT(env, argc == 1, "Wrong number of arguments");
126+
napi_value finalizer = args[0];
127+
napi_valuetype finalizer_valuetype;
128+
NODE_API_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
129+
NODE_API_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
130+
napi_ref finalizer_ref;
131+
NODE_API_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
132+
133+
napi_value theBuffer;
134+
NODE_API_CALL(
135+
env,
136+
napi_create_external_buffer(env,
137+
sizeof(theText),
138+
(void*)theText,
139+
malignDeleter,
140+
finalizer_ref, // finalize_hint
141+
&theBuffer));
142+
return theBuffer;
143+
}
144+
110145
static napi_value Init(napi_env env, napi_value exports) {
111146
napi_value theValue;
112147

@@ -123,6 +158,7 @@ static napi_value Init(napi_env env, napi_value exports) {
123158
DECLARE_NODE_API_PROPERTY("bufferHasInstance", bufferHasInstance),
124159
DECLARE_NODE_API_PROPERTY("bufferInfo", bufferInfo),
125160
DECLARE_NODE_API_PROPERTY("staticBuffer", staticBuffer),
161+
DECLARE_NODE_API_PROPERTY("malignFinalizerBuffer", malignFinalizerBuffer),
126162
};
127163

128164
NODE_API_CALL(env, napi_define_properties(

0 commit comments

Comments
 (0)