From 6a8d45720dc7fc6ed16e964867e5c5ba80da2456 Mon Sep 17 00:00:00 2001 From: Jimmy Miller Date: Tue, 1 Sep 2026 11:19:36 -0400 Subject: [PATCH] node-api: avoid TSFN mutex reentry during teardown ThreadSafeFunction releases its Node-API environment reference while finalizing its loop-owned resources. That operation can synchronously invoke an external finalizer, which may release a still-valid TSFN ownership. Do not hold the TSFN mutex across this reentrant path. Use an intermediate resource-cleanup state to retain the TSFN until resource cleanup completes, then permit the loop or final native owner to destroy it. Fixes: https://github.com/nodejs/node/issues/65100 Signed-off-by: Jimmy Miller Assisted-by: OpenCode --- src/node_api.cc | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index e0e7cca2a4ba..39f109489cfa 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -218,6 +218,7 @@ class ThreadSafeFunction { node::Utf8Value(env_->isolate, name).ToStringView()), thread_count(thread_count_), state(kOpen), + resources_released(false), dispatch_state(kDispatchIdle), context(context_), max_queue_size(max_queue_size_), @@ -320,17 +321,22 @@ class ThreadSafeFunction { } void MaybeDelete() { + bool delete_this; { node::Mutex::ScopedLock lock(this->mutex); - 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; - } + state = kResourceCleanup; + } + + ReleaseResources(); + + { + node::Mutex::ScopedLock lock(this->mutex); + state = kClosed; + delete_this = thread_count == 0; } + + if (!delete_this) return; + // Make sure to release lock before destroying delete this; } @@ -384,8 +390,8 @@ class ThreadSafeFunction { protected: void ReleaseResources() { - if (state != kClosed) { - state = kClosed; + if (!resources_released) { + resources_released = true; ref.Reset(); node::RemoveEnvironmentCleanupHook(env->isolate, Cleanup, this); env->Unref(); @@ -553,7 +559,7 @@ class ThreadSafeFunction { using node::AsyncResource::CallbackScope; }; - enum State : unsigned char { kOpen, kClosing, kClosed }; + enum State : unsigned char { kOpen, kClosing, kResourceCleanup, kClosed }; static const unsigned char kDispatchIdle = 0; static const unsigned char kDispatchRunning = 1 << 0; @@ -570,6 +576,8 @@ class ThreadSafeFunction { uv_async_t async; size_t thread_count; State state; + + bool resources_released; std::atomic_uchar dispatch_state; // These are variables set once, upon creation, and then never again, which