src: keep per-Environment state out of thread_locals

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

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<Context> current_context = isolate->GetCurrentContext();

src/api/environment.cc

@@ -1048,6 +1048,7 @@ Maybe<void> InitializePrimordials(Local<Context> 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();

src/crypto/crypto_context.cc

@@ -95,37 +95,27 @@ struct X509Less {
 };
 using X509Set = std::set<ncrypto::X509Pointer, X509Less>;
 
-// Per-thread root cert store. See NewRootCertStore() on what it contains.
-static thread_local DeleteFnPtr<X509_STORE, X509_STORE_free> 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<X509Set> 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<X509_STORE, X509_STORE_free> store;
+  // Set by tls.setDefaultCACertificates(). Once set, NewRootCertStore()
+  // copies these certificates instead of loading the defaults.
+  std::unique_ptr<X509Set> 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<Array> X509sToArrayOfStrings(Environment* env,
 
 void GetUserRootCertificates(const FunctionCallbackInfo<Value>& 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<Array> 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<Value>& args) {
   CHECK(args[0]->IsArray());
   Local<Array> cert_array = args[0].As<Array>();
   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<X509Set>();
+    root_certs->store.reset();
+    root_certs->certs_from_users = std::make_unique<X509Set>();
     return;
   }
 
@@ -1263,7 +1255,6 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& 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<Value>& 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<Value>& args) {

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<ncrypto::DigestCache>();
   provider_cipher_cache = std::make_unique<ncrypto::CipherCache>();
@@ -1256,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
@@ -1273,6 +1278,7 @@ Environment::~Environment() {
     cpu_profiler_->Dispose();
     cpu_profiler_ = nullptr;
   }
+  isolate_data_->RemoveEnvironment();
 }
 
 void Environment::InitializeLibuv() {
@@ -1436,6 +1442,9 @@ void Environment::ClosePerEnvHandles() {
   close_and_finish(reinterpret_cast<uv_handle_t*>(&task_queues_async_));
 }
 
+// NOLINTNEXTLINE(runtime/thread_local)
+thread_local int handle_cleanup_depth = 0;
+
 void Environment::CleanupHandles() {
   {
     Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_);
@@ -1453,8 +1462,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()) {

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;
 }
@@ -182,10 +189,8 @@ 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;
+  void AddEnvironment() { environment_count_++; }
+  void RemoveEnvironment() { environment_count_--; }
 
 #define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
 #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
@@ -288,6 +293,7 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
 
   std::shared_ptr<PerIsolateOptions> options_;
   worker::Worker* worker_context_ = nullptr;
+  size_t environment_count_ = 0;
   PerIsolateWrapperData* wrapper_data_;
 
   static Mutex isolate_data_mutex_;
@@ -1212,6 +1218,7 @@ class Environment final : public MemoryRetainer {
   std::unique_ptr<ncrypto::MacCache> provider_mac_cache;
   std::vector<std::string> supported_mac_algorithms;
   bool supported_mac_algorithms_initialized = false;
+  DeleteFnPtr<crypto::RootCertStore, crypto::FreeRootCertStore> root_cert_store;
 #endif  // HAVE_OPENSSL
 
   v8::Global<v8::Module> temporary_required_module_facade_original;

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<Value> 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

src/node.h

@@ -889,8 +889,8 @@ NODE_EXTERN v8::MaybeLocal<v8::Value> 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,

src/node_binding.cc

@@ -180,6 +180,7 @@ struct dl_wrap {
 static Mutex dlhandles_mutex;
 static std::unordered_set<dl_wrap*, dl_wrap::hash, dl_wrap::equal>
     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

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<FastStringKey, int, FastStringKey::Hash>
     generic_usage_counters;
+// NOLINTNEXTLINE(runtime/thread_local)
 thread_local std::unordered_map<FastStringKey, int, FastStringKey::Hash>
     v8_fast_api_call_counts;
 

src/node_errors.cc

@@ -190,9 +190,11 @@ static std::string GetErrorSource(Isolate* isolate,
 }
 
 static std::atomic<bool> is_in_oom{false};
+// NOLINTNEXTLINE(runtime/thread_local)
 static thread_local std::atomic<bool> 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<StackTrace> GetCurrentStackTrace(Isolate* isolate, int frame_count) {
   if (isolate == nullptr) {

src/node_internals.h

@@ -283,6 +283,12 @@ class InternalCallbackScope {
   std::optional<v8::Isolate::AllowJavascriptExecutionScope> 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.
+// NOLINTNEXTLINE(runtime/thread_local)
+extern thread_local int handle_cleanup_depth;
+
 class DebugSealHandleScope {
  public:
   explicit inline DebugSealHandleScope(v8::Isolate* isolate = nullptr)

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