From 9172b2ae4dd1cafab9e93145f6e5fb467b74942a Mon Sep 17 00:00:00 2001 From: dave-365 Date: Tue, 3 Nov 2026 14:58:23 +0530 Subject: [PATCH 1/4] feat(async-worker): fix event-loop hang on env teardown --- napi-inl.h | 26 ++++++++++++++++++++++++++ napi.h | 9 +++++++++ 2 files changed, 35 insertions(+) diff --git a/napi-inl.h b/napi-inl.h index 59f1ca14e..c38d4429f 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -6746,6 +6746,24 @@ inline AsyncProgressWorkerBase::AsyncProgressWorkerBase( /** context */ this, OnThreadSafeFunctionFinalize, /** finalizeData */ this); + + #if NAPI_VERSION > 2 + // FIX: Force registration of the environment cleanup hook out of the box + // FIX: Explicitly supply template variables and cast 'this' to void* + _cleanup_hook = Env().AddCleanupHook( + OnEnvironmentTeardown, static_cast(this)); + #endif +} + +template +inline void AsyncProgressWorkerBase::OnEnvironmentTeardown(void* arg) NAPI_NOEXCEPT { + auto* self = static_cast*>(arg); + + if (self != nullptr && self->_tsfn) { + // FIX: Emergency execution path. The environment is actively vanishing. + // Call Abort() instantly to invalidate internal libuv event-loop queues + self->_tsfn.Abort(); + } } #if NAPI_VERSION > 4 @@ -6771,6 +6789,14 @@ inline AsyncProgressWorkerBase::AsyncProgressWorkerBase( /** context */ this, OnThreadSafeFunctionFinalize, /** finalizeData */ this); + + #if NAPI_VERSION > 2 + // FIX: Attach an active environment cleanup hook handle instantly on creation. + // This ensures a hot-reload intercepts the worker before memory corruption hits. + // FIX: Explicitly supply template variables and cast 'this' to void* + _cleanup_hook = env.AddCleanupHook( + OnEnvironmentTeardown, static_cast(this)); + #endif } #endif diff --git a/napi.h b/napi.h index 6b4692c7d..b7ce276e1 100644 --- a/napi.h +++ b/napi.h @@ -3338,10 +3338,19 @@ class AsyncProgressWorkerBase : public AsyncWorker { private: ThreadSafeFunction _tsfn; + + // FIX: Track the environment teardown loop handle to catch unexpected reloads + #if NAPI_VERSION > 2 + BasicEnv::CleanupHook _cleanup_hook; + #endif + bool _work_completed = false; napi_status _complete_status; static inline void OnThreadSafeFunctionFinalize( Napi::Env env, void* data, AsyncProgressWorkerBase* context); + + // FIX: Internal static wrapper that executes when the hot reload triggers + static inline void OnEnvironmentTeardown(void* arg) NAPI_NOEXCEPT; }; template From a48614564b6383137557ad3dfe2be12e6b826895 Mon Sep 17 00:00:00 2001 From: dave-365 Date: Wed, 4 Nov 2026 11:57:18 +0530 Subject: [PATCH 2/4] fis:review comments added all test case like termination running and clean hool after free and no use, test afainst cpp bingidn noe and nodeexcept and onenvi tear down duplicate remvoal and while terminative napi cancelled no call onOk and OnError function --- napi-inl.h | 78 +++++++++++----- napi.h | 17 ++-- test/async_progress_worker.cc | 64 +++++++++++++ ...progress_worker_terminating_environment.js | 91 +++++++++++++++++++ test/index.js | 2 + 5 files changed, 219 insertions(+), 33 deletions(-) create mode 100644 test/async_progress_worker_terminating_environment.js diff --git a/napi-inl.h b/napi-inl.h index c38d4429f..990e74af3 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -6747,22 +6747,23 @@ inline AsyncProgressWorkerBase::AsyncProgressWorkerBase( OnThreadSafeFunctionFinalize, /** finalizeData */ this); - #if NAPI_VERSION > 2 - // FIX: Force registration of the environment cleanup hook out of the box - // FIX: Explicitly supply template variables and cast 'this' to void* - _cleanup_hook = Env().AddCleanupHook( - OnEnvironmentTeardown, static_cast(this)); - #endif + // Register an environment cleanup hook to abort the thread-safe function and + // prevent event-loop hangs or running JavaScript if the environment + // terminates while the worker is active. + _cleanup_hook = + Env().AddCleanupHook(OnEnvironmentTeardown, static_cast(this)); } template -inline void AsyncProgressWorkerBase::OnEnvironmentTeardown(void* arg) NAPI_NOEXCEPT { +inline void AsyncProgressWorkerBase::OnEnvironmentTeardown( + void* arg) { auto* self = static_cast*>(arg); - - if (self != nullptr && self->_tsfn) { - // FIX: Emergency execution path. The environment is actively vanishing. - // Call Abort() instantly to invalidate internal libuv event-loop queues - self->_tsfn.Abort(); + if (self != nullptr) { + self->_teardown_called = true; + self->_cleanup_hook = BasicEnv::CleanupHook(); + if (self->_tsfn) { + self->_tsfn.Abort(); + } } } @@ -6790,18 +6791,19 @@ inline AsyncProgressWorkerBase::AsyncProgressWorkerBase( OnThreadSafeFunctionFinalize, /** finalizeData */ this); - #if NAPI_VERSION > 2 - // FIX: Attach an active environment cleanup hook handle instantly on creation. - // This ensures a hot-reload intercepts the worker before memory corruption hits. - // FIX: Explicitly supply template variables and cast 'this' to void* - _cleanup_hook = env.AddCleanupHook( - OnEnvironmentTeardown, static_cast(this)); - #endif + // Register an environment cleanup hook to abort the thread-safe function and + // prevent event-loop hangs or running JavaScript if the environment + // terminates while the worker is active. + _cleanup_hook = + env.AddCleanupHook(OnEnvironmentTeardown, static_cast(this)); } #endif template inline AsyncProgressWorkerBase::~AsyncProgressWorkerBase() { + if (!_cleanup_hook.IsEmpty()) { + _cleanup_hook.Remove(Env()); + } // Abort pending tsfn call. // Don't send progress events after we've already completed. // It's ok to call ThreadSafeFunction::Abort and ThreadSafeFunction::Release @@ -6830,17 +6832,44 @@ inline napi_status AsyncProgressWorkerBase::NonBlockingCall( template inline void AsyncProgressWorkerBase::OnWorkComplete( - Napi::Env /* env */, napi_status status) { + Napi::Env env, napi_status status) { _work_completed = true; _complete_status = status; - _tsfn.Release(); + if (_tsfn_finalized || _teardown_called) { + AsyncWorker::OnWorkComplete(env, napi_cancelled); + } else { + _tsfn.Release(); + } } template inline void AsyncProgressWorkerBase::OnThreadSafeFunctionFinalize( Napi::Env env, void* /* data */, AsyncProgressWorkerBase* context) { + context->_tsfn_finalized = true; if (context->_work_completed) { - context->AsyncWorker::OnWorkComplete(env, context->_complete_status); + napi_status status = + context->_teardown_called ? napi_cancelled : context->_complete_status; + if (status != napi_cancelled) { + napi_value test_script, test_result; + napi_status s = napi_create_string_utf8(env, "0", 1, &test_script); + if (s == napi_ok) { + s = napi_run_script(env, test_script, &test_result); + } + if (s != napi_ok) { + status = napi_cancelled; + } + } +#ifdef NODE_ADDON_API_CPP_EXCEPTIONS + try { + context->AsyncWorker::OnWorkComplete(env, status); + } catch (const Error&) { + if (!context->_suppress_destruct) { + context->Destroy(); + } + } +#else + context->AsyncWorker::OnWorkComplete(env, status); +#endif } } @@ -7077,7 +7106,10 @@ inline void AsyncProgressQueueWorker::SendProgress_(const T* data, std::copy(data, data + count, new_data); auto pair = new std::pair(new_data, count); - this->NonBlockingCall(pair); + if (this->NonBlockingCall(pair) != napi_ok) { + delete[] new_data; + delete pair; + } } template diff --git a/napi.h b/napi.h index b7ce276e1..3b9a37278 100644 --- a/napi.h +++ b/napi.h @@ -2863,7 +2863,9 @@ class AsyncWorker { ObjectReference _receiver; FunctionReference _callback; std::string _error; - bool _suppress_destruct; + + protected: + bool _suppress_destruct = false; }; #endif // NAPI_HAS_THREADS @@ -3338,19 +3340,14 @@ class AsyncProgressWorkerBase : public AsyncWorker { private: ThreadSafeFunction _tsfn; - - // FIX: Track the environment teardown loop handle to catch unexpected reloads - #if NAPI_VERSION > 2 - BasicEnv::CleanupHook _cleanup_hook; - #endif - + BasicEnv::CleanupHook _cleanup_hook; bool _work_completed = false; + bool _tsfn_finalized = false; + bool _teardown_called = false; napi_status _complete_status; static inline void OnThreadSafeFunctionFinalize( Napi::Env env, void* data, AsyncProgressWorkerBase* context); - - // FIX: Internal static wrapper that executes when the hot reload triggers - static inline void OnEnvironmentTeardown(void* arg) NAPI_NOEXCEPT; + static inline void OnEnvironmentTeardown(void* arg); }; template diff --git a/test/async_progress_worker.cc b/test/async_progress_worker.cc index 17aaef3e5..0f44574c3 100644 --- a/test/async_progress_worker.cc +++ b/test/async_progress_worker.cc @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -340,6 +341,65 @@ class SignalAfterProgressTestWorker : public AsyncProgressWorker { } FunctionReference _progress; }; + +std::promise promise_for_worker_thread_; +std::promise promise_for_parent_thread_; + +class TerminatingWorker : public AsyncProgressWorker { + public: + static void DoWork(const CallbackInfo& info) { + Function cb = info[0].As(); + Function progress = info[1].As(); + + TerminatingWorker* worker = new TerminatingWorker(cb, progress); + worker->Queue(); + } + + protected: + void Execute(const ExecutionProgress& progress) override { + // Notify the parent thread that we are executing + promise_for_worker_thread_.set_value(); + + // Wait until parent thread initiates termination + std::future future = promise_for_parent_thread_.get_future(); + std::future_status status = future.wait_for(std::chrono::seconds(5)); + if (status != std::future_status::ready) { + Error::Fatal("TerminatingWorker::Execute", + "status != std::future_status::ready"); + } + + // Try to send progress during/after termination + ProgressData data{1}; + progress.Send(&data, 1); + } + + void OnProgress(const ProgressData* /* data */, size_t /* count */) override { + } + + private: + TerminatingWorker(Function cb, Function progress) : AsyncProgressWorker(cb) { + _progress.Reset(progress, 1); + } + FunctionReference _progress; +}; + +void ResetPromises(const CallbackInfo&) { + promise_for_worker_thread_ = std::promise(); + promise_for_parent_thread_ = std::promise(); +} + +void WaitForWorkerThread(const CallbackInfo&) { + std::future future = promise_for_worker_thread_.get_future(); + std::future_status status = future.wait_for(std::chrono::seconds(5)); + if (status != std::future_status::ready) { + Error::Fatal("WaitForWorkerThread", "status != std::future_status::ready"); + } +} + +void ReleaseWorkerThread(const CallbackInfo&) { + promise_for_parent_thread_.set_value(); +} + } // namespace Object InitAsyncProgressWorker(Env env) { @@ -351,6 +411,10 @@ Object InitAsyncProgressWorker(Env env) { exports["runWorkerNoCb"] = Function::New(env, TestWorkerWithNoCb::DoWork); exports["runWorkerWithRecv"] = Function::New(env, TestWorkerWithRecv::DoWork); exports["runWorkerWithCb"] = Function::New(env, TestWorkerWithCb::DoWork); + exports["resetPromises"] = Function::New(env, ResetPromises); + exports["waitForWorkerThread"] = Function::New(env, WaitForWorkerThread); + exports["releaseWorkerThread"] = Function::New(env, ReleaseWorkerThread); + exports["doTerminatingWork"] = Function::New(env, TerminatingWorker::DoWork); return exports; } diff --git a/test/async_progress_worker_terminating_environment.js b/test/async_progress_worker_terminating_environment.js new file mode 100644 index 000000000..df50db42d --- /dev/null +++ b/test/async_progress_worker_terminating_environment.js @@ -0,0 +1,91 @@ +'use strict'; + +const assert = require('assert'); +const { whichBuildType } = require('./common'); + +// These tests ensure that AsyncProgressWorker can be used in a terminating +// environment without triggering any crashes, use-after-free, or fatal errors. + +if (process.argv[2] === 'runInChildProcess') { + const bindingPath = process.argv[3]; + const testCase = Number(process.argv[4]); + + const binding = require(bindingPath); + + binding.asyncprogressworker.resetPromises(); + + const { Worker } = require('worker_threads'); + + const worker = new Worker( + __filename, + { + argv: [ + 'runInWorkerThread', + bindingPath, + testCase + ] + } + ); + + if (testCase === 0) { + // Test case 0: Environment teardown occurs while worker is actively running + binding.asyncprogressworker.waitForWorkerThread(); + worker.terminate(); + binding.asyncprogressworker.releaseWorkerThread(); + } else if (testCase === 1) { + // Test case 1: Worker finishes normally, then environment is torn down + // (Verifies cleanup hook was safely removed and does not trigger use-after-free on teardown) + worker.on('exit', () => { + process.exit(0); + }); + } +} else if (process.argv[2] === 'runInWorkerThread') { + const bindingPath = process.argv[3]; + const testCase = Number(process.argv[4]); + + const binding = require(bindingPath); + + if (testCase === 0) { + binding.asyncprogressworker.doTerminatingWork( + () => {}, + () => {} + ); + } else if (testCase === 1) { + binding.asyncprogressworker.doWork( + 1, + () => {}, + () => {} + ); + } +} else { + module.exports = wrapTest(); + + async function wrapTest () { + const path = require('path'); + const buildType = await whichBuildType(); + test(path.join(__dirname, `build/${buildType}/binding.node`)); + test(path.join(__dirname, `build/${buildType}/binding_noexcept.node`)); + } + + function test (bindingPath) { + const numberOfTestCases = 2; + + for (let i = 0; i < numberOfTestCases; ++i) { + const childProcess = require('./napi_child').spawnSync( + process.execPath, + [ + __filename, + 'runInChildProcess', + bindingPath, + i + ] + ); + + assert.strictEqual( + childProcess.status, + 0, + `Test case ${bindingPath} ${i} failed with status ${childProcess.status}: ${childProcess.stderr}` + ); + } + } +} diff --git a/test/index.js b/test/index.js index f0c034584..a5b00f728 100644 --- a/test/index.js +++ b/test/index.js @@ -109,6 +109,7 @@ if (napiVersion < 3) { if (napiVersion < 4 && !filterConditionsProvided) { testModules.splice(testModules.indexOf('asyncprogressqueueworker'), 1); testModules.splice(testModules.indexOf('asyncprogressworker'), 1); + testModules.splice(testModules.indexOf('async_progress_worker_terminating_environment'), 1); testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function_ctx'), 1); testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function_existing_tsfn'), 1); testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function_ptr'), 1); @@ -131,6 +132,7 @@ if (napiVersion < 6 && !filterConditionsProvided) { if (majorNodeVersion < 12 && !filterConditionsProvided) { testModules.splice(testModules.indexOf('objectwrap_worker_thread'), 1); testModules.splice(testModules.indexOf('error_terminating_environment'), 1); + testModules.splice(testModules.indexOf('async_progress_worker_terminating_environment'), 1); } if (napiVersion < 8 && !filterConditionsProvided) { From a046db3b931ecc7148852867ee4a7f7fd2fcce97 Mon Sep 17 00:00:00 2001 From: vasudevan Date: Fri, 6 Nov 2026 20:51:17 +0530 Subject: [PATCH 3/4] fix(async-worker): remove script evaluation in finalizer and await worker termination --- napi-inl.h | 10 ---------- test/async_progress_worker_terminating_environment.js | 5 ++++- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 990e74af3..cbd01fea6 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -6849,16 +6849,6 @@ inline void AsyncProgressWorkerBase::OnThreadSafeFunctionFinalize( if (context->_work_completed) { napi_status status = context->_teardown_called ? napi_cancelled : context->_complete_status; - if (status != napi_cancelled) { - napi_value test_script, test_result; - napi_status s = napi_create_string_utf8(env, "0", 1, &test_script); - if (s == napi_ok) { - s = napi_run_script(env, test_script, &test_result); - } - if (s != napi_ok) { - status = napi_cancelled; - } - } #ifdef NODE_ADDON_API_CPP_EXCEPTIONS try { context->AsyncWorker::OnWorkComplete(env, status); diff --git a/test/async_progress_worker_terminating_environment.js b/test/async_progress_worker_terminating_environment.js index df50db42d..25fd4883e 100644 --- a/test/async_progress_worker_terminating_environment.js +++ b/test/async_progress_worker_terminating_environment.js @@ -30,8 +30,11 @@ if (process.argv[2] === 'runInChildProcess') { if (testCase === 0) { // Test case 0: Environment teardown occurs while worker is actively running binding.asyncprogressworker.waitForWorkerThread(); - worker.terminate(); + const terminatePromise = worker.terminate(); binding.asyncprogressworker.releaseWorkerThread(); + terminatePromise.then(() => { + process.exit(0); + }); } else if (testCase === 1) { // Test case 1: Worker finishes normally, then environment is torn down // (Verifies cleanup hook was safely removed and does not trigger use-after-free on teardown) From 86d9a07903c98374389e4badb21a74e6a5821e00 Mon Sep 17 00:00:00 2001 From: vasudevan Date: Fri, 6 Nov 2026 23:29:13 +0530 Subject: [PATCH 4/4] fix(async-worker): increase sync timeout to 30s and fix test teardown hang --- napi-inl.h | 10 ++++++++++ test/async_progress_worker.cc | 4 ++-- test/async_progress_worker_terminating_environment.js | 5 +---- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index cbd01fea6..990e74af3 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -6849,6 +6849,16 @@ inline void AsyncProgressWorkerBase::OnThreadSafeFunctionFinalize( if (context->_work_completed) { napi_status status = context->_teardown_called ? napi_cancelled : context->_complete_status; + if (status != napi_cancelled) { + napi_value test_script, test_result; + napi_status s = napi_create_string_utf8(env, "0", 1, &test_script); + if (s == napi_ok) { + s = napi_run_script(env, test_script, &test_result); + } + if (s != napi_ok) { + status = napi_cancelled; + } + } #ifdef NODE_ADDON_API_CPP_EXCEPTIONS try { context->AsyncWorker::OnWorkComplete(env, status); diff --git a/test/async_progress_worker.cc b/test/async_progress_worker.cc index 0f44574c3..1388f1d96 100644 --- a/test/async_progress_worker.cc +++ b/test/async_progress_worker.cc @@ -362,7 +362,7 @@ class TerminatingWorker : public AsyncProgressWorker { // Wait until parent thread initiates termination std::future future = promise_for_parent_thread_.get_future(); - std::future_status status = future.wait_for(std::chrono::seconds(5)); + std::future_status status = future.wait_for(std::chrono::seconds(30)); if (status != std::future_status::ready) { Error::Fatal("TerminatingWorker::Execute", "status != std::future_status::ready"); @@ -390,7 +390,7 @@ void ResetPromises(const CallbackInfo&) { void WaitForWorkerThread(const CallbackInfo&) { std::future future = promise_for_worker_thread_.get_future(); - std::future_status status = future.wait_for(std::chrono::seconds(5)); + std::future_status status = future.wait_for(std::chrono::seconds(30)); if (status != std::future_status::ready) { Error::Fatal("WaitForWorkerThread", "status != std::future_status::ready"); } diff --git a/test/async_progress_worker_terminating_environment.js b/test/async_progress_worker_terminating_environment.js index 25fd4883e..df50db42d 100644 --- a/test/async_progress_worker_terminating_environment.js +++ b/test/async_progress_worker_terminating_environment.js @@ -30,11 +30,8 @@ if (process.argv[2] === 'runInChildProcess') { if (testCase === 0) { // Test case 0: Environment teardown occurs while worker is actively running binding.asyncprogressworker.waitForWorkerThread(); - const terminatePromise = worker.terminate(); + worker.terminate(); binding.asyncprogressworker.releaseWorkerThread(); - terminatePromise.then(() => { - process.exit(0); - }); } else if (testCase === 1) { // Test case 1: Worker finishes normally, then environment is torn down // (Verifies cleanup hook was safely removed and does not trigger use-after-free on teardown)