diff --git a/src/node_api.cc b/src/node_api.cc index e0e7cca2a4ba..543c1bdb1a05 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -319,15 +319,22 @@ class ThreadSafeFunction { } } + // Runs on JS thread. void MaybeDelete() { + CHECK_EQ(state, kClosing); + // Release the resources like napi_env reference and maybe call into + // user code. `state` must be `kClosing` when calling into user here + // to avoid the `delete` below racing with re-entrant finalization. + ReleaseResources(); + { node::Mutex::ScopedLock lock(this->mutex); + // Mark the TSFN as ready to be deleted. + state = kClosed; if (thread_count > 0) { // At this point this TSFN is effectively done, but we need to keep // it alive for other threads that still have pointers to it until // they release them. - // But we already release all the resources that we can at this point - ReleaseResources(); return; } } @@ -383,9 +390,10 @@ class ThreadSafeFunction { inline void* Context() { return context; } protected: + // This calls into user code via `env->Unref()`, which may trigger finalizers, + // and calls back into `napi_release_threadsafe_function`. void ReleaseResources() { if (state != kClosed) { - state = kClosed; ref.Reset(); node::RemoveEnvironmentCleanupHook(env->isolate, Cleanup, this); env->Unref(); diff --git a/test/node-api/test_threadsafe_function_shutdown/binding.gyp b/test/node-api/test_threadsafe_function_shutdown/binding.gyp index eb08b447a94a..0cc8c234132c 100644 --- a/test/node-api/test_threadsafe_function_shutdown/binding.gyp +++ b/test/node-api/test_threadsafe_function_shutdown/binding.gyp @@ -1,11 +1,19 @@ { "targets": [ { - "target_name": "binding", - "sources": ["binding.cc"], + "target_name": "concurrent_calls", + "sources": ["concurrent_calls.cc"], "cflags_cc": ["--std=c++20"], 'cflags!': [ '-fno-exceptions', '-fno-rtti' ], 'cflags_cc!': [ '-fno-exceptions', '-fno-rtti' ], + }, + { + "target_name": "reentrant_release", + "sources": ["reentrant_release.c"] + }, + { + "target_name": "multi_thread_count_release", + "sources": ["multi_thread_count_release.c"] } ] } diff --git a/test/node-api/test_threadsafe_function_shutdown/binding.cc b/test/node-api/test_threadsafe_function_shutdown/concurrent_calls.cc similarity index 100% rename from test/node-api/test_threadsafe_function_shutdown/binding.cc rename to test/node-api/test_threadsafe_function_shutdown/concurrent_calls.cc diff --git a/test/node-api/test_threadsafe_function_shutdown/multi_thread_count_release.c b/test/node-api/test_threadsafe_function_shutdown/multi_thread_count_release.c new file mode 100644 index 000000000000..71fef4303bdd --- /dev/null +++ b/test/node-api/test_threadsafe_function_shutdown/multi_thread_count_release.c @@ -0,0 +1,69 @@ +#include +#include "../../js-native-api/common.h" + +#define THREAD_COUNT 3 + +static void CallJs(napi_env env, napi_value cb, void* context, void* data) { + NODE_API_BASIC_ASSERT_RETURN_VOID(false, "The queue stays empty"); +} + +static void Finalize(napi_env env, void* data, void* hint) { + napi_ref callback_ref = data; + napi_value callback; + napi_value undefined; + NODE_API_CALL_RETURN_VOID( + env, napi_get_reference_value(env, callback_ref, &callback)); + NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, callback_ref)); + NODE_API_CALL_RETURN_VOID(env, napi_get_undefined(env, &undefined)); + NODE_API_CALL_RETURN_VOID( + env, napi_call_function(env, undefined, callback, 0, NULL, NULL)); +} + +static napi_value Run(napi_env env, napi_callback_info info) { + size_t argc = 2; + napi_value args[2]; + NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL)); + bool abort; + napi_ref callback_ref; + napi_threadsafe_function tsfn; + NODE_API_CALL(env, napi_get_value_bool(env, args[0], &abort)); + NODE_API_CALL(env, napi_create_reference(env, args[1], 1, &callback_ref)); + napi_value name; + NODE_API_CALL(env, + napi_create_string_utf8( + env, "tsfn_thread_count", NAPI_AUTO_LENGTH, &name)); + NODE_API_CALL(env, + napi_create_threadsafe_function(env, + NULL, + NULL, + name, + 0, + THREAD_COUNT, + callback_ref, + Finalize, + NULL, + CallJs, + &tsfn)); + if (abort) { + NODE_API_CALL(env, napi_release_threadsafe_function(tsfn, napi_tsfn_abort)); + for (int i = 1; i < THREAD_COUNT; i++) { + napi_status status = + napi_call_threadsafe_function(tsfn, NULL, napi_tsfn_nonblocking); + NODE_API_ASSERT( + env, status == napi_closing, "Call after abort returns napi_closing"); + } + } else { + for (int i = 0; i < THREAD_COUNT; i++) { + NODE_API_CALL(env, + napi_release_threadsafe_function(tsfn, napi_tsfn_release)); + } + } + return NULL; +} + +NAPI_MODULE_INIT() { + napi_value run; + NODE_API_CALL( + env, napi_create_function(env, "run", NAPI_AUTO_LENGTH, Run, NULL, &run)); + return run; +} diff --git a/test/node-api/test_threadsafe_function_shutdown/reentrant_release.c b/test/node-api/test_threadsafe_function_shutdown/reentrant_release.c new file mode 100644 index 000000000000..9a47bc55d514 --- /dev/null +++ b/test/node-api/test_threadsafe_function_shutdown/reentrant_release.c @@ -0,0 +1,59 @@ +#include +#include +#include "../../js-native-api/common.h" + +typedef struct { + napi_threadsafe_function tsfn; +} Holder; + +static void CallJs(napi_env env, napi_value js_cb, void* context, void* data) { + NODE_API_BASIC_ASSERT_RETURN_VOID(false, "The queue stays empty"); +} + +// Runs while the napi_env tears down, reached from the thread-safe function +// dropping its own napi_env reference. Releasing the thread-safe function +// from here reenters TSFN finalization. +static void FinalizeHolder(napi_env env, void* data, void* hint) { + Holder* holder = data; + napi_status status = + napi_release_threadsafe_function(holder->tsfn, napi_tsfn_abort); + if (status != napi_ok) { + abort(); + } + free(holder); +} + +NAPI_MODULE_INIT() { + napi_value name; + napi_value external; + Holder* holder = malloc(sizeof(*holder)); + + NODE_API_CALL( + env, + napi_create_string_utf8(env, "tsfn_teardown", NAPI_AUTO_LENGTH, &name)); + + // The initial thread count is never released, so the thread-safe function is + // still ref-ed by another thread when the environment tears down. + NODE_API_CALL(env, + napi_create_threadsafe_function(env, + NULL, + NULL, + name, + 0, + 1, + NULL, + NULL, + NULL, + CallJs, + &holder->tsfn)); + // Allow the worker uv_loop to exit. + NODE_API_CALL(env, napi_unref_threadsafe_function(env, holder->tsfn)); + + // Held by the module exports, which the module cache keeps alive, so the + // napi_external finalizer runs during napi_env teardown. + NODE_API_CALL( + env, napi_create_external(env, holder, FinalizeHolder, NULL, &external)); + NODE_API_CALL(env, napi_set_named_property(env, exports, "holder", external)); + + return exports; +} diff --git a/test/node-api/test_threadsafe_function_shutdown/test.js b/test/node-api/test_threadsafe_function_shutdown/test-concurrent-calls.js similarity index 84% rename from test/node-api/test_threadsafe_function_shutdown/test.js rename to test/node-api/test_threadsafe_function_shutdown/test-concurrent-calls.js index 9b2587b5bbdf..154aceea9919 100644 --- a/test/node-api/test_threadsafe_function_shutdown/test.js +++ b/test/node-api/test_threadsafe_function_shutdown/test-concurrent-calls.js @@ -4,7 +4,7 @@ const common = require('../../common'); const process = require('process'); const assert = require('assert'); const { fork } = require('child_process'); -const binding = require(`./build/${common.buildType}/binding`); +const binding = require(`./build/${common.buildType}/concurrent_calls`); if (process.argv[2] === 'child') { binding(); diff --git a/test/node-api/test_threadsafe_function_shutdown/test-multi-thread-count-release.js b/test/node-api/test_threadsafe_function_shutdown/test-multi-thread-count-release.js new file mode 100644 index 000000000000..3c309884adbe --- /dev/null +++ b/test/node-api/test_threadsafe_function_shutdown/test-multi-thread-count-release.js @@ -0,0 +1,11 @@ +'use strict'; + +const common = require('../../common'); +const run = require(`./build/${common.buildType}/multi_thread_count_release`); + +// Ensures that the finalizer is called exactly once regardless of initial thread count +// of a TSFN. +// Also this verifies that multiple thread count does not cause re-entrant finalization. + +run(false, common.mustCall()); // Three releases. +run(true, common.mustCall()); // One abort, then two calls returning napi_closing. diff --git a/test/node-api/test_threadsafe_function_shutdown/test-reentrant-release.js b/test/node-api/test_threadsafe_function_shutdown/test-reentrant-release.js new file mode 100644 index 000000000000..1752135f2859 --- /dev/null +++ b/test/node-api/test_threadsafe_function_shutdown/test-reentrant-release.js @@ -0,0 +1,20 @@ +'use strict'; + +const common = require('../../common'); +const assert = require('assert'); +const { Worker, isMainThread } = require('worker_threads'); + +// A worker that exits while a native addon still owns a thread-safe function. +// The TSFN finalizer could trigger napi_env finalization and the addon +// may re-enter the TSFN finalization. +// Refs: https://github.com/nodejs/node/issues/65100 + +if (isMainThread) { + const worker = new Worker(__filename); + worker.on('error', common.mustNotCall()); + worker.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 0); + })); +} else { + require(`./build/${common.buildType}/reentrant_release`); +}