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_teardown/binding.c b/test/node-api/test_threadsafe_function_teardown/binding.c new file mode 100644 index 000000000000..2f7fd9a51059 --- /dev/null +++ b/test/node-api/test_threadsafe_function_teardown/binding.c @@ -0,0 +1,57 @@ +#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) {} + +// 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_teardown/binding.gyp b/test/node-api/test_threadsafe_function_teardown/binding.gyp new file mode 100644 index 000000000000..dab75f9ec6d6 --- /dev/null +++ b/test/node-api/test_threadsafe_function_teardown/binding.gyp @@ -0,0 +1,8 @@ +{ + "targets": [ + { + "target_name": "binding", + "sources": ["binding.c"] + } + ] +} diff --git a/test/node-api/test_threadsafe_function_teardown/test.js b/test/node-api/test_threadsafe_function_teardown/test.js new file mode 100644 index 000000000000..15c2a291a3e9 --- /dev/null +++ b/test/node-api/test_threadsafe_function_teardown/test.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}/binding`); +}