src: make BuiltinLoader threadsafe and non-global

As discussed in #45888, using a global `BuiltinLoader` instance is probably undesirable in a world in which embedders are able to create Node.js Environments with different sources and therefore mutually incompatible code caching properties. This PR makes it so that `BuiltinLoader` is no longer a global singleton and instead only shared between `Environment`s that have a direct relation to each other, and addresses a few thread safety issues along with that.
nodejs · nodejs-github-bot · Jan 18, 2023 · Dec 22, 2022 · Jan 6, 2023 · Jan 6, 2023
commit be7dcbab7de1178e31e563b34d8eb38d43414ddd
diff --git a/src/api/environment.cc b/src/api/environment.cc
@@ -473,7 +473,7 @@ MaybeLocal<Value> LoadEnvironment(
   return LoadEnvironment(
       env, [&](const StartExecutionCallbackInfo& info) -> MaybeLocal<Value> {
         std::string name = "embedder_main_" + std::to_string(env->thread_id());
-        builtins::BuiltinLoader::Add(name.c_str(), main_script_source_utf8);
+        env->builtin_loader()->Add(name.c_str(), main_script_source_utf8);
         Realm* realm = env->principal_realm();
 
         return realm->ExecuteBootstrapper(name.c_str());
@@ -714,10 +714,12 @@ Maybe<bool> InitializePrimordials(Local<Context> context) {
                                         "internal/per_context/messageport",
                                         nullptr};
 
+  auto builtin_loader = builtins::BuiltinLoader::Create();
   for (const char** module = context_files; *module != nullptr; module++) {
     Local<Value> arguments[] = {exports, primordials};
-    if (builtins::BuiltinLoader::CompileAndCall(
-            context, *module, arraysize(arguments), arguments, nullptr)
+    if (builtin_loader
+            ->CompileAndCall(
+                context, *module, arraysize(arguments), arguments, nullptr)
             .IsEmpty()) {
       // Execution failed during context creation.
       return Nothing<bool>();

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -436,6 +436,16 @@ inline std::vector<double>* Environment::destroy_async_id_list() {
   return &destroy_async_id_list_;
 }
 
+inline std::shared_ptr<builtins::BuiltinLoader> Environment::builtin_loader() {
+  DCHECK(builtin_loader_);
+  return builtin_loader_;
+}
+
+inline void Environment::set_builtin_loader(
+    std::shared_ptr<builtins::BuiltinLoader> loader) {
+  builtin_loader_ = loader;
+}
+
 inline double Environment::new_async_id() {
   async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter] += 1;
   return async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter];

diff --git a/src/env.cc b/src/env.cc
@@ -752,6 +752,8 @@ Environment::Environment(IsolateData* isolate_data,
                   env_info,
                   flags,
                   thread_id) {
+  // TODO(addaleax): Make this part of CreateEnvironment().
+  set_builtin_loader(builtins::BuiltinLoader::Create());
   InitializeMainContext(context, env_info);
 }
 

diff --git a/src/env.h b/src/env.h
@@ -721,6 +721,9 @@ class Environment : public MemoryRetainer {
   // List of id's that have been destroyed and need the destroy() cb called.
   inline std::vector<double>* destroy_async_id_list();
 
+  std::shared_ptr<builtins::BuiltinLoader> builtin_loader();
+  void set_builtin_loader(std::shared_ptr<builtins::BuiltinLoader> loader);
+
   std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
   std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
   std::unordered_map<uint32_t, contextify::ContextifyScript*>
@@ -1144,6 +1147,8 @@ class Environment : public MemoryRetainer {
 
   std::unique_ptr<Realm> principal_realm_ = nullptr;
 
+  std::shared_ptr<builtins::BuiltinLoader> builtin_loader_;
+
   // Used by allocate_managed_buffer() and release_managed_buffer() to keep
   // track of the BackingStore for a given pointer.
   std::unordered_map<char*, std::unique_ptr<v8::BackingStore>>

diff --git a/src/node.cc b/src/node.cc
@@ -1183,9 +1183,6 @@ ExitCode LoadSnapshotDataAndRun(const SnapshotData** snapshot_data_ptr,
     }
   }
 
-  if ((*snapshot_data_ptr) != nullptr) {
-    BuiltinLoader::RefreshCodeCache((*snapshot_data_ptr)->code_cache);
-  }
   NodeMainInstance main_instance(*snapshot_data_ptr,
                                  uv_default_loop(),
                                  per_process::v8_platform.Platform(),

diff --git a/src/node_binding.cc b/src/node_binding.cc
@@ -631,13 +631,13 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
     CHECK(exports->SetPrototype(context, Null(isolate)).FromJust());
     DefineConstants(isolate, exports);
   } else if (!strcmp(*module_v, "natives")) {
-    exports = builtins::BuiltinLoader::GetSourceObject(context);
+    exports = realm->env()->builtin_loader()->GetSourceObject(context);
     // Legacy feature: process.binding('natives').config contains stringified
     // config.gypi
     CHECK(exports
               ->Set(context,
                     realm->isolate_data()->config_string(),
-                    builtins::BuiltinLoader::GetConfigString(isolate))
+                    realm->env()->builtin_loader()->GetConfigString(isolate))
               .FromJust());
   } else {
     return THROW_ERR_INVALID_MODULE(isolate, "No such binding: %s", *module_v);