Skip to content

Commit 16a18c0

Browse files
legendecaslegendecas
authored
fix: handle c++ exception in TSFN callback (#1345)
1 parent 4e62db4 commit 16a18c0

10 files changed

+204
-15
lines changed

‎napi-inl.h

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -263,10 +263,12 @@ struct ThreadSafeFinalize {
263263
template <typename ContextType, typename DataType, typename CallJs, CallJs call>
264264
inline typename std::enable_if<call != static_cast<CallJs>(nullptr)>::type
265265
CallJsWrapper(napi_env env, napi_value jsCallback, void* context, void* data) {
266-
call(env,
267-
Function(env, jsCallback),
268-
static_cast<ContextType*>(context),
269-
static_cast<DataType*>(data));
266+
details::WrapVoidCallback([&]() {
267+
call(env,
268+
Function(env, jsCallback),
269+
static_cast<ContextType*>(context),
270+
static_cast<DataType*>(data));
271+
});
270272
}
271273

272274
template <typename ContextType, typename DataType, typename CallJs, CallJs call>
@@ -275,9 +277,11 @@ CallJsWrapper(napi_env env,
275277
napi_value jsCallback,
276278
void* /*context*/,
277279
void* /*data*/) {
278-
if (jsCallback != nullptr) {
279-
Function(env, jsCallback).Call(0, nullptr);
280-
}
280+
details::WrapVoidCallback([&]() {
281+
if (jsCallback != nullptr) {
282+
Function(env, jsCallback).Call(0, nullptr);
283+
}
284+
});
281285
}
282286

283287
#if NAPI_VERSION > 4
@@ -6135,13 +6139,15 @@ inline void ThreadSafeFunction::CallJS(napi_env env,
61356139
return;
61366140
}
61376141

6138-
if (data != nullptr) {
6139-
auto* callbackWrapper = static_cast<CallbackWrapper*>(data);
6140-
(*callbackWrapper)(env, Function(env, jsCallback));
6141-
delete callbackWrapper;
6142-
} else if (jsCallback != nullptr) {
6143-
Function(env, jsCallback).Call({});
6144-
}
6142+
details::WrapVoidCallback([&]() {
6143+
if (data != nullptr) {
6144+
auto* callbackWrapper = static_cast<CallbackWrapper*>(data);
6145+
(*callbackWrapper)(env, Function(env, jsCallback));
6146+
delete callbackWrapper;
6147+
} else if (jsCallback != nullptr) {
6148+
Function(env, jsCallback).Call({});
6149+
}
6150+
});
61456151
}
61466152

61476153
////////////////////////////////////////////////////////////////////////////////

‎test/binding.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,14 @@ Object InitPromise(Env env);
5050
Object InitRunScript(Env env);
5151
#if (NAPI_VERSION > 3)
5252
Object InitThreadSafeFunctionCtx(Env env);
53+
Object InitThreadSafeFunctionException(Env env);
5354
Object InitThreadSafeFunctionExistingTsfn(Env env);
5455
Object InitThreadSafeFunctionPtr(Env env);
5556
Object InitThreadSafeFunctionSum(Env env);
5657
Object InitThreadSafeFunctionUnref(Env env);
5758
Object InitThreadSafeFunction(Env env);
5859
Object InitTypedThreadSafeFunctionCtx(Env env);
60+
Object InitTypedThreadSafeFunctionException(Env env);
5961
Object InitTypedThreadSafeFunctionExistingTsfn(Env env);
6062
Object InitTypedThreadSafeFunctionPtr(Env env);
6163
Object InitTypedThreadSafeFunctionSum(Env env);
@@ -139,6 +141,8 @@ Object Init(Env env, Object exports) {
139141
exports.Set("symbol", InitSymbol(env));
140142
#if (NAPI_VERSION > 3)
141143
exports.Set("threadsafe_function_ctx", InitThreadSafeFunctionCtx(env));
144+
exports.Set("threadsafe_function_exception",
145+
InitThreadSafeFunctionException(env));
142146
exports.Set("threadsafe_function_existing_tsfn",
143147
InitThreadSafeFunctionExistingTsfn(env));
144148
exports.Set("threadsafe_function_ptr", InitThreadSafeFunctionPtr(env));
@@ -147,6 +151,8 @@ Object Init(Env env, Object exports) {
147151
exports.Set("threadsafe_function", InitThreadSafeFunction(env));
148152
exports.Set("typed_threadsafe_function_ctx",
149153
InitTypedThreadSafeFunctionCtx(env));
154+
exports.Set("typed_threadsafe_function_exception",
155+
InitTypedThreadSafeFunctionException(env));
150156
exports.Set("typed_threadsafe_function_existing_tsfn",
151157
InitTypedThreadSafeFunctionExistingTsfn(env));
152158
exports.Set("typed_threadsafe_function_ptr",

‎test/binding.gyp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,15 @@
5555
'run_script.cc',
5656
'symbol.cc',
5757
'threadsafe_function/threadsafe_function_ctx.cc',
58+
'threadsafe_function/threadsafe_function_exception.cc',
5859
'threadsafe_function/threadsafe_function_existing_tsfn.cc',
5960
'threadsafe_function/threadsafe_function_ptr.cc',
6061
'threadsafe_function/threadsafe_function_sum.cc',
6162
'threadsafe_function/threadsafe_function_unref.cc',
6263
'threadsafe_function/threadsafe_function.cc',
6364
'type_taggable.cc',
6465
'typed_threadsafe_function/typed_threadsafe_function_ctx.cc',
66+
'typed_threadsafe_function/typed_threadsafe_function_exception.cc',
6567
'typed_threadsafe_function/typed_threadsafe_function_existing_tsfn.cc',
6668
'typed_threadsafe_function/typed_threadsafe_function_ptr.cc',
6769
'typed_threadsafe_function/typed_threadsafe_function_sum.cc',

‎test/child_processes/threadsafe_function_exception.js

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
'use strict';
2+
3+
const assert = require('assert');
4+
const common = require('../common');
5+
6+
module.exports = {
7+
testCall: async binding => {
8+
const { testCall } = binding.threadsafe_function_exception;
9+
10+
await new Promise(resolve => {
11+
process.once('uncaughtException', common.mustCall(err => {
12+
assert.strictEqual(err.message, 'test');
13+
resolve();
14+
}, 1));
15+
16+
testCall(common.mustCall(() => {
17+
throw new Error('test');
18+
}, 1));
19+
});
20+
},
21+
testCallWithNativeCallback: async binding => {
22+
const { testCallWithNativeCallback } = binding.threadsafe_function_exception;
23+
24+
await new Promise(resolve => {
25+
process.once('uncaughtException', common.mustCall(err => {
26+
assert.strictEqual(err.message, 'test-from-native');
27+
resolve();
28+
}, 1));
29+
30+
testCallWithNativeCallback();
31+
});
32+
}
33+
};

‎test/child_processes/typed_threadsafe_function_exception.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
'use strict';
2+
3+
const assert = require('assert');
4+
const common = require('../common');
5+
6+
module.exports = {
7+
testCall: async binding => {
8+
const { testCall } = binding.typed_threadsafe_function_exception;
9+
10+
await new Promise(resolve => {
11+
process.once('uncaughtException', common.mustCall(err => {
12+
assert.strictEqual(err.message, 'test-from-native');
13+
resolve();
14+
}, 1));
15+
16+
testCall();
17+
});
18+
}
19+
};

‎test/common/index.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,14 +202,15 @@ exports.runTestWithBuildType = async function (test, buildType) {
202202
// in the main process. Two examples are addon and addon_data, both of which
203203
// use Napi::Env::SetInstanceData(). This helper function provides a common
204204
// approach for running such tests.
205-
exports.runTestInChildProcess = function ({ suite, testName, expectedStderr }) {
205+
exports.runTestInChildProcess = function ({ suite, testName, expectedStderr, execArgv }) {
206206
return exports.runTestWithBindingPath((bindingName) => {
207207
return new Promise((resolve) => {
208208
bindingName = escapeBackslashes(bindingName);
209209
// Test suites are assumed to be located here.
210210
const suitePath = escapeBackslashes(path.join(__dirname, '..', 'child_processes', suite));
211211
const child = spawn(process.execPath, [
212212
'--expose-gc',
213+
...(execArgv ?? []),
213214
'-e',
214215
`require('${suitePath}').${testName}(require('${bindingName}'))`
215216
]);

‎test/threadsafe_function/threadsafe_function_exception.cc

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
#include <cstdlib>
2+
#include "napi.h"
3+
#include "test_helper.h"
4+
5+
#if (NAPI_VERSION > 3)
6+
7+
using namespace Napi;
8+
9+
namespace {
10+
11+
void CallJS(napi_env env, napi_value /* callback */, void* /*data*/) {
12+
Napi::Error error = Napi::Error::New(env, "test-from-native");
13+
NAPI_THROW_VOID(error);
14+
}
15+
16+
void TestCall(const CallbackInfo& info) {
17+
Napi::Env env = info.Env();
18+
19+
ThreadSafeFunction wrapped =
20+
ThreadSafeFunction::New(env,
21+
info[0].As<Napi::Function>(),
22+
Object::New(env),
23+
String::New(env, "Test"),
24+
0,
25+
1);
26+
wrapped.BlockingCall(static_cast<void*>(nullptr));
27+
wrapped.Release();
28+
}
29+
30+
void TestCallWithNativeCallback(const CallbackInfo& info) {
31+
Napi::Env env = info.Env();
32+
33+
ThreadSafeFunction wrapped = ThreadSafeFunction::New(
34+
env, Napi::Function(), Object::New(env), String::New(env, "Test"), 0, 1);
35+
wrapped.BlockingCall(static_cast<void*>(nullptr), CallJS);
36+
wrapped.Release();
37+
}
38+
39+
} // namespace
40+
41+
Object InitThreadSafeFunctionException(Env env) {
42+
Object exports = Object::New(env);
43+
exports["testCall"] = Function::New(env, TestCall);
44+
exports["testCallWithNativeCallback"] =
45+
Function::New(env, TestCallWithNativeCallback);
46+
47+
return exports;
48+
}
49+
50+
#endif

‎test/threadsafe_function/threadsafe_function_exception.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
module.exports = common.runTest(test);
6+
7+
const execArgv = ['--force-node-api-uncaught-exceptions-policy=true'];
8+
async function test () {
9+
await common.runTestInChildProcess({
10+
suite: 'threadsafe_function_exception',
11+
testName: 'testCall',
12+
execArgv
13+
});
14+
15+
await common.runTestInChildProcess({
16+
suite: 'threadsafe_function_exception',
17+
testName: 'testCallWithNativeCallback',
18+
execArgv
19+
});
20+
}

‎test/typed_threadsafe_function/typed_threadsafe_function_exception.cc

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
#include <cstdlib>
2+
#include "napi.h"
3+
#include "test_helper.h"
4+
5+
#if (NAPI_VERSION > 3)
6+
7+
using namespace Napi;
8+
9+
namespace {
10+
11+
void CallJS(Napi::Env env,
12+
Napi::Function /* callback */,
13+
std::nullptr_t* /* context */,
14+
void* /*data*/) {
15+
Napi::Error error = Napi::Error::New(env, "test-from-native");
16+
NAPI_THROW_VOID(error);
17+
}
18+
19+
using TSFN = TypedThreadSafeFunction<std::nullptr_t, void, CallJS>;
20+
21+
void TestCall(const CallbackInfo& info) {
22+
Napi::Env env = info.Env();
23+
24+
TSFN wrapped = TSFN::New(
25+
env, Napi::Function(), Object::New(env), String::New(env, "Test"), 0, 1);
26+
wrapped.BlockingCall(static_cast<void*>(nullptr));
27+
wrapped.Release();
28+
}
29+
30+
} // namespace
31+
32+
Object InitTypedThreadSafeFunctionException(Env env) {
33+
Object exports = Object::New(env);
34+
exports["testCall"] = Function::New(env, TestCall);
35+
36+
return exports;
37+
}
38+
39+
#endif

‎test/typed_threadsafe_function/typed_threadsafe_function_exception.js

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
module.exports = common.runTest(test);
6+
7+
async function test () {
8+
await common.runTestInChildProcess({
9+
suite: 'typed_threadsafe_function_exception',
10+
testName: 'testCall',
11+
execArgv: ['--force-node-api-uncaught-exceptions-policy=true']
12+
});
13+
}

0 commit comments

Comments
 (0)