From 0c5dd214536314c8448dd878246867819ef75193 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 18 Jun 2026 17:38:03 +0200 Subject: [PATCH] src: keep global list of addon-provided cleanup hooks A recent change, 215027c8eded2e, introduced flakiness into our test suite that exposed an issue with the cleanup hook API design. Specifically, the signatures of `AddEnvironmentCleanupHook()` and `RemoveEnvironmentCleanupHook()` are problematic. Both functions take `Isolate*` arguments, as addons are not generally expected to have to care about the Node.js `Environment` as a first-class scope provider. However, this model made the incorrect assumption that in the situations in which `RemoveEnvironmentCleanupHook()` would be invoked an `Environment` would always be associated with the current `Isolate` (via the current V8 `Context`, if there is one). This occasionally breaks down when `RemoveEnvironmentCleanupHook()` is called during garbage collection -- which would be an expected use case of the functionality, but one that has not been covered through our tests before 215027c8eded2e. Since Node.js guarantees API and ABI stability within a major version, and this is a bug that is independent from the aforementioned change, this commit resolves it by adding global mutable state to keep track off cleanup hooks registered through the Node.js public API. Obviously, this solution does not represent a desirable long-term state, and a semver-minor follow up should add an API that does not require modifications to these data structures, likely based on the async cleanup hook API which already solves this issue properly. Refs: https://github.com/nodejs/node/pull/63642 Fixes: https://github.com/nodejs/node/issues/63923 Signed-off-by: Anna Henningsen --- src/api/hooks.cc | 59 +++++++++++++++++++++++++++-- test/addons/worker-addon/binding.cc | 19 +++++++++- 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 0594dca02cd60a..09a79fb9da81f0 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -115,20 +115,71 @@ struct ACHHandle final { // this. void DeleteACHHandle::operator ()(ACHHandle* handle) const { delete handle; } +// TODO(addaleax): Having this extra set of data structures is far from +// ideal, but unfortunately the public synchronous cleanup hook API was +// slightly mis-designed; in particular, RemoveEnvironmentCleanupHook() needs +// to keep working when the Isolate either has no active context (such as +// during GC) or that context is associated with another Node.js Environment. +// We should align this with the asynchronous API, which handles this properly +// through an explicit reference to the cleanup hook instead of requiring +// lookups in internal maps. +struct CleanupHookThunk final { + Isolate* isolate; + Environment* env; + CleanupHook fun; + void* arg; + + bool operator==(const CleanupHookThunk& other) const { + // `env` is intentionally not part of this comparison + return isolate == other.isolate && fun == other.fun && arg == other.arg; + } +}; +struct CleanupHookThunkHash { + size_t operator()(const CleanupHookThunk& thunk) const { + return std::hash()(thunk.arg); + } +}; +using CleanupHookRegistry = + std::unordered_set; +static ExclusiveAccess cleanup_hook_registry; + +static void CleanupHookThunkRun(void* arg) { + const CleanupHookThunk* thunk = static_cast(arg); + thunk->fun(thunk->arg); + RemoveEnvironmentCleanupHook(thunk->isolate, thunk->fun, thunk->arg); +} + void AddEnvironmentCleanupHook(Isolate* isolate, CleanupHook fun, void* arg) { Environment* env = Environment::GetCurrent(isolate); CHECK_NOT_NULL(env); - env->AddCleanupHook(fun, arg); + void* wrapped_arg; + { + ExclusiveAccess::Scoped registry( + &cleanup_hook_registry); + auto result = registry->insert({isolate, env, fun, arg}); + CHECK(result.second); + wrapped_arg = const_cast(&*result.first); + } + env->AddCleanupHook(CleanupHookThunkRun, wrapped_arg); } void RemoveEnvironmentCleanupHook(Isolate* isolate, CleanupHook fun, void* arg) { - Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); - env->RemoveCleanupHook(fun, arg); + CleanupHookThunk thunk; + void* wrapped_arg; + { + ExclusiveAccess::Scoped registry( + &cleanup_hook_registry); + auto result = registry->find({isolate, nullptr, fun, arg}); + if (result == registry->end()) return; + wrapped_arg = const_cast(&*result); + thunk = *result; + registry->erase(result); + } + thunk.env->RemoveCleanupHook(CleanupHookThunkRun, wrapped_arg); } static void FinishAsyncCleanupHook(void* arg) { diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc index f7b08b659c94c1..ce13eadb66d4c4 100644 --- a/test/addons/worker-addon/binding.cc +++ b/test/addons/worker-addon/binding.cc @@ -54,8 +54,23 @@ void Initialize(Local exports, Isolate* isolate = Isolate::GetCurrent(); node::AddEnvironmentCleanupHook( isolate, Cleanup, const_cast(static_cast("cleanup"))); - node::AddEnvironmentCleanupHook(isolate, Dummy, nullptr); - node::RemoveEnvironmentCleanupHook(isolate, Dummy, nullptr); + + // Test that adding and removing a cleanup hook works as expected + { + node::AddEnvironmentCleanupHook(isolate, Dummy, nullptr); + node::RemoveEnvironmentCleanupHook(isolate, Dummy, nullptr); + } + + // Test that adding and removing a cleanup hook also works if there + // is no active context during removal + { + node::AddEnvironmentCleanupHook(isolate, Dummy, nullptr); + { + context->Exit(); + node::RemoveEnvironmentCleanupHook(isolate, Dummy, nullptr); + context->Enter(); + } + } if (getenv("addExtraItemToEventLoop") != nullptr) { // Add an item to the event loop that we do not clean up in order to make