From fece197977b9e25b030d2a0d8c3a581988f09343 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 23 Sep 2026 12:14:17 +0000 Subject: [PATCH 1/8] src: track handle cleanup per thread, not per IsolateData The FreeEnvironment() fix for sibling Environments keeps the depth of nested Environment::CleanupHandles() calls on the IsolateData, so that InternalCallbackScope can re-allow JavaScript for sibling Environments while one of them is being freed. Environments that each have their own IsolateData on the same isolate and loop never see that counter and still fail with "illegal access". Environments that share a loop share a thread, so keep the depth in a thread_local instead. Refs: https://github.com/nodejs/node/pull/65977 Signed-off-by: Shelley Vohr --- src/api/callback.cc | 3 +-- src/env.cc | 6 ++++-- src/env.h | 5 ----- src/node_internals.h | 5 +++++ 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/api/callback.cc b/src/api/callback.cc index c3850fa4afef..ef0c754ce562 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -96,8 +96,7 @@ InternalCallbackScope::InternalCallbackScope( } Isolate* isolate = env->isolate(); - // See IsolateData::handle_cleanup_depth. - if (env->isolate_data()->handle_cleanup_depth > 0) allow_js_.emplace(isolate); + if (handle_cleanup_depth > 0) allow_js_.emplace(isolate); HandleScope handle_scope(isolate); Local current_context = isolate->GetCurrentContext(); diff --git a/src/env.cc b/src/env.cc index 24cd7b62e4f0..43aec44c7d55 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1436,6 +1436,8 @@ void Environment::ClosePerEnvHandles() { close_and_finish(reinterpret_cast(&task_queues_async_)); } +thread_local int handle_cleanup_depth = 0; + void Environment::CleanupHandles() { { Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_); @@ -1453,8 +1455,8 @@ void Environment::CleanupHandles() { for (HandleWrap* handle : handle_wrap_queue_) handle->Close(); - isolate_data()->handle_cleanup_depth++; - auto done = OnScopeLeave([&]() { isolate_data()->handle_cleanup_depth--; }); + handle_cleanup_depth++; + auto done = OnScopeLeave([]() { handle_cleanup_depth--; }); while (handle_cleanup_waiting_ != 0 || request_waiting_ != 0 || !handle_wrap_queue_.IsEmpty()) { diff --git a/src/env.h b/src/env.h index 57f8369f5983..751e3acec599 100644 --- a/src/env.h +++ b/src/env.h @@ -182,11 +182,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { inline worker::Worker* worker_context() const; inline void set_worker_context(worker::Worker* context); - // Non-zero while an Environment on this isolate is closing its handles with - // JS disallowed isolate-wide; InternalCallbackScope re-allows it for the - // other Environments whose callbacks run in those loop turns. - int handle_cleanup_depth = 0; - #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) diff --git a/src/node_internals.h b/src/node_internals.h index 1eacd95e3a42..1c4da8e2d203 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -283,6 +283,11 @@ class InternalCallbackScope { std::optional allow_js_; }; +// Non-zero while an Environment on this thread is closing its handles with JS +// disallowed isolate-wide; InternalCallbackScope re-allows it for the other +// Environments whose callbacks run in those loop turns. +extern thread_local int handle_cleanup_depth; + class DebugSealHandleScope { public: explicit inline DebugSealHandleScope(v8::Isolate* isolate = nullptr) From 7a663af47bbce7f474dfacbabad2d16ee2186f55 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 22 Sep 2026 13:48:46 +0000 Subject: [PATCH 2/8] inspector: throw ERR_INSPECTOR_NOT_AVAILABLE from an Environment without one An Environment created with kNoCreateInspector threw a bare string from inspector.Session#connect(), inspector.open() and the other Agent entry points, so callers could not tell the condition apart by error code. Use the ERR_INSPECTOR_NOT_AVAILABLE code that connectToMainThread() already throws for the same situation. Signed-off-by: Shelley Vohr --- src/inspector_agent.cc | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 9cdca9e1de21..a424e9e1a658 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -511,15 +511,8 @@ bool IsFilePath(const std::string& path) { #endif // __POSIX__ void ThrowUninitializedInspectorError(Environment* env) { - HandleScope scope(env->isolate()); - - std::string_view msg = - "This Environment was initialized without a V8::Inspector"; - Local exception; - if (ToV8Value(env->context(), msg, env->isolate()).ToLocal(&exception)) { - env->isolate()->ThrowException(exception); - } - // V8 will have scheduled a superseding error here. + THROW_ERR_INSPECTOR_NOT_AVAILABLE( + env, "This Environment was initialized without a V8::Inspector"); } } // namespace From 7a6bc3b13879c74e16af0a2bd1d1725a77be4793 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 22 Sep 2026 13:48:46 +0000 Subject: [PATCH 3/8] src: assert that an IsolateData outlives its Environments FreeIsolateData() while an Environment created from it is still alive left that Environment with a dangling pointer and failed later in unrelated code. Count the Environments using an IsolateData and CHECK in its destructor that none are left. Signed-off-by: Shelley Vohr --- src/env.cc | 7 ++++++- src/env.h | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/env.cc b/src/env.cc index 43aec44c7d55..16a326987cfc 100644 --- a/src/env.cc +++ b/src/env.cc @@ -649,7 +649,10 @@ IsolateData::IsolateData(Isolate* isolate, } } -IsolateData::~IsolateData() {} +IsolateData::~IsolateData() { + // FreeIsolateData() before FreeEnvironment() of an Environment using it. + CHECK_EQ(environment_count_, 0); +} // Deprecated API, embedders should use v8::Object::Wrap() directly instead. void SetCppgcReference(Isolate* isolate, @@ -981,6 +984,7 @@ Environment::Environment(IsolateData* isolate_data, ? AllocateEnvironmentThreadId().id : thread_id.id), thread_name_(thread_name) { + isolate_data->AddEnvironment(); #if HAVE_OPENSSL && NCRYPTO_USE_OPENSSL3_PROVIDER provider_digest_cache = std::make_unique(); provider_cipher_cache = std::make_unique(); @@ -1273,6 +1277,7 @@ Environment::~Environment() { cpu_profiler_->Dispose(); cpu_profiler_ = nullptr; } + isolate_data_->RemoveEnvironment(); } void Environment::InitializeLibuv() { diff --git a/src/env.h b/src/env.h index 751e3acec599..9c7bd95769e3 100644 --- a/src/env.h +++ b/src/env.h @@ -182,6 +182,9 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { inline worker::Worker* worker_context() const; inline void set_worker_context(worker::Worker* context); + void AddEnvironment() { environment_count_++; } + void RemoveEnvironment() { environment_count_--; } + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) @@ -283,6 +286,7 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { std::shared_ptr options_; worker::Worker* worker_context_ = nullptr; + size_t environment_count_ = 0; PerIsolateWrapperData* wrapper_data_; static Mutex isolate_data_mutex_; From c171538503274d09a23217a82e592ba1e355fca7 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 22 Sep 2026 13:48:46 +0000 Subject: [PATCH 4/8] test: cover Environments sharing an embedder-owned isolate Signed-off-by: Shelley Vohr --- .../cctest/test_environment_shared_isolate.cc | 458 ++++++++++++++++++ 1 file changed, 458 insertions(+) create mode 100644 test/cctest/test_environment_shared_isolate.cc diff --git a/test/cctest/test_environment_shared_isolate.cc b/test/cctest/test_environment_shared_isolate.cc new file mode 100644 index 000000000000..e8787d45c103 --- /dev/null +++ b/test/cctest/test_environment_shared_isolate.cc @@ -0,0 +1,458 @@ +// Several node::Environments on one v8::Isolate that the embedder created, +// registered with the platform and gave a CppHeap, each in its own +// embedder-created v8::Context, driven by one event loop and freed one at a +// time while the others keep running. doc/api/embedding.md allows this and +// embedders that host Node.js inside an existing JS runtime depend on it. + +#include "cppgc/allocation.h" +#include "cppgc/garbage-collected.h" +#include "node_test_fixture.h" +#include "v8-cppgc.h" + +#include +#include + +using node::Environment; +using node::IsolateData; +using v8::Context; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::Value; +namespace EnvironmentFlags = node::EnvironmentFlags; + +namespace { + +class EmbedderObject final : public v8::Object::Wrappable { + public: + static int alive; + EmbedderObject() { alive++; } + ~EmbedderObject() override { alive--; } +}; +int EmbedderObject::alive = 0; + +void SetFlag(void* flag) { + *static_cast(flag) = true; +} + +int cleanup_hook_runs = 0; +void CountCleanupHook(void* arg) { + (*static_cast(arg))++; +} + +enum class IsolateDataMode { + // One IsolateData created with the platform, shared by every Environment. + kShared, + // One IsolateData per Environment, created without a platform and freed + // together with its Environment. + kPerEnvironmentWithoutPlatform, +}; + +struct Instance { + int id; + v8::Global context; + IsolateData* isolate_data = nullptr; + Environment* env = nullptr; + bool exit_handler_called = false; + int exit_code = -1; + node::StopFlags::Flags stop_flags = node::StopFlags::kDoNotTerminateIsolate; +}; + +} // namespace + +class SharedIsolateTest + : public NodeZeroIsolateTestFixture, + public ::testing::WithParamInterface { + protected: + Isolate* isolate_ = nullptr; + v8::CppHeap* cpp_heap_ = nullptr; + IsolateData* shared_isolate_data_ = nullptr; + + void SetUp() override { + NodeZeroIsolateTestFixture::SetUp(); + + Isolate::CreateParams params; + params.array_buffer_allocator = allocator.get(); + params.cpp_heap = + v8::CppHeap::Create(platform.get(), v8::CppHeapCreateParams{{}}) + .release(); + cpp_heap_ = params.cpp_heap; + + isolate_ = Isolate::Allocate(); + CHECK_NOT_NULL(isolate_); + platform->RegisterIsolate(isolate_, ¤t_loop); + Isolate::Initialize(isolate_, params); + node::IsolateSettings settings; + settings.flags |= + node::IsolateSettingsFlags::SHOULD_NOT_SET_PREPARE_STACK_TRACE_CALLBACK; + node::SetIsolateUpForNode(isolate_, settings); + isolate_->Enter(); + + if (GetParam() == IsolateDataMode::kShared) { + HandleScope handle_scope(isolate_); + shared_isolate_data_ = + node::CreateIsolateData(isolate_, ¤t_loop, platform.get()); + CHECK_NOT_NULL(shared_isolate_data_); + } + } + + void TearDown() override { + if (shared_isolate_data_ != nullptr) { + node::FreeIsolateData(shared_isolate_data_); + shared_isolate_data_ = nullptr; + } + EXPECT_EQ(isolate_->GetCppHeap(), cpp_heap_); + platform->DrainTasks(isolate_); + isolate_->Exit(); + bool platform_finished = false; + platform->AddIsolateFinishedCallback(isolate_, SetFlag, &platform_finished); + isolate_->Dispose(); + platform->UnregisterIsolate(isolate_); + while (!platform_finished) uv_run(¤t_loop, UV_RUN_ONCE); + isolate_ = nullptr; + } + + std::unique_ptr CreateInstance(int id, + EnvironmentFlags::Flags flags, + bool throw_after_bootstrap = false) { + auto instance = std::make_unique(); + instance->id = id; + HandleScope handle_scope(isolate_); + Local context = Context::New(isolate_); + CHECK(node::InitializeContext(context).FromJust()); + instance->context.Reset(isolate_, context); + Context::Scope context_scope(context); + + if (GetParam() == IsolateDataMode::kShared) { + instance->isolate_data = shared_isolate_data_; + } else { + instance->isolate_data = + node::CreateIsolateData(isolate_, ¤t_loop, nullptr); + CHECK_NOT_NULL(instance->isolate_data); + } + + std::vector args{"node"}; + if (throw_after_bootstrap) args.push_back("--throw"); + std::vector exec_args; + instance->env = node::CreateEnvironment( + instance->isolate_data, context, args, exec_args, flags); + CHECK_NOT_NULL(instance->env); + + Instance* raw = instance.get(); + if (throw_after_bootstrap) instance->stop_flags = node::StopFlags::kNoFlags; + node::SetProcessExitHandler(instance->env, + [raw](Environment* env, int exit_code) { + raw->exit_handler_called = true; + raw->exit_code = exit_code; + node::Stop(env, raw->stop_flags); + }); + + std::string script = + "const id = " + std::to_string(id) + + ";\n" + "const vm = require('vm');\n" + "const { setInterval, clearInterval, setImmediate } =" + " require('timers');\n" + "require('net');\n" + "globalThis.state = { id, ticks: 0, immediates: 0, events: [] };\n" + "const interval = setInterval(() => {\n" + " state.ticks++;\n" + " new vm.Script('1 + 1');\n" + "}, 1);\n" + "(function again() {\n" + " state.immediates++;\n" + " setImmediate(again);\n" + "})();\n" + "process.on('beforeExit', () => state.events.push('beforeExit'));\n" + "process.on('exit', () => {\n" + " state.events.push('exit');\n" + " clearInterval(interval);\n" + "});\n" + "if (process.argv.includes('--throw')) throw new Error('uncaught');\n" + "return id;\n"; + v8::MaybeLocal result = + node::LoadEnvironment(instance->env, script.c_str()); + if (throw_after_bootstrap) { + CHECK(result.IsEmpty()); + CHECK(instance->exit_handler_called); + } else { + CHECK_EQ(result.ToLocalChecked()->Int32Value(context).FromJust(), id); + } + + node::AddEnvironmentCleanupHook( + isolate_, CountCleanupHook, &cleanup_hook_runs); + return instance; + } + + void FreeInstance(std::unique_ptr instance) { + node::FreeEnvironment(instance->env); + if (GetParam() == IsolateDataMode::kPerEnvironmentWithoutPlatform) { + node::FreeIsolateData(instance->isolate_data); + } + instance->context.Reset(); + } + + Local Evaluate(Instance* instance, const char* source) { + v8::EscapableHandleScope handle_scope(isolate_); + Local context = instance->context.Get(isolate_); + Context::Scope context_scope(context); + Local script = + v8::Script::Compile( + context, v8::String::NewFromUtf8(isolate_, source).ToLocalChecked()) + .ToLocalChecked(); + return handle_scope.Escape(script->Run(context).ToLocalChecked()); + } + + bool TryEvaluate(Instance* instance, const char* source) { + HandleScope handle_scope(isolate_); + Local context = instance->context.Get(isolate_); + Context::Scope context_scope(context); + v8::TryCatch try_catch(isolate_); + Local script = + v8::Script::Compile( + context, v8::String::NewFromUtf8(isolate_, source).ToLocalChecked()) + .ToLocalChecked(); + return !script->Run(context).IsEmpty() && !try_catch.HasTerminated(); + } + + int EvaluateInt(Instance* instance, const char* source) { + HandleScope handle_scope(isolate_); + Local context = instance->context.Get(isolate_); + return Evaluate(instance, source)->Int32Value(context).FromJust(); + } + + std::string EvaluateString(Instance* instance, const char* source) { + HandleScope handle_scope(isolate_); + v8::String::Utf8Value utf8(isolate_, Evaluate(instance, source)); + return *utf8; + } + + // What an embedder's message pump does: a bounded number of loop turns and + // foreground task flushes, never SpinEventLoop() on one Environment. + void PumpLoop(int turns) { + for (int i = 0; i < turns; i++) { + uv_run(¤t_loop, UV_RUN_NOWAIT); + platform->DrainTasks(isolate_); + } + } + + // Runs the loop until every instance's interval timer and immediate chain + // have both run again; a callback lost to a stray termination or exception + // breaks its chain and shows up here. + void PumpUntilAllTicked(const std::vector& instances) { + std::vector ticks, immediates; + for (Instance* instance : instances) { + ticks.push_back(EvaluateInt(instance, "state.ticks")); + immediates.push_back(EvaluateInt(instance, "state.immediates")); + } + for (int turn = 0; turn < 10000; turn++) { + uv_run(¤t_loop, UV_RUN_ONCE); + platform->DrainTasks(isolate_); + bool all_progressed = true; + for (size_t i = 0; i < instances.size(); i++) { + if (EvaluateInt(instances[i], "state.ticks") <= ticks[i] || + EvaluateInt(instances[i], "state.immediates") <= immediates[i]) { + all_progressed = false; + } + } + if (all_progressed) return; + } + FAIL() << "an Environment stopped running its timers or immediates"; + } +}; + +TEST_P(SharedIsolateTest, EnvironmentsComeAndGoWhileSiblingsRun) { + const HandleScope handle_scope(isolate_); + cleanup_hook_runs = 0; + + const auto sibling_flags = static_cast( + EnvironmentFlags::kNoCreateInspector | + EnvironmentFlags::kNoBrowserGlobals | + EnvironmentFlags::kNoRegisterESMLoader | + EnvironmentFlags::kNoGlobalSearchPaths); + + std::unique_ptr owner = + CreateInstance(0, EnvironmentFlags::kDefaultFlags); + std::unique_ptr second = CreateInstance(1, sibling_flags); + std::unique_ptr third = CreateInstance(2, sibling_flags); + EXPECT_EQ(isolate_->GetCppHeap(), cpp_heap_); + + PumpUntilAllTicked({owner.get(), second.get(), third.get()}); + + // The embedder's own cppgc objects live on the same heap as Node's. + v8::Global embedder_holder; + { + HandleScope inner(isolate_); + Local context = second->context.Get(isolate_); + Context::Scope context_scope(context); + Local holder = v8::FunctionTemplate::New(isolate_) + ->InstanceTemplate() + ->NewInstance(context) + .ToLocalChecked(); + v8::Object::Wrap( + isolate_, + holder, + cppgc::MakeGarbageCollected( + isolate_->GetCppHeap()->GetAllocationHandle())); + embedder_holder.Reset(isolate_, holder); + } + isolate_->LowMemoryNotification(); + EXPECT_EQ(EmbedderObject::alive, 1); + +#if HAVE_INSPECTOR + EXPECT_EQ(EvaluateInt( + owner.get(), + "(() => {\n" + " const { Session } = process.getBuiltinModule('inspector');\n" + " const session = new Session();\n" + " session.connect();\n" + " let value = -1;\n" + " session.post('Runtime.evaluate', { expression: 'state.id + " + "42' },\n" + " (err, res) => { value = err ? -2 : " + "res.result.value; });\n" + " console.time('session'); console.timeEnd('session');\n" + " session.disconnect();\n" + " return value;\n" + "})()"), + 42); + // A sibling without an inspector of its own gets an exception, not an abort. + EXPECT_EQ( + EvaluateString( + second.get(), + "(() => {\n" + " try {\n" + " const { Session } = process.getBuiltinModule('inspector');\n" + " const session = new Session();\n" + " session.connect();\n" + " session.disconnect();\n" + " return 'connected';\n" + " } catch (e) { return String(e.code); }\n" + "})()"), + "ERR_INSPECTOR_NOT_AVAILABLE"); +#endif // HAVE_INSPECTOR + + if (GetParam() == IsolateDataMode::kShared) { + // Workers need a platform; their exit must not disturb the siblings. + EXPECT_EQ( + EvaluateInt(third.get(), + "(() => {\n" + " const { Worker } =" + " process.getBuiltinModule('worker_threads');\n" + " state.workerExit = -1;\n" + " new Worker('process.exit(7)', { eval: true })\n" + " .on('exit', (code) => { state.workerExit = code; });\n" + " return 0;\n" + "})()"), + 0); + for (int turn = 0; turn < 10000; turn++) { + PumpLoop(1); + if (EvaluateInt(third.get(), "state.workerExit") == 7) break; + } + EXPECT_EQ(EvaluateInt(third.get(), "state.workerExit"), 7); + } + + PumpUntilAllTicked({owner.get(), second.get(), third.get()}); + + // Free one Environment while its siblings have timers and immediates due. + const int owner_ticks = EvaluateInt(owner.get(), "state.ticks"); + FreeInstance(std::move(second)); + EXPECT_EQ(cleanup_hook_runs, 1); + EXPECT_FALSE(owner->exit_handler_called); + EXPECT_FALSE(third->exit_handler_called); + PumpUntilAllTicked({owner.get(), third.get()}); + EXPECT_GT(EvaluateInt(owner.get(), "state.ticks"), owner_ticks); + + // The embedder's object outlives the Environment whose context wrapped it. + isolate_->LowMemoryNotification(); + EXPECT_EQ(EmbedderObject::alive, 1); + embedder_holder.Reset(); + + // An uncaught exception whose exit handler calls Stop() without + // kDoNotTerminateIsolate, then free: the termination it requests must not + // leak into the sibling's next script. + third->stop_flags = node::StopFlags::kNoFlags; + EXPECT_EQ(EvaluateInt(third.get(), + "process.getBuiltinModule('timers').setImmediate(" + "() => { throw new Error('uncaught'); }), 0"), + 0); + for (int turn = 0; turn < 100 && !third->exit_handler_called; turn++) { + PumpLoop(1); + } + EXPECT_TRUE(third->exit_handler_called); + EXPECT_EQ(third->exit_code, 1); + FreeInstance(std::move(third)); + EXPECT_EQ(cleanup_hook_runs, 2); + EXPECT_TRUE(TryEvaluate(owner.get(), "state.ticks")); + PumpUntilAllTicked({owner.get()}); + EXPECT_FALSE(owner->exit_handler_called); + + // The same when the exception is thrown by the bootstrap script itself and + // no JavaScript of that Environment runs after its exit handler. + std::unique_ptr failed = CreateInstance(4, sibling_flags, true); + EXPECT_EQ(failed->exit_code, 1); + FreeInstance(std::move(failed)); + EXPECT_EQ(cleanup_hook_runs, 3); + EXPECT_TRUE(TryEvaluate(owner.get(), "state.ticks")); + PumpUntilAllTicked({owner.get()}); + + // A new sibling can join after others left, here a second one that owns an + // inspector and debug signal handler of its own. + std::unique_ptr late = + CreateInstance(3, EnvironmentFlags::kDefaultFlags); + PumpUntilAllTicked({owner.get(), late.get()}); + + // beforeExit / exit are per Environment. + { + HandleScope inner(isolate_); + Context::Scope context_scope(late->context.Get(isolate_)); + node::EmitProcessBeforeExit(late->env).Check(); + } + EXPECT_EQ(EvaluateString(late.get(), "state.events.join()"), "beforeExit"); + EXPECT_EQ(EvaluateString(owner.get(), "state.events.join()"), ""); + { + HandleScope inner(isolate_); + Context::Scope context_scope(late->context.Get(isolate_)); + EXPECT_EQ(node::EmitProcessExit(late->env).FromJust(), 0); + } + EXPECT_EQ(EvaluateString(late.get(), "state.events.join()"), + "beforeExit,exit"); + EXPECT_EQ(EvaluateString(owner.get(), "state.events.join()"), ""); + node::Stop(late->env, node::StopFlags::kDoNotTerminateIsolate); + PumpUntilAllTicked({owner.get()}); + EXPECT_EQ(EvaluateInt(late.get(), "state.events.length"), 2); + FreeInstance(std::move(late)); + EXPECT_EQ(cleanup_hook_runs, 4); + + PumpUntilAllTicked({owner.get()}); + node::Stop(owner->env, node::StopFlags::kDoNotTerminateIsolate); + FreeInstance(std::move(owner)); + EXPECT_EQ(cleanup_hook_runs, 5); + + // Nothing the Environments left behind keeps the shared loop busy. + PumpLoop(2); + EXPECT_EQ(uv_loop_alive(¤t_loop), 0); +} + +TEST_P(SharedIsolateTest, FreeIsolateDataBeforeItsEnvironmentAsserts) { + GTEST_FLAG_SET(death_test_style, "threadsafe"); + const HandleScope handle_scope(isolate_); + std::unique_ptr instance = + CreateInstance(0, EnvironmentFlags::kNoCreateInspector); + IsolateData* isolate_data = instance->isolate_data; + EXPECT_DEATH_IF_SUPPORTED(node::FreeIsolateData(isolate_data), + "environment_count_"); + node::Stop(instance->env, node::StopFlags::kDoNotTerminateIsolate); + FreeInstance(std::move(instance)); +} + +INSTANTIATE_TEST_SUITE_P( + EnvironmentTest, + SharedIsolateTest, + ::testing::Values(IsolateDataMode::kShared, + IsolateDataMode::kPerEnvironmentWithoutPlatform), + [](const ::testing::TestParamInfo& info) { + return info.param == IsolateDataMode::kShared + ? "SharedIsolateData" + : "IsolateDataPerEnvironmentWithoutPlatform"; + }); From 65b3b9efdbb2182ecdb51fe98c2f3cc5e44e8fc8 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 22 Sep 2026 13:58:41 +0000 Subject: [PATCH 5/8] doc: update FreeEnvironment() notes for sibling Environments With the FreeEnvironment() fix for sibling Environments and the handle cleanup depth tracked per thread, only the Environment being freed loses JavaScript while FreeEnvironment() runs the shared loop. Callbacks of the other Environments on that loop run their JavaScript as usual. Update embedding.md and the comment in node.h, which still describe JavaScript as disallowed on the whole isolate. Signed-off-by: Shelley Vohr --- doc/api/embedding.md | 11 ++++------- src/node.h | 4 ++-- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/doc/api/embedding.md b/doc/api/embedding.md index dfb84b49ef9b..7098973c6c2a 100644 --- a/doc/api/embedding.md +++ b/doc/api/embedding.md @@ -95,13 +95,10 @@ to as `node::Environment`. Each `node::Environment` is associated with: `node::Environment`s that share a `node::IsolateData` also share its `uv_loop_t`. `node::FreeEnvironment()` runs that loop until the handles of the -`node::Environment` being freed have closed, and JavaScript execution is -disallowed on the whole `v8::Isolate` while it does, so pending timers, I/O -callbacks and thread pool completions that belong to other `node::Environment`s -on the same loop can run inside that call without being able to call into -JavaScript. `node::Environment`s that are freed independently of one another -should each use their own `uv_loop_t` and `node::IsolateData`, or the embedder -should make sure the others have no pending work when one of them is freed. +`node::Environment` being freed have closed. Timers, I/O callbacks and thread +pool completions of the other `node::Environment`s that become due in those +loop iterations run normally, including their JavaScript; only the +`node::Environment` being freed can no longer call into JavaScript. In order to set up a `v8::Isolate`, an `v8::ArrayBuffer::Allocator` needs to be provided. One possible choice is the default Node.js allocator, which diff --git a/src/node.h b/src/node.h index 9018a4b39b02..6f39ed6f7f40 100644 --- a/src/node.h +++ b/src/node.h @@ -889,8 +889,8 @@ NODE_EXTERN v8::MaybeLocal LoadEnvironment( EmbedderPreloadCallback preload = nullptr); // Runs `env`'s event loop until its handles have closed, with JavaScript -// execution disallowed on the isolate; see doc/api/embedding.md if that loop -// is shared with other Environments. +// execution disallowed for `env`; see doc/api/embedding.md if that loop is +// shared with other Environments. NODE_EXTERN void FreeEnvironment(Environment* env); // Set a callback that is called when process.exit() is called from JS, From 201e4f3d88db58e6964f8c85aebc0c2dce7abfcb Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Sat, 26 Sep 2026 10:50:59 +0000 Subject: [PATCH 6/8] crypto: keep the root cert store per Environment The root cert store and the certificates set through tls.setDefaultCACertificates() were thread_local, with a cleanup hook on whichever Environment used TLS first. When several Environments share a thread, setting the default CA certificates in one of them replaced the trusted CAs of the others. Keep both on the Environment, next to its other OpenSSL state. Signed-off-by: Shelley Vohr --- src/crypto/crypto_context.cc | 73 ++++++++----------- src/env.cc | 1 + src/env.h | 8 ++ .../cctest/test_environment_shared_isolate.cc | 29 ++++++++ 4 files changed, 70 insertions(+), 41 deletions(-) diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 823d87e7f204..9ad4b5abdd85 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -95,37 +95,27 @@ struct X509Less { }; using X509Set = std::set; -// Per-thread root cert store. See NewRootCertStore() on what it contains. -static thread_local DeleteFnPtr root_cert_store; -// If the user calls tls.setDefaultCACertificates() this will be used -// to hold the user-provided certificates, the root_cert_store and any new -// copy generated by NewRootCertStore() will then contain the certificates -// from this set. -static thread_local std::unique_ptr root_certs_from_users; -static thread_local bool has_cleanup_hook = false; - -static void CleanupRootCertStore(void*) { - root_cert_store.reset(); - root_certs_from_users.reset(); - has_cleanup_hook = false; -} - -static void EnsureRootCertStoreCleanupHook(Environment* env) { - if (env == nullptr || has_cleanup_hook) { - return; - } +struct RootCertStore { + // See NewRootCertStore() on what it contains. + DeleteFnPtr store; + // Set by tls.setDefaultCACertificates(). Once set, NewRootCertStore() + // copies these certificates instead of loading the defaults. + std::unique_ptr certs_from_users; +}; - env->AddCleanupHook(CleanupRootCertStore, nullptr); - has_cleanup_hook = true; +void FreeRootCertStore(RootCertStore* root_certs) { + delete root_certs; +} + +static RootCertStore* GetRootCertStore(Environment* env) { + if (!env->root_cert_store) env->root_cert_store.reset(new RootCertStore()); + return env->root_cert_store.get(); } X509_STORE* GetOrCreateRootCertStore(Environment* env) { - EnsureRootCertStoreCleanupHook(env); - if (root_cert_store != nullptr) { - return root_cert_store.get(); - } - root_cert_store.reset(NewRootCertStore(env)); - return root_cert_store.get(); + RootCertStore* root_certs = GetRootCertStore(env); + if (!root_certs->store) root_certs->store.reset(NewRootCertStore(env)); + return root_certs->store.get(); } // Takes a string or buffer and loads it into a BIO. @@ -1062,8 +1052,9 @@ X509_STORE* NewRootCertStore(Environment* env) { // If the root cert store is already reset by users through // tls.setDefaultCACertificates(), just create a copy from the // user-provided certificates. - if (root_certs_from_users != nullptr) { - for (const auto& cert : *root_certs_from_users) { + const auto& certs_from_users = GetRootCertStore(env)->certs_from_users; + if (certs_from_users) { + for (const auto& cert : *certs_from_users) { CHECK_EQ(1, X509_STORE_add_cert(store, cert.get())); } return store; @@ -1230,12 +1221,13 @@ MaybeLocal X509sToArrayOfStrings(Environment* env, void GetUserRootCertificates(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - CHECK_NOT_NULL(root_certs_from_users); + const auto& certs_from_users = GetRootCertStore(env)->certs_from_users; + CHECK(certs_from_users); Local results; if (X509sToArrayOfStrings(env, - root_certs_from_users->begin(), - root_certs_from_users->end(), - root_certs_from_users->size()) + certs_from_users->begin(), + certs_from_users->end(), + certs_from_users->size()) .ToLocal(&results)) { args.GetReturnValue().Set(results); } @@ -1246,12 +1238,12 @@ void ResetRootCertStore(const FunctionCallbackInfo& args) { CHECK(args[0]->IsArray()); Local cert_array = args[0].As(); Environment* env = Environment::GetCurrent(context); - EnsureRootCertStoreCleanupHook(env); + RootCertStore* root_certs = GetRootCertStore(env); if (cert_array->Length() == 0) { // If the array is empty, just clear the user certs and reset the store. - root_cert_store.reset(); - root_certs_from_users = std::make_unique(); + root_certs->store.reset(); + root_certs->certs_from_users = std::make_unique(); return; } @@ -1263,7 +1255,6 @@ void ResetRootCertStore(const FunctionCallbackInfo& args) { } if (certs->empty()) { - Environment* env = Environment::GetCurrent(context); return THROW_ERR_CRYPTO_OPERATION_FAILED( env, "No valid certificates found in the provided array"); } @@ -1275,11 +1266,11 @@ void ResetRootCertStore(const FunctionCallbackInfo& args) { // is not consumed by insert (element already exists). } - root_certs_from_users = std::move(new_set); + root_certs->certs_from_users = std::move(new_set); - // Reset the global root cert store so it will be recreated with the - // new certificates. - root_cert_store.reset(); + // Reset the root cert store so it will be recreated with the new + // certificates. + root_certs->store.reset(); } void GetSystemCACertificates(const FunctionCallbackInfo& args) { diff --git a/src/env.cc b/src/env.cc index 16a326987cfc..7c8efd27dfcf 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1260,6 +1260,7 @@ Environment::~Environment() { // environment-owned methods before unloading any addon DSOs. provider_digest_cache.reset(); provider_cipher_cache.reset(); + root_cert_store.reset(); #if OPENSSL_WITH_EVP_MAC provider_mac_cache.reset(); #endif diff --git a/src/env.h b/src/env.h index 9c7bd95769e3..32acc81a00bb 100644 --- a/src/env.h +++ b/src/env.h @@ -76,6 +76,13 @@ class MacCache; namespace node { +#if HAVE_OPENSSL +namespace crypto { +struct RootCertStore; +void FreeRootCertStore(RootCertStore* root_certs); +} // namespace crypto +#endif // HAVE_OPENSSL + namespace shadow_realm { class ShadowRealm; } @@ -1211,6 +1218,7 @@ class Environment final : public MemoryRetainer { std::unique_ptr provider_mac_cache; std::vector supported_mac_algorithms; bool supported_mac_algorithms_initialized = false; + DeleteFnPtr root_cert_store; #endif // HAVE_OPENSSL v8::Global temporary_required_module_facade_original; diff --git a/test/cctest/test_environment_shared_isolate.cc b/test/cctest/test_environment_shared_isolate.cc index e8787d45c103..c303403f64c5 100644 --- a/test/cctest/test_environment_shared_isolate.cc +++ b/test/cctest/test_environment_shared_isolate.cc @@ -6,8 +6,12 @@ #include "cppgc/allocation.h" #include "cppgc/garbage-collected.h" +#include "env-inl.h" #include "node_test_fixture.h" #include "v8-cppgc.h" +#if HAVE_OPENSSL +#include "crypto/crypto_context.h" +#endif #include #include @@ -446,6 +450,31 @@ TEST_P(SharedIsolateTest, FreeIsolateDataBeforeItsEnvironmentAsserts) { FreeInstance(std::move(instance)); } +#if HAVE_OPENSSL +TEST_P(SharedIsolateTest, RootCertStoreIsPerEnvironment) { + const HandleScope handle_scope(isolate_); + std::unique_ptr first = + CreateInstance(0, EnvironmentFlags::kNoCreateInspector); + std::unique_ptr second = + CreateInstance(1, EnvironmentFlags::kNoCreateInspector); + auto store_size = [](Instance* instance) { + return sk_X509_OBJECT_num(X509_STORE_get0_objects( + node::crypto::GetOrCreateRootCertStore(instance->env))); + }; + const int default_size = store_size(second.get()); + EXPECT_GT(default_size, 0); + + Evaluate(first.get(), + "process.getBuiltinModule('tls').setDefaultCACertificates([])"); + EXPECT_EQ(store_size(first.get()), 0); + EXPECT_EQ(store_size(second.get()), default_size); + + FreeInstance(std::move(first)); + EXPECT_EQ(store_size(second.get()), default_size); + FreeInstance(std::move(second)); +} +#endif // HAVE_OPENSSL + INSTANTIATE_TEST_SUITE_P( EnvironmentTest, SharedIsolateTest, From d9a25c0e303d6d172944ee356754f7b597f9b4d2 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Sat, 26 Sep 2026 10:50:59 +0000 Subject: [PATCH 7/8] quic: give each BindingData its own allocator state The ngtcp2 and nghttp3 allocators shared one thread_local state whose BindingData pointer was set by the last BindingData that handed out an allocator. With several Environments on a thread, memory allocated for a session in one Environment was accounted against another's BindingData and failed a CHECK when freed. Give each BindingData its own heap-allocated state. nghttp3 buffers backing external strings can be freed after the BindingData is gone, so the state counts live allocations and is deleted once the BindingData has been destroyed and the last of them is freed. Signed-off-by: Shelley Vohr --- src/quic/README.md | 12 ++-- src/quic/bindingdata.cc | 63 +++++++++---------- src/quic/bindingdata.h | 10 +-- .../cctest/test_environment_shared_isolate.cc | 37 +++++++++++ 4 files changed, 77 insertions(+), 45 deletions(-) diff --git a/src/quic/README.md b/src/quic/README.md index 8c23ed2f4af9..acea22840a7e 100644 --- a/src/quic/README.md +++ b/src/quic/README.md @@ -150,28 +150,30 @@ The Application is selected as soon as the ALPN protocol is known: immediately for clients, and for servers from the `OnClientHello` TLS callback (see [Server handshake ordering](#server-handshake-ordering)). -### Thread-Local Allocator +### Allocator Both ngtcp2 and nghttp3 require custom allocators (`ngtcp2_mem`, `nghttp3_mem`). These allocator structs must outlive every object they create. Some nghttp3 objects (notably `rcbuf`s backing V8 external strings) can survive past `BindingData` destruction during isolate teardown. -The solution uses `thread_local` storage: +Each `BindingData` owns a heap-allocated `QuicAllocState` that holds both +allocator structs and counts live allocations: ```cpp struct QuicAllocState { - BindingData* binding = nullptr; // Nulled in ~BindingData + BindingData* binding; // Nulled in ~BindingData + size_t live_allocations = 0; ngtcp2_mem ngtcp2; nghttp3_mem nghttp3; }; -thread_local QuicAllocState quic_alloc_state; ``` Each allocation prepends its size before the returned pointer. This allows `free` and `realloc` to report correct sizes for memory tracking. When `binding` is null (after `BindingData` destruction), allocations still -succeed but memory tracking is silently skipped. +succeed but memory tracking is silently skipped. The state is deleted once +`binding` is null and the last allocation has been freed. ## Session Lifecycle diff --git a/src/quic/bindingdata.cc b/src/quic/bindingdata.cc index e73ae82e980d..49f6984d574b 100644 --- a/src/quic/bindingdata.cc +++ b/src/quic/bindingdata.cc @@ -37,33 +37,30 @@ using v8::Value; namespace quic { // ============================================================================ -// Thread-local QUIC allocator. +// QUIC allocator. // -// Both ngtcp2 and nghttp3 take an allocator struct (ngtcp2_mem / -// nghttp3_mem) whose pointer is stored inside every object they -// allocate. Some of those objects — notably nghttp3 rcbufs backing -// V8 external strings — can outlive the BindingData that created them -// (freed during V8 isolate teardown, after Environment cleanup). -// -// To handle this safely, both allocators live in a thread-local static -// struct that is never destroyed. Memory tracking goes through the -// BindingData pointer when it is alive and is silently skipped during -// teardown (after ~BindingData nulls the pointer). +// ngtcp2 and nghttp3 keep a pointer to their allocator struct in every object +// they allocate, and nghttp3 rcbufs backing V8 external strings can be freed +// after the BindingData is gone. A QuicAllocState is therefore deleted only +// once its BindingData has been destroyed and its last allocation freed. // // The allocation functions use the same prepended-size-header scheme as // NgLibMemoryManager (node_mem-inl.h) so that frees always know the // allocation size regardless of whether BindingData is still around. -namespace { struct QuicAllocState { - BindingData* binding = nullptr; + BindingData* binding; + size_t live_allocations = 0; ngtcp2_mem ngtcp2 = {}; nghttp3_mem nghttp3 = {}; + + void OnFreed() { + CHECK_GT(live_allocations, 0); + if (--live_allocations == 0 && binding == nullptr) delete this; + } }; -thread_local QuicAllocState quic_alloc_state; -// Core allocation functions shared by both ngtcp2 and nghttp3. -// user_data always points to the thread-local QuicAllocState. +namespace { void* QuicRealloc(void* ptr, size_t size, void* user_data) { auto* state = static_cast(user_data); @@ -78,6 +75,10 @@ void* QuicRealloc(void* ptr, size_t size, void* user_data) { previous_size = *reinterpret_cast(original_ptr); if (previous_size == 0) { char* ret = UncheckedRealloc(original_ptr, size); + if (size == 0) { + state->OnFreed(); + return nullptr; + } if (ret != nullptr) ret += kReserveSizeAndAlign; return ret; } @@ -96,6 +97,7 @@ void* QuicRealloc(void* ptr, size_t size, void* user_data) { state->binding->env()->external_memory_accounter()->Update( state->binding->env()->isolate(), new_size); } + if (ptr == nullptr) state->live_allocations++; *reinterpret_cast(mem) = size; mem += kReserveSizeAndAlign; } else if (size == 0) { @@ -104,6 +106,7 @@ void* QuicRealloc(void* ptr, size_t size, void* user_data) { state->binding->env()->external_memory_accounter()->Decrease( state->binding->env()->isolate(), previous_size); } + if (ptr != nullptr) state->OnFreed(); } return mem; } @@ -232,7 +235,8 @@ BindingData& BindingData::Get(Environment* env) { } BindingData::~BindingData() { - quic_alloc_state.binding = nullptr; + alloc_state_->binding = nullptr; + if (alloc_state_->live_allocations == 0) delete alloc_state_; // flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor // body completes. The inner CheckWrap (and its uv_check_t) will be freed // later by the uv_close callback, after CleanupHandles() runs uv_run(). @@ -240,27 +244,11 @@ BindingData::~BindingData() { } ngtcp2_mem* BindingData::ngtcp2_allocator() { - quic_alloc_state.binding = this; - quic_alloc_state.ngtcp2 = { - &quic_alloc_state, - Ngtcp2Malloc, - Ngtcp2Free, - Ngtcp2Calloc, - Ngtcp2Realloc, - }; - return &quic_alloc_state.ngtcp2; + return &alloc_state_->ngtcp2; } nghttp3_mem* BindingData::nghttp3_allocator() { - quic_alloc_state.binding = this; - quic_alloc_state.nghttp3 = { - &quic_alloc_state, - Nghttp3Malloc, - Nghttp3Free, - Nghttp3Calloc, - Nghttp3Realloc, - }; - return &quic_alloc_state.nghttp3; + return &alloc_state_->nghttp3; } void BindingData::CheckAllocatedSize(size_t previous_size) const { @@ -349,7 +337,12 @@ JS_METHOD_IMPL(BindingData::SetHeadersInterest) { BindingData::BindingData(Realm* realm, Local object) : BaseObject(realm, object), + alloc_state_(new QuicAllocState{this}), flush_check_(env(), [this]() { OnFlushCheck(); }) { + alloc_state_->ngtcp2 = { + alloc_state_, Ngtcp2Malloc, Ngtcp2Free, Ngtcp2Calloc, Ngtcp2Realloc}; + alloc_state_->nghttp3 = { + alloc_state_, Nghttp3Malloc, Nghttp3Free, Nghttp3Calloc, Nghttp3Realloc}; MakeWeak(); // Unref so the check handle doesn't keep the event loop alive on its own. flush_check_.Unref(); diff --git a/src/quic/bindingdata.h b/src/quic/bindingdata.h index 2ef9f7685314..7d424b9bf6f6 100644 --- a/src/quic/bindingdata.h +++ b/src/quic/bindingdata.h @@ -24,6 +24,7 @@ class Endpoint; class Packet; class Session; class SessionManager; +struct QuicAllocState; // ============================================================================ @@ -281,15 +282,12 @@ class BindingData final // NgLibMemoryManager — the base class provides CheckAllocatedSize, // IncreaseAllocatedSize, DecreaseAllocatedSize, and StopTrackingMemory. - // Actual allocations go through the thread-local allocators below. + // Actual allocations go through the allocators below. void CheckAllocatedSize(size_t previous_size) const; void IncreaseAllocatedSize(size_t size); void DecreaseAllocatedSize(size_t size); - // Thread-local allocators that outlive BindingData destruction. - // Both ngtcp2 and nghttp3 store the allocator pointer inside every - // object they allocate; some of those objects (e.g., nghttp3 rcbufs - // backing V8 external strings) can be freed after BindingData is gone. + // The allocators can outlive the BindingData; see QuicAllocState. ngtcp2_mem* ngtcp2_allocator(); nghttp3_mem* nghttp3_allocator(); @@ -384,6 +382,8 @@ class BindingData final ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}}; ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}}; + QuicAllocState* alloc_state_; + // Deferred send flush state. The CheckWrapHandle fires immediately after // the I/O poll phase in the same event loop tick, allowing batched // receive processing: all packets are read during poll, then diff --git a/test/cctest/test_environment_shared_isolate.cc b/test/cctest/test_environment_shared_isolate.cc index c303403f64c5..def48ab88f3b 100644 --- a/test/cctest/test_environment_shared_isolate.cc +++ b/test/cctest/test_environment_shared_isolate.cc @@ -12,6 +12,10 @@ #if HAVE_OPENSSL #include "crypto/crypto_context.h" #endif +#if HAVE_QUIC +#include "node_realm-inl.h" +#include "quic/bindingdata.h" +#endif #include #include @@ -475,6 +479,39 @@ TEST_P(SharedIsolateTest, RootCertStoreIsPerEnvironment) { } #endif // HAVE_OPENSSL +#if HAVE_QUIC +TEST_P(SharedIsolateTest, QuicAllocatorIsPerEnvironment) { + const HandleScope handle_scope(isolate_); + std::unique_ptr first = + CreateInstance(0, EnvironmentFlags::kNoCreateInspector); + std::unique_ptr second = + CreateInstance(1, EnvironmentFlags::kNoCreateInspector); + auto allocator = [this](Instance* instance) { + HandleScope inner(isolate_); + Local context = instance->context.Get(isolate_); + Context::Scope context_scope(context); + Local name = v8::String::NewFromUtf8Literal(isolate_, "quic"); + instance->env->principal_realm() + ->internal_binding_loader() + ->Call(context, v8::Undefined(isolate_), 1, &name) + .ToLocalChecked(); + return node::quic::BindingData::Get(instance->env).ngtcp2_allocator(); + }; + + ngtcp2_mem* first_mem = allocator(first.get()); + void* first_ptr = first_mem->malloc(64, first_mem->user_data); + ngtcp2_mem* second_mem = allocator(second.get()); + void* second_ptr = second_mem->malloc(16, second_mem->user_data); + EXPECT_NE(first_mem, second_mem); + first_mem->free(first_ptr, first_mem->user_data); + + FreeInstance(std::move(second)); + second_ptr = second_mem->realloc(second_ptr, 32, second_mem->user_data); + second_mem->free(second_ptr, second_mem->user_data); + FreeInstance(std::move(first)); +} +#endif // HAVE_QUIC + INSTANTIATE_TEST_SUITE_P( EnvironmentTest, SharedIsolateTest, From 1843e59cc454756bc458420ccbd54887cd96a39b Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Sat, 26 Sep 2026 10:50:59 +0000 Subject: [PATCH 8/8] tools: lint thread_local in src Several Environments can share a thread, so state in src/ that belongs to one of them cannot be kept in a thread_local. Add a cpplint rule that rejects thread_local in src/ unless the declaration is marked with NOLINTNEXTLINE(runtime/thread_local), and mark the existing uses, which are per-thread by design. Signed-off-by: Shelley Vohr --- src/api/environment.cc | 1 + src/env.cc | 1 + src/node_binding.cc | 2 ++ src/node_debug.cc | 2 ++ src/node_errors.cc | 2 ++ src/node_internals.h | 1 + src/quic/data.cc | 1 + src/quic/defs.h | 1 + tools/cpplint.py | 22 ++++++++++++++++++++++ 9 files changed, 33 insertions(+) diff --git a/src/api/environment.cc b/src/api/environment.cc index ee9da42b606b..b8963b0fbcad 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -1048,6 +1048,7 @@ Maybe InitializePrimordials(Local context, // in the first place. However, creating BuiltinLoader instances is // relatively cheap and all the scripts that we may want to run at // startup are always present in it. + // NOLINTNEXTLINE(runtime/thread_local) thread_local builtins::BuiltinLoader builtin_loader; // Primordials can always be just eagerly compiled. builtin_loader.SetEagerCompile(); diff --git a/src/env.cc b/src/env.cc index 7c8efd27dfcf..3ba1579e0c66 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1442,6 +1442,7 @@ void Environment::ClosePerEnvHandles() { close_and_finish(reinterpret_cast(&task_queues_async_)); } +// NOLINTNEXTLINE(runtime/thread_local) thread_local int handle_cleanup_depth = 0; void Environment::CleanupHandles() { diff --git a/src/node_binding.cc b/src/node_binding.cc index 568325e8496a..8cb5578eb048 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -180,6 +180,7 @@ struct dl_wrap { static Mutex dlhandles_mutex; static std::unordered_set dlhandles; +// NOLINTNEXTLINE(runtime/thread_local) static thread_local std::string dlerror_storage; char* wrapped_dlerror() { @@ -286,6 +287,7 @@ using v8::Value; // Globals per process static node_module* modlist_internal; static node_module* modlist_linked; +// NOLINTNEXTLINE(runtime/thread_local) static thread_local node_module* thread_local_modpending; // This is set by node::Init() which is used by embedders diff --git a/src/node_debug.cc b/src/node_debug.cc index c995e791f252..8efb049a76fc 100644 --- a/src/node_debug.cc +++ b/src/node_debug.cc @@ -24,8 +24,10 @@ using v8::Number; using v8::Object; using v8::Value; +// NOLINTNEXTLINE(runtime/thread_local) thread_local std::unordered_map generic_usage_counters; +// NOLINTNEXTLINE(runtime/thread_local) thread_local std::unordered_map v8_fast_api_call_counts; diff --git a/src/node_errors.cc b/src/node_errors.cc index cf000047d38d..cf71884c88db 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -190,9 +190,11 @@ static std::string GetErrorSource(Isolate* isolate, } static std::atomic is_in_oom{false}; +// NOLINTNEXTLINE(runtime/thread_local) static thread_local std::atomic is_retrieving_js_stacktrace{false}; // This is thread-local because it only guards re-entrancy within the current // thread's uncaught-exception path; no cross-thread synchronization is needed. +// NOLINTNEXTLINE(runtime/thread_local) static thread_local bool is_in_uncaught_exception = false; MaybeLocal GetCurrentStackTrace(Isolate* isolate, int frame_count) { if (isolate == nullptr) { diff --git a/src/node_internals.h b/src/node_internals.h index 1c4da8e2d203..bb45d73e9090 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -286,6 +286,7 @@ class InternalCallbackScope { // Non-zero while an Environment on this thread is closing its handles with JS // disallowed isolate-wide; InternalCallbackScope re-allows it for the other // Environments whose callbacks run in those loop turns. +// NOLINTNEXTLINE(runtime/thread_local) extern thread_local int handle_cleanup_depth; class DebugSealHandleScope { diff --git a/src/quic/data.cc b/src/quic/data.cc index 9599adec62f8..f266caf68132 100644 --- a/src/quic/data.cc +++ b/src/quic/data.cc @@ -33,6 +33,7 @@ using v8::Undefined; using v8::Value; namespace quic { +// NOLINTNEXTLINE(runtime/thread_local) thread_local int DebugIndentScope::indent_ = 0; Path::Path(const SocketAddress& local, const SocketAddress& remote) { diff --git a/src/quic/defs.h b/src/quic/defs.h index 45b4c77d1584..2f249b4ee469 100644 --- a/src/quic/defs.h +++ b/src/quic/defs.h @@ -395,6 +395,7 @@ class DebugIndentScope final { } private: + // NOLINTNEXTLINE(runtime/thread_local) static thread_local int indent_; }; diff --git a/tools/cpplint.py b/tools/cpplint.py index 464d95b8824f..1e26e3ded396 100755 --- a/tools/cpplint.py +++ b/tools/cpplint.py @@ -350,6 +350,7 @@ "runtime/printf_format", "runtime/references", "runtime/string", + "runtime/thread_local", "runtime/threadsafe_fn", "runtime/vlog", "runtime/v8_persistent", @@ -7446,6 +7447,25 @@ def CheckStringValueUsage(filename, lines, error): 'Use node::TwoByteValue instead.') +def CheckThreadLocalUsage(filename, lines, error): + """Logs an error if thread_local is used in src/. + Args: + filename: The name of the current file. + lines: An array of strings, each representing a line of the file. + error: The function to call with any errors found. + """ + if not (filename.startswith('src/') or filename.startswith('src\\')): + return + + for linenum, line in enumerate(lines): + if re.search(r'\bthread_local\b', line.split('//', 1)[0]): + error(filename, linenum, 'runtime/thread_local', 5, + 'Several Environments can share a thread, so keep state that ' + 'belongs to one on the Environment or its BindingData. Mark ' + 'intentionally per-thread state with ' + 'NOLINTNEXTLINE(runtime/thread_local).') + + def ProcessLine( filename, file_extension, @@ -7609,6 +7629,8 @@ def ProcessFileData(filename, file_extension, lines, error, extra_check_function CheckStringValueUsage(filename, lines, error) + CheckThreadLocalUsage(filename, lines, error) + def ProcessConfigOverrides(filename): """Loads the configuration files and processes the config overrides.