GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — node-api: emit uncaught-exception on unhandled tsfn callbacks · nodejs/node@73f852e

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
  • doc/api
    • cli.md
  • src
    • js_native_api_v8.h
    • node_api.cc
    • node_api_internals.h
    • node_options.cc
    • node_options.h
  • test
    • js-native-api/test_reference
      • test_finalizer.js
      • test_reference.c
    • node-api
      • test_buffer
        • test_buffer.c
        • test_finalizer.js
      • test_threadsafe_function
        • binding.c
        • test_force_uncaught_exception.js
        • test_uncaught_exception.js
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎doc/api/cli.md‎

Copy file name to clipboardExpand all lines: doc/api/cli.md
+13Lines changed: 13 additions & 0 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,18 @@ under this flag.
419419

420420
To allow polyfills to be added, `--require` runs before freezing intrinsics.
421421

422+
### `--force-node-api-uncaught-exceptions-policy`
423+
424+
<!-- YAML
425+
added: REPLACEME
426+
-->
427+
428+
Enforces `uncaughtException` event on Node-API asynchronous callbacks.
429+
430+
To prevent from an existing add-on from crashing the process, this flag is not
431+
enabled by default. In the future, this flag will be enabled by default to
432+
enforce the correct behavior.
433+
422434
### `--heapsnapshot-near-heap-limit=max_count`
423435

424436
<!-- YAML
@@ -1576,6 +1588,7 @@ Node.js options that are allowed are:
15761588
* `--experimental-wasm-modules`
15771589
* `--force-context-aware`
15781590
* `--force-fips`
1591+
* `--force-node-api-uncaught-exceptions-policy`
15791592
* `--frozen-intrinsics`
15801593
* `--heapsnapshot-near-heap-limit`
15811594
* `--heapsnapshot-signal`
Collapse file

‎src/js_native_api_v8.h‎

Copy file name to clipboardExpand all lines: src/js_native_api_v8.h
+13-9Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,7 @@ struct napi_env__ {
5757
CHECK_EQ(isolate, context->GetIsolate());
5858
napi_clear_last_error(this);
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,11 +94,23 @@ struct napi_env__ {
10294
}
10395
}
10496

97+
// This should be overridden to schedule the finalization to a properiate
98+
// timing, like next tick of the event loop.
10599
virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
106100
v8::HandleScope handle_scope(isolate);
107101
CallIntoModule([&](napi_env env) { cb(env, data, hint); });
108102
}
109103

104+
void FinalizeAll() {
105+
// First we must finalize those references that have `napi_finalizer`
106+
// callbacks. The reason is that addons might store other references which
107+
// they delete during their `napi_finalizer` callbacks. If we deleted such
108+
// references here first, they would be doubly deleted when the
109+
// `napi_finalizer` deleted them subsequently.
110+
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
111+
v8impl::RefTracker::FinalizeAll(&reflist);
112+
}
113+
110114
v8impl::Persistent<v8::Value> last_exception;
111115

112116
// We store references in two different lists, depending on whether they have
Collapse file

‎src/node_api.cc‎

Copy file name to clipboardExpand all lines: src/node_api.cc
+65-36Lines changed: 65 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "node_buffer.h"
1010
#include "node_errors.h"
1111
#include "node_internals.h"
12+
#include "node_process.h"
1213
#include "node_url.h"
1314
#include "threadpoolwork-inl.h"
1415
#include "tracing/traced_value.h"
@@ -23,6 +24,11 @@ node_napi_env__::node_napi_env__(v8::Local<v8::Context> context,
2324
CHECK_NOT_NULL(node_env());
2425
}
2526

27+
node_napi_env__::~node_napi_env__() {
28+
destructing = true;
29+
FinalizeAll();
30+
}
31+
2632
bool node_napi_env__::can_call_into_js() const {
2733
return node_env()->can_call_into_js();
2834
}
@@ -35,19 +41,64 @@ v8::Maybe<bool> node_napi_env__::mark_arraybuffer_as_untransferable(
3541
}
3642

3743
void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) {
44+
CallFinalizer<true>(cb, data, hint);
45+
}
46+
47+
template <bool enforceUncaughtExceptionPolicy>
48+
void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) {
49+
if (destructing) {
50+
// we can not defer finalizers when the environment is being destructed.
51+
v8::HandleScope handle_scope(isolate);
52+
v8::Context::Scope context_scope(context());
53+
CallbackIntoModule<enforceUncaughtExceptionPolicy>(
54+
[&](napi_env env) { cb(env, data, hint); });
55+
return;
56+
}
3857
// we need to keep the env live until the finalizer has been run
3958
// EnvRefHolder provides an exception safe wrapper to Ref and then
4059
// Unref once the lambda is freed
4160
EnvRefHolder liveEnv(static_cast<napi_env>(this));
4261
node_env()->SetImmediate(
4362
[=, liveEnv = std::move(liveEnv)](node::Environment* node_env) {
44-
napi_env env = liveEnv.env();
63+
node_napi_env__* env = static_cast<node_napi_env__*>(liveEnv.env());
4564
v8::HandleScope handle_scope(env->isolate);
4665
v8::Context::Scope context_scope(env->context());
47-
env->CallIntoModule([&](napi_env env) { cb(env, data, hint); });
66+
env->CallbackIntoModule<enforceUncaughtExceptionPolicy>(
67+
[&](napi_env env) { cb(env, data, hint); });
4868
});
4969
}
5070

71+
void node_napi_env__::trigger_fatal_exception(v8::Local<v8::Value> local_err) {
72+
v8::Local<v8::Message> local_msg =
73+
v8::Exception::CreateMessage(isolate, local_err);
74+
node::errors::TriggerUncaughtException(isolate, local_err, local_msg);
75+
}
76+
77+
// option enforceUncaughtExceptionPolicy is added for not breaking existing
78+
// running n-api add-ons, and should be deprecated in the next major Node.js
79+
// release.
80+
template <bool enforceUncaughtExceptionPolicy, typename T>
81+
void node_napi_env__::CallbackIntoModule(T&& call) {
82+
CallIntoModule(call, [](napi_env env_, v8::Local<v8::Value> local_err) {
83+
node_napi_env__* env = static_cast<node_napi_env__*>(env_);
84+
node::Environment* node_env = env->node_env();
85+
if (!node_env->options()->force_node_api_uncaught_exceptions_policy &&
86+
!enforceUncaughtExceptionPolicy) {
87+
ProcessEmitDeprecationWarning(
88+
node_env,
89+
"Uncaught N-API callback exception detected, please run node "
90+
"with option --force-node-api-uncaught-exceptions-policy=true"
91+
"to handle those exceptions properly.",
92+
"DEP0XXX");
93+
return;
94+
}
95+
// If there was an unhandled exception in the complete callback,
96+
// report it as a fatal exception. (There is no JavaScript on the
97+
// callstack that can possibly handle it.)
98+
env->trigger_fatal_exception(local_err);
99+
});
100+
}
101+
51102
namespace v8impl {
52103

53104
namespace {
@@ -60,20 +111,10 @@ class BufferFinalizer : private Finalizer {
60111
static_cast<BufferFinalizer*>(hint)};
61112
finalizer->_finalize_data = data;
62113

63-
node::Environment* node_env =
64-
static_cast<node_napi_env>(finalizer->_env)->node_env();
65-
node_env->SetImmediate(
66-
[finalizer = std::move(finalizer)](node::Environment* env) {
67-
if (finalizer->_finalize_callback == nullptr) return;
68-
69-
v8::HandleScope handle_scope(finalizer->_env->isolate);
70-
v8::Context::Scope context_scope(finalizer->_env->context());
71-
72-
finalizer->_env->CallIntoModule([&](napi_env env) {
73-
finalizer->_finalize_callback(
74-
env, finalizer->_finalize_data, finalizer->_finalize_hint);
75-
});
76-
});
114+
if (finalizer->_finalize_callback == nullptr) return;
115+
finalizer->_env->CallFinalizer(finalizer->_finalize_callback,
116+
finalizer->_finalize_data,
117+
finalizer->_finalize_hint);
77118
}
78119

79120
struct Deleter {
@@ -102,13 +143,6 @@ static inline napi_env NewEnv(v8::Local<v8::Context> context,
102143
return result;
103144
}
104145

105-
static inline void trigger_fatal_exception(napi_env env,
106-
v8::Local<v8::Value> local_err) {
107-
v8::Local<v8::Message> local_msg =
108-
v8::Exception::CreateMessage(env->isolate, local_err);
109-
node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg);
110-
}
111-
112146
class ThreadSafeFunction : public node::AsyncResource {
113147
public:
114148
ThreadSafeFunction(v8::Local<v8::Function> func,
@@ -325,7 +359,7 @@ class ThreadSafeFunction : public node::AsyncResource {
325359
v8::Local<v8::Function>::New(env->isolate, ref);
326360
js_callback = v8impl::JsValueFromV8LocalValue(js_cb);
327361
}
328-
env->CallIntoModule(
362+
env->CallbackIntoModule<false>(
329363
[&](napi_env env) { call_js_cb(env, js_callback, context, data); });
330364
}
331365

@@ -336,7 +370,9 @@ class ThreadSafeFunction : public node::AsyncResource {
336370
v8::HandleScope scope(env->isolate);
337371
if (finalize_cb) {
338372
CallbackScope cb_scope(this);
339-
env->CallIntoModule(
373+
// Do not use CallFinalizer since it will defer the invocation, which
374+
// would lead to accessing a deleted ThreadSafeFunction.
375+
env->CallbackIntoModule<false>(
340376
[&](napi_env env) { finalize_cb(env, finalize_data, context); });
341377
}
342378
EmptyQueueAndDelete();
@@ -721,7 +757,7 @@ napi_status napi_fatal_exception(napi_env env, napi_value err) {
721757
CHECK_ARG(env, err);
722758

723759
v8::Local<v8::Value> local_err = v8impl::V8LocalValueFromJsValue(err);
724-
v8impl::trigger_fatal_exception(env, local_err);
760+
static_cast<node_napi_env>(env)->trigger_fatal_exception(local_err);
725761

726762
return napi_clear_last_error(env);
727763
}
@@ -1061,16 +1097,9 @@ class Work : public node::AsyncResource, public node::ThreadPoolWork {
10611097

10621098
CallbackScope callback_scope(this);
10631099

1064-
_env->CallIntoModule(
1065-
[&](napi_env env) {
1066-
_complete(env, ConvertUVErrorCode(status), _data);
1067-
},
1068-
[](napi_env env, v8::Local<v8::Value> local_err) {
1069-
// If there was an unhandled exception in the complete callback,
1070-
// report it as a fatal exception. (There is no JavaScript on the
1071-
// callstack that can possibly handle it.)
1072-
v8impl::trigger_fatal_exception(env, local_err);
1073-
});
1100+
_env->CallbackIntoModule<true>([&](napi_env env) {
1101+
_complete(env, ConvertUVErrorCode(status), _data);
1102+
});
10741103

10751104
// Note: Don't access `work` after this point because it was
10761105
// likely deleted by the complete callback.
Collapse file

‎src/node_api_internals.h‎

Copy file name to clipboardExpand all lines: src/node_api_internals.h
+8Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,26 @@
1111
struct node_napi_env__ : public napi_env__ {
1212
node_napi_env__(v8::Local<v8::Context> context,
1313
const std::string& module_filename);
14+
~node_napi_env__();
1415

1516
bool can_call_into_js() const override;
1617
v8::Maybe<bool> mark_arraybuffer_as_untransferable(
1718
v8::Local<v8::ArrayBuffer> ab) const override;
1819
void CallFinalizer(napi_finalize cb, void* data, void* hint) override;
20+
template <bool enforceUncaughtExceptionPolicy>
21+
void CallFinalizer(napi_finalize cb, void* data, void* hint);
22+
23+
void trigger_fatal_exception(v8::Local<v8::Value> local_err);
24+
template <bool enforceUncaughtExceptionPolicy, typename T>
25+
void CallbackIntoModule(T&& call);
1926

2027
inline node::Environment* node_env() const {
2128
return node::Environment::GetCurrent(context());
2229
}
2330
inline const char* GetFilename() const { return filename.c_str(); }
2431

2532
std::string filename;
33+
bool destructing = false;
2634
};
2735

2836
using node_napi_env = node_napi_env__*;
Collapse file

‎src/node_options.cc‎

Copy file name to clipboardExpand all lines: src/node_options.cc
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,12 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
410410
&EnvironmentOptions::force_async_hooks_checks,
411411
kAllowedInEnvironment,
412412
true);
413+
AddOption(
414+
"--force-node-api-uncaught-exceptions-policy",
415+
"enforces 'uncaughtException' event on Node API asynchronous callbacks",
416+
&EnvironmentOptions::force_node_api_uncaught_exceptions_policy,
417+
kAllowedInEnvironment,
418+
false);
413419
AddOption("--addons",
414420
"disable loading native addons",
415421
&EnvironmentOptions::allow_native_addons,
Collapse file

‎src/node_options.h‎

Copy file name to clipboardExpand all lines: src/node_options.h
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ class EnvironmentOptions : public Options {
116116
bool experimental_repl_await = true;
117117
bool experimental_vm_modules = false;
118118
bool expose_internals = false;
119+
bool force_node_api_uncaught_exceptions_policy = false;
119120
bool frozen_intrinsics = false;
120121
int64_t heap_snapshot_near_heap_limit = 0;
121122
std::string heap_snapshot_signal;
Collapse file

‎test/js-native-api/test_reference/test_finalizer.js‎

Copy file name to clipboard
+20Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
'use strict';
2+
// Flags: --expose-gc --force-node-api-uncaught-exceptions-policy
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());
Collapse file

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

Copy file name to clipboardExpand all lines: test/js-native-api/test_reference/test_reference.c
+42-1Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,20 @@ static void FinalizeExternal(napi_env env, void* data, void* hint) {
2121
finalize_count++;
2222
}
2323

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

@@ -99,6 +113,31 @@ CreateExternalWithFinalize(napi_env env, napi_callback_info info) {
99113
return result;
100114
}
101115

116+
static napi_value
117+
CreateExternalWithJsFinalize(napi_env env, napi_callback_info info) {
118+
size_t argc = 1;
119+
napi_value args[1];
120+
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
121+
NODE_API_ASSERT(env, argc == 1, "Wrong number of arguments");
122+
napi_value finalizer = args[0];
123+
napi_valuetype finalizer_valuetype;
124+
NODE_API_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
125+
NODE_API_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
126+
napi_ref finalizer_ref;
127+
NODE_API_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
128+
129+
napi_value result;
130+
NODE_API_CALL(env,
131+
napi_create_external(env,
132+
&test_value,
133+
FinalizeExternalCallJs,
134+
finalizer_ref, /* finalize_hint */
135+
&result));
136+
137+
finalize_count = 0;
138+
return result;
139+
}
140+
102141
static napi_value CheckExternal(napi_env env, napi_callback_info info) {
103142
size_t argc = 1;
104143
napi_value arg;
@@ -223,6 +262,8 @@ napi_value Init(napi_env env, napi_value exports) {
223262
DECLARE_NODE_API_PROPERTY("createExternal", CreateExternal),
224263
DECLARE_NODE_API_PROPERTY("createExternalWithFinalize",
225264
CreateExternalWithFinalize),
265+
DECLARE_NODE_API_PROPERTY("createExternalWithJsFinalize",
266+
CreateExternalWithJsFinalize),
226267
DECLARE_NODE_API_PROPERTY("checkExternal", CheckExternal),
227268
DECLARE_NODE_API_PROPERTY("createReference", CreateReference),
228269
DECLARE_NODE_API_PROPERTY("createSymbol", CreateSymbol),
@@ -234,7 +275,7 @@ napi_value Init(napi_env env, napi_value exports) {
234275
DECLARE_NODE_API_PROPERTY("decrementRefcount", DecrementRefcount),
235276
DECLARE_NODE_API_GETTER("referenceValue", GetReferenceValue),
236277
DECLARE_NODE_API_PROPERTY("validateDeleteBeforeFinalize",
237-
ValidateDeleteBeforeFinalize),
278+
ValidateDeleteBeforeFinalize),
238279
};
239280

240281
NODE_API_CALL(env, napi_define_properties(

0 commit comments

Comments
0 (0)
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.