From 7564ab1773e63f91537e2efc05f016726af295ff Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Fri, 30 Jun 2023 09:56:38 +0000 Subject: [PATCH 1/2] fix: handle c++ exception in TSFN callback --- napi-inl.h | 34 +++++++----- test/binding.cc | 6 ++ test/binding.gyp | 2 + .../threadsafe_function_exception.cc | 50 +++++++++++++++++ .../threadsafe_function_exception.js | 55 +++++++++++++++++++ .../typed_threadsafe_function_exception.cc | 39 +++++++++++++ .../typed_threadsafe_function_exception.js | 45 +++++++++++++++ 7 files changed, 217 insertions(+), 14 deletions(-) create mode 100644 test/threadsafe_function/threadsafe_function_exception.cc create mode 100644 test/threadsafe_function/threadsafe_function_exception.js create mode 100644 test/typed_threadsafe_function/typed_threadsafe_function_exception.cc create mode 100644 test/typed_threadsafe_function/typed_threadsafe_function_exception.js diff --git a/napi-inl.h b/napi-inl.h index d8c813a73..492b8f61a 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -263,10 +263,12 @@ struct ThreadSafeFinalize { template inline typename std::enable_if(nullptr)>::type CallJsWrapper(napi_env env, napi_value jsCallback, void* context, void* data) { - call(env, - Function(env, jsCallback), - static_cast(context), - static_cast(data)); + details::WrapVoidCallback([&]() { + call(env, + Function(env, jsCallback), + static_cast(context), + static_cast(data)); + }); } template @@ -275,9 +277,11 @@ CallJsWrapper(napi_env env, napi_value jsCallback, void* /*context*/, void* /*data*/) { - if (jsCallback != nullptr) { - Function(env, jsCallback).Call(0, nullptr); - } + details::WrapVoidCallback([&]() { + if (jsCallback != nullptr) { + Function(env, jsCallback).Call(0, nullptr); + } + }); } #if NAPI_VERSION > 4 @@ -6135,13 +6139,15 @@ inline void ThreadSafeFunction::CallJS(napi_env env, return; } - if (data != nullptr) { - auto* callbackWrapper = static_cast(data); - (*callbackWrapper)(env, Function(env, jsCallback)); - delete callbackWrapper; - } else if (jsCallback != nullptr) { - Function(env, jsCallback).Call({}); - } + details::WrapVoidCallback([&]() { + if (data != nullptr) { + auto* callbackWrapper = static_cast(data); + (*callbackWrapper)(env, Function(env, jsCallback)); + delete callbackWrapper; + } else if (jsCallback != nullptr) { + Function(env, jsCallback).Call({}); + } + }); } //////////////////////////////////////////////////////////////////////////////// diff --git a/test/binding.cc b/test/binding.cc index dd5637c72..0415c69bc 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -50,12 +50,14 @@ Object InitPromise(Env env); Object InitRunScript(Env env); #if (NAPI_VERSION > 3) Object InitThreadSafeFunctionCtx(Env env); +Object InitThreadSafeFunctionException(Env env); Object InitThreadSafeFunctionExistingTsfn(Env env); Object InitThreadSafeFunctionPtr(Env env); Object InitThreadSafeFunctionSum(Env env); Object InitThreadSafeFunctionUnref(Env env); Object InitThreadSafeFunction(Env env); Object InitTypedThreadSafeFunctionCtx(Env env); +Object InitTypedThreadSafeFunctionException(Env env); Object InitTypedThreadSafeFunctionExistingTsfn(Env env); Object InitTypedThreadSafeFunctionPtr(Env env); Object InitTypedThreadSafeFunctionSum(Env env); @@ -139,6 +141,8 @@ Object Init(Env env, Object exports) { exports.Set("symbol", InitSymbol(env)); #if (NAPI_VERSION > 3) exports.Set("threadsafe_function_ctx", InitThreadSafeFunctionCtx(env)); + exports.Set("threadsafe_function_exception", + InitThreadSafeFunctionException(env)); exports.Set("threadsafe_function_existing_tsfn", InitThreadSafeFunctionExistingTsfn(env)); exports.Set("threadsafe_function_ptr", InitThreadSafeFunctionPtr(env)); @@ -147,6 +151,8 @@ Object Init(Env env, Object exports) { exports.Set("threadsafe_function", InitThreadSafeFunction(env)); exports.Set("typed_threadsafe_function_ctx", InitTypedThreadSafeFunctionCtx(env)); + exports.Set("typed_threadsafe_function_exception", + InitTypedThreadSafeFunctionException(env)); exports.Set("typed_threadsafe_function_existing_tsfn", InitTypedThreadSafeFunctionExistingTsfn(env)); exports.Set("typed_threadsafe_function_ptr", diff --git a/test/binding.gyp b/test/binding.gyp index 3b9b3c730..0dcfda61d 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -55,6 +55,7 @@ 'run_script.cc', 'symbol.cc', 'threadsafe_function/threadsafe_function_ctx.cc', + 'threadsafe_function/threadsafe_function_exception.cc', 'threadsafe_function/threadsafe_function_existing_tsfn.cc', 'threadsafe_function/threadsafe_function_ptr.cc', 'threadsafe_function/threadsafe_function_sum.cc', @@ -62,6 +63,7 @@ 'threadsafe_function/threadsafe_function.cc', 'type_taggable.cc', 'typed_threadsafe_function/typed_threadsafe_function_ctx.cc', + 'typed_threadsafe_function/typed_threadsafe_function_exception.cc', 'typed_threadsafe_function/typed_threadsafe_function_existing_tsfn.cc', 'typed_threadsafe_function/typed_threadsafe_function_ptr.cc', 'typed_threadsafe_function/typed_threadsafe_function_sum.cc', diff --git a/test/threadsafe_function/threadsafe_function_exception.cc b/test/threadsafe_function/threadsafe_function_exception.cc new file mode 100644 index 000000000..9ffe703ec --- /dev/null +++ b/test/threadsafe_function/threadsafe_function_exception.cc @@ -0,0 +1,50 @@ +#include +#include "napi.h" +#include "test_helper.h" + +#if (NAPI_VERSION > 3) + +using namespace Napi; + +namespace { + +void CallJS(napi_env env, napi_value /* callback */, void* /*data*/) { + Napi::Error error = Napi::Error::New(env, "test-from-native"); + NAPI_THROW_VOID(error); +} + +void TestCall(const CallbackInfo& info) { + Napi::Env env = info.Env(); + + ThreadSafeFunction wrapped = + ThreadSafeFunction::New(env, + info[0].As(), + Object::New(env), + String::New(env, "Test"), + 0, + 1); + wrapped.BlockingCall(static_cast(nullptr)); + wrapped.Release(); +} + +void TestCallWithNativeCallback(const CallbackInfo& info) { + Napi::Env env = info.Env(); + + ThreadSafeFunction wrapped = ThreadSafeFunction::New( + env, Napi::Function(), Object::New(env), String::New(env, "Test"), 0, 1); + wrapped.BlockingCall(static_cast(nullptr), CallJS); + wrapped.Release(); +} + +} // namespace + +Object InitThreadSafeFunctionException(Env env) { + Object exports = Object::New(env); + exports["testCall"] = Function::New(env, TestCall); + exports["testCallWithNativeCallback"] = + Function::New(env, TestCallWithNativeCallback); + + return exports; +} + +#endif diff --git a/test/threadsafe_function/threadsafe_function_exception.js b/test/threadsafe_function/threadsafe_function_exception.js new file mode 100644 index 000000000..c1590d272 --- /dev/null +++ b/test/threadsafe_function/threadsafe_function_exception.js @@ -0,0 +1,55 @@ +'use strict'; + +const assert = require('assert'); +const common = require('../common'); +const { spawnSync } = require('../napi_child'); + +function test (bindingPath) { + const { status } = spawnSync( + process.execPath, + [ + '--force-node-api-uncaught-exceptions-policy=true', + __filename, + 'child', + bindingPath + ], + { stdio: 'inherit' } + ); + + assert.strictEqual(status, 0); +} + +if (process.argv[2] === 'child') { + child(process.argv[3]) + .catch(err => { + process.exitCode = 1; + console.error(err); + }); +} else { + module.exports = common.runTestWithBindingPath(test); +} + +async function child (bindingPath) { + const binding = require(bindingPath); + const { testCall, testCallWithNativeCallback } = binding.threadsafe_function_exception; + + await new Promise(resolve => { + process.once('uncaughtException', common.mustCall(err => { + assert.strictEqual(err.message, 'test'); + resolve(); + }, 1)); + + testCall(common.mustCall(() => { + throw new Error('test'); + }, 1)); + }); + + await new Promise(resolve => { + process.once('uncaughtException', common.mustCall(err => { + assert.strictEqual(err.message, 'test-from-native'); + resolve(); + }, 1)); + + testCallWithNativeCallback(); + }); +} diff --git a/test/typed_threadsafe_function/typed_threadsafe_function_exception.cc b/test/typed_threadsafe_function/typed_threadsafe_function_exception.cc new file mode 100644 index 000000000..c55ca23a4 --- /dev/null +++ b/test/typed_threadsafe_function/typed_threadsafe_function_exception.cc @@ -0,0 +1,39 @@ +#include +#include "napi.h" +#include "test_helper.h" + +#if (NAPI_VERSION > 3) + +using namespace Napi; + +namespace { + +void CallJS(Napi::Env env, + Napi::Function /* callback */, + std::nullptr_t* /* context */, + void* /*data*/) { + Napi::Error error = Napi::Error::New(env, "test-from-native"); + NAPI_THROW_VOID(error); +} + +using TSFN = TypedThreadSafeFunction; + +void TestCall(const CallbackInfo& info) { + Napi::Env env = info.Env(); + + TSFN wrapped = TSFN::New( + env, Napi::Function(), Object::New(env), String::New(env, "Test"), 0, 1); + wrapped.BlockingCall(static_cast(nullptr)); + wrapped.Release(); +} + +} // namespace + +Object InitTypedThreadSafeFunctionException(Env env) { + Object exports = Object::New(env); + exports["testCall"] = Function::New(env, TestCall); + + return exports; +} + +#endif diff --git a/test/typed_threadsafe_function/typed_threadsafe_function_exception.js b/test/typed_threadsafe_function/typed_threadsafe_function_exception.js new file mode 100644 index 000000000..dc5457c56 --- /dev/null +++ b/test/typed_threadsafe_function/typed_threadsafe_function_exception.js @@ -0,0 +1,45 @@ +'use strict'; + +const assert = require('assert'); +const common = require('../common'); + +const { spawnSync } = require('../napi_child'); + +function test (bindingPath) { + const { status } = spawnSync( + process.execPath, + [ + '--force-node-api-uncaught-exceptions-policy=true', + __filename, + 'child', + bindingPath + ], + { stdio: 'inherit' } + ); + + assert.strictEqual(status, 0); +} + +if (process.argv[2] === 'child') { + child(process.argv[3]) + .catch(err => { + process.exitCode = 1; + console.error(err); + }); +} else { + module.exports = common.runTestWithBindingPath(test); +} + +async function child (bindingPath) { + const binding = require(bindingPath); + const { testCall } = binding.typed_threadsafe_function_exception; + + await new Promise(resolve => { + process.once('uncaughtException', common.mustCall(err => { + assert.strictEqual(err.message, 'test-from-native'); + resolve(); + }, 1)); + + testCall(); + }); +} From 740dca4e501b53dccf642c9db0004f9d2c82fca2 Mon Sep 17 00:00:00 2001 From: legendecas Date: Thu, 20 Jul 2023 00:51:43 +0800 Subject: [PATCH 2/2] fixup! fix: handle c++ exception in TSFN callback --- .../threadsafe_function_exception.js | 33 +++++++++++ .../typed_threadsafe_function_exception.js | 19 +++++++ test/common/index.js | 3 +- .../threadsafe_function_exception.js | 57 ++++--------------- .../typed_threadsafe_function_exception.js | 44 ++------------ 5 files changed, 71 insertions(+), 85 deletions(-) create mode 100644 test/child_processes/threadsafe_function_exception.js create mode 100644 test/child_processes/typed_threadsafe_function_exception.js diff --git a/test/child_processes/threadsafe_function_exception.js b/test/child_processes/threadsafe_function_exception.js new file mode 100644 index 000000000..4fc63d7c8 --- /dev/null +++ b/test/child_processes/threadsafe_function_exception.js @@ -0,0 +1,33 @@ +'use strict'; + +const assert = require('assert'); +const common = require('../common'); + +module.exports = { + testCall: async binding => { + const { testCall } = binding.threadsafe_function_exception; + + await new Promise(resolve => { + process.once('uncaughtException', common.mustCall(err => { + assert.strictEqual(err.message, 'test'); + resolve(); + }, 1)); + + testCall(common.mustCall(() => { + throw new Error('test'); + }, 1)); + }); + }, + testCallWithNativeCallback: async binding => { + const { testCallWithNativeCallback } = binding.threadsafe_function_exception; + + await new Promise(resolve => { + process.once('uncaughtException', common.mustCall(err => { + assert.strictEqual(err.message, 'test-from-native'); + resolve(); + }, 1)); + + testCallWithNativeCallback(); + }); + } +}; diff --git a/test/child_processes/typed_threadsafe_function_exception.js b/test/child_processes/typed_threadsafe_function_exception.js new file mode 100644 index 000000000..5cbfab268 --- /dev/null +++ b/test/child_processes/typed_threadsafe_function_exception.js @@ -0,0 +1,19 @@ +'use strict'; + +const assert = require('assert'); +const common = require('../common'); + +module.exports = { + testCall: async binding => { + const { testCall } = binding.typed_threadsafe_function_exception; + + await new Promise(resolve => { + process.once('uncaughtException', common.mustCall(err => { + assert.strictEqual(err.message, 'test-from-native'); + resolve(); + }, 1)); + + testCall(); + }); + } +}; diff --git a/test/common/index.js b/test/common/index.js index a82d49f50..6a842569f 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -202,7 +202,7 @@ exports.runTestWithBuildType = async function (test, buildType) { // in the main process. Two examples are addon and addon_data, both of which // use Napi::Env::SetInstanceData(). This helper function provides a common // approach for running such tests. -exports.runTestInChildProcess = function ({ suite, testName, expectedStderr }) { +exports.runTestInChildProcess = function ({ suite, testName, expectedStderr, execArgv }) { return exports.runTestWithBindingPath((bindingName) => { return new Promise((resolve) => { bindingName = escapeBackslashes(bindingName); @@ -210,6 +210,7 @@ exports.runTestInChildProcess = function ({ suite, testName, expectedStderr }) { const suitePath = escapeBackslashes(path.join(__dirname, '..', 'child_processes', suite)); const child = spawn(process.execPath, [ '--expose-gc', + ...(execArgv ?? []), '-e', `require('${suitePath}').${testName}(require('${bindingName}'))` ]); diff --git a/test/threadsafe_function/threadsafe_function_exception.js b/test/threadsafe_function/threadsafe_function_exception.js index c1590d272..688a53bcf 100644 --- a/test/threadsafe_function/threadsafe_function_exception.js +++ b/test/threadsafe_function/threadsafe_function_exception.js @@ -1,55 +1,20 @@ 'use strict'; -const assert = require('assert'); const common = require('../common'); -const { spawnSync } = require('../napi_child'); -function test (bindingPath) { - const { status } = spawnSync( - process.execPath, - [ - '--force-node-api-uncaught-exceptions-policy=true', - __filename, - 'child', - bindingPath - ], - { stdio: 'inherit' } - ); +module.exports = common.runTest(test); - assert.strictEqual(status, 0); -} - -if (process.argv[2] === 'child') { - child(process.argv[3]) - .catch(err => { - process.exitCode = 1; - console.error(err); - }); -} else { - module.exports = common.runTestWithBindingPath(test); -} - -async function child (bindingPath) { - const binding = require(bindingPath); - const { testCall, testCallWithNativeCallback } = binding.threadsafe_function_exception; - - await new Promise(resolve => { - process.once('uncaughtException', common.mustCall(err => { - assert.strictEqual(err.message, 'test'); - resolve(); - }, 1)); - - testCall(common.mustCall(() => { - throw new Error('test'); - }, 1)); +const execArgv = ['--force-node-api-uncaught-exceptions-policy=true']; +async function test () { + await common.runTestInChildProcess({ + suite: 'threadsafe_function_exception', + testName: 'testCall', + execArgv }); - await new Promise(resolve => { - process.once('uncaughtException', common.mustCall(err => { - assert.strictEqual(err.message, 'test-from-native'); - resolve(); - }, 1)); - - testCallWithNativeCallback(); + await common.runTestInChildProcess({ + suite: 'threadsafe_function_exception', + testName: 'testCallWithNativeCallback', + execArgv }); } diff --git a/test/typed_threadsafe_function/typed_threadsafe_function_exception.js b/test/typed_threadsafe_function/typed_threadsafe_function_exception.js index dc5457c56..60ff67363 100644 --- a/test/typed_threadsafe_function/typed_threadsafe_function_exception.js +++ b/test/typed_threadsafe_function/typed_threadsafe_function_exception.js @@ -1,45 +1,13 @@ 'use strict'; -const assert = require('assert'); const common = require('../common'); -const { spawnSync } = require('../napi_child'); +module.exports = common.runTest(test); -function test (bindingPath) { - const { status } = spawnSync( - process.execPath, - [ - '--force-node-api-uncaught-exceptions-policy=true', - __filename, - 'child', - bindingPath - ], - { stdio: 'inherit' } - ); - - assert.strictEqual(status, 0); -} - -if (process.argv[2] === 'child') { - child(process.argv[3]) - .catch(err => { - process.exitCode = 1; - console.error(err); - }); -} else { - module.exports = common.runTestWithBindingPath(test); -} - -async function child (bindingPath) { - const binding = require(bindingPath); - const { testCall } = binding.typed_threadsafe_function_exception; - - await new Promise(resolve => { - process.once('uncaughtException', common.mustCall(err => { - assert.strictEqual(err.message, 'test-from-native'); - resolve(); - }, 1)); - - testCall(); +async function test () { + await common.runTestInChildProcess({ + suite: 'typed_threadsafe_function_exception', + testName: 'testCall', + execArgv: ['--force-node-api-uncaught-exceptions-policy=true'] }); }