make worker instantiation thread safe + some other review findings

Author: dygabo

lib/internal/modules/esm/hooks.js

@@ -515,12 +515,15 @@ class HooksProxy {
   #isWorkerOwner = false;
 
   constructor() {
-    const { InternalWorker, hooksPort, hasHooksThread } = require('internal/worker');
+    const { InternalWorker, hooksPort } = require('internal/worker');
     const lock = new SharedArrayBuffer(SHARED_MEMORY_BYTE_LENGTH);
     this.#lock = new Int32Array(lock);
 
-    if (!hasHooksThread()) {
-      // Main thread is the only one that creates the internal single hooks worker
+    try {
+      // The customization hooks thread is only created once. All other threads reuse
+      // the existing instance. If another thread created it, the constructor will throw
+      // Fallback is to use the existing hooksPort created by the thread that originally
+      // spawned the customization hooks thread.
       this.#worker = new InternalWorker(loaderWorkerId, {
         stderr: false,
         stdin: false,
@@ -534,16 +537,21 @@ class HooksProxy {
       this.#worker.on('exit', process.exit);
       this.#isWorkerOwner = true;
       this.#portToHooksThread = this.#worker;
-    } else {
-      this.#portToHooksThread = hooksPort;
+    } catch (e) {
+      if (e.code === 'ERR_HOOKS_THREAD_EXISTS') {
+        this.#portToHooksThread = hooksPort;
+      } else {
+        throw e;
+      }
     }
   }
 
   waitForWorker() {
-    // There is one Hooks instance for each worker thread. But only one of these Hooks instances
-    // has an InternalWorker. That was the Hooks instance created for the main thread.
-    // It means for all Hooks instances that are not on the main thread => they are ready because they
-    // delegate to the single InternalWorker anyway.
+    // There is one Hooks instance for each worker thread. But only one of
+    // these Hooks instances has an InternalWorker. That was the Hooks instance
+    // created for the first thread that registers a hook set.
+    // It means for all Hooks instances that are not on that thread => they are
+    // done because they delegate to the single InternalWorker.
     if (!this.#isWorkerOwner) {
       return;
     }

lib/internal/modules/esm/loader.js

@@ -622,8 +622,6 @@ class CustomizedModuleLoader {
    * @returns {{ format: string, url: URL['href'] } | undefined}
    */
   register(originalSpecifier, parentURL, data, transferList) {
-    // Only the main thread has a Hooks instance with worker thread. All other Worker threads
-    // delegate their hooks to the HooksThread of the main thread.
     return hooksProxy.makeSyncRequest('register', transferList, originalSpecifier, parentURL, data);
   }
 

src/node_errors.h

@@ -70,6 +70,7 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
   V(ERR_DLOPEN_FAILED, Error)                                                  \
   V(ERR_ENCODING_INVALID_ENCODED_DATA, TypeError)                              \
   V(ERR_EXECUTION_ENVIRONMENT_NOT_AVAILABLE, Error)                            \
+  V(ERR_HOOKS_THREAD_EXISTS, Error)                                            \
   V(ERR_ILLEGAL_CONSTRUCTOR, Error)                                            \
   V(ERR_INVALID_ADDRESS, Error)                                                \
   V(ERR_INVALID_ARG_VALUE, TypeError)                                          \

src/node_worker.cc

@@ -47,6 +47,7 @@ namespace worker {
 
 constexpr double kMB = 1024 * 1024;
 std::atomic_bool Worker::internalExists{false};
+Mutex Worker::instantiationMutex;
 
 Worker::Worker(Environment* env,
                Local<Object> wrap,
@@ -484,18 +485,28 @@ Worker::~Worker() {
 }
 
 void Worker::New(const FunctionCallbackInfo<Value>& args) {
+  Mutex::ScopedLock lock(instantiationMutex);
   Environment* env = Environment::GetCurrent(args);
   auto is_internal = args[5];
   CHECK(is_internal->IsBoolean());
   if (is_internal->IsFalse()) {
     THROW_IF_INSUFFICIENT_PERMISSIONS(
         env, permission::PermissionScope::kWorkerThreads, "");
-  } else {
-    internalExists = true;
   }
   Isolate* isolate = args.GetIsolate();
 
   CHECK(args.IsConstructCall());
+  auto creatingHooksThread = is_internal->IsTrue();
+
+  if (creatingHooksThread && internalExists) {
+    isolate->ThrowException(ERR_HOOKS_THREAD_EXISTS(
+        isolate, "Customization hooks thread already exists"));
+    return;
+  }
+
+  if (creatingHooksThread) {
+    internalExists = true;
+  }
 
   if (env->isolate_data()->platform() == nullptr) {
     THROW_ERR_MISSING_PLATFORM_FOR_WORKER(env);

src/node_worker.h

@@ -105,6 +105,8 @@ class Worker : public AsyncWrap {
   // Optional name used for debugging in inspector and trace events.
   std::string name_;
   static std::atomic_bool internalExists;
+  // this mutex is to synchronize ::New calls
+  static Mutex instantiationMutex;
 
   // Custom resource constraints:
   double resource_limits_[kTotalResourceLimitCount];