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