module: handle cached linked async jobs in require(esm)

This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: #57187 Fixes: #57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
nodejs · joyeecheung · Oct 31, 2024 · Feb 18, 2025 · Feb 17, 2025 · Mar 4, 2025
commit d52043a41b7d3fe975eb7fd19df96d784617a726
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -36,12 +36,22 @@ const {
   getDefaultConditions,
 } = require('internal/modules/esm/utils');
 const { kImplicitAssertType } = require('internal/modules/esm/assert');
-const { ModuleWrap, kEvaluating, kEvaluated } = internalBinding('module_wrap');
+const {
+  ModuleWrap,
+  kEvaluated,
+  kEvaluating,
+  kInstantiated,
+  throwIfPromiseRejected,
+} = internalBinding('module_wrap');
 const {
   urlToFilename,
 } = require('internal/modules/helpers');
 let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;
 
+let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
+  debug = fn;
+});
+
 /**
  * @typedef {import('./hooks.js').HooksProxy} HooksProxy
  * @typedef {import('./module_job.js').ModuleJobBase} ModuleJobBase
@@ -75,6 +85,23 @@ function getTranslators() {
   return translators;
 }
 
+/**
+ * Generate message about potential race condition caused by requiring a cached module that has started
+ * async linking.
+ * @param {string} filename Filename of the module being required.
+ * @param {string|undefined} parentFilename Filename of the module calling require().
+ * @returns {string} Error message.
+ */
+function getRaceMessage(filename, parentFilename) {
+  let raceMessage = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `;
+  raceMessage += 'This may be caused by a race condition if the module is simultaneously dynamically ';
+  raceMessage += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.';
+  if (parentFilename) {
+    raceMessage += ` (from ${parentFilename})`;
+  }
+  return raceMessage;
+}
+
 /**
  * @type {HooksProxy}
  * Multiple loader instances exist for various, specific reasons (see code comments at site).
@@ -297,35 +324,53 @@ class ModuleLoader {
     //    evaluated at this point.
     // TODO(joyeecheung): add something similar to CJS loader's requireStack to help
     // debugging the the problematic links in the graph for import.
+    debug('importSyncForRequire', parent?.filename, '->', filename, job);
     if (job !== undefined) {
       mod[kRequiredModuleSymbol] = job.module;
       const parentFilename = urlToFilename(parent?.filename);
       // TODO(node:55782): this race may stop to happen when the ESM resolution and loading become synchronous.
       if (!job.module) {
-        let message = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `;
-        message += 'This may be caused by a race condition if the module is simultaneously dynamically ';
-        message += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.';
-        if (parentFilename) {
-          message += ` (from ${parentFilename})`;
-        }
-        assert(job.module, message);
+        assert.fail(getRaceMessage(filename, parentFilename));
       }
       if (job.module.async) {
         throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename);
       }
-      // job.module may be undefined if it's asynchronously loaded. Which means
-      // there is likely a cycle.
-      if (job.module.getStatus() !== kEvaluated) {
-        let message = `Cannot require() ES Module ${filename} in a cycle.`;
-        if (parentFilename) {
-          message += ` (from ${parentFilename})`;
-        }
-        message += 'A cycle involving require(esm) is disallowed to maintain ';
-        message += 'invariants madated by the ECMAScript specification';
-        message += 'Try making at least part of the dependency in the graph lazily loaded.';
-        throw new ERR_REQUIRE_CYCLE_MODULE(message);
+      const status = job.module.getStatus();
+      debug('Module status', filename, status);
+      if (status === kEvaluated) {
+        return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) };
+      } else if (status === kInstantiated) {
+        // When it's an async job cached by another import request,
+        // which has finished linking but has not started its
+        // evaluation because the async run() task would be later
+        // in line. Then start the evaluation now with runSync(), which
+        // is guaranteed to finish by the time the other run() get to it,
+        // and the other task would just get the cached evaluation results,
+        // similar to what would happen when both are async.
+        mod[kRequiredModuleSymbol] = job.module;
+        const { namespace } = job.runSync(parent);
+        return { wrap: job.module, namespace: namespace || job.module.getNamespace() };
       }
-      return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) };
+      // When the cached async job have already encountered a linking
+      // error that gets wrapped into a rejection, but is still later
+      // in line to throw on it, just unwrap and throw the linking error
+      // from require().
+      if (job.instantiated) {
+        throwIfPromiseRejected(job.instantiated);
+      }
+      if (status !== kEvaluating) {
+        assert.fail(`Unexpected module status ${status}. ` +
+                    getRaceMessage(filename, parentFilename));
+      }
+      let message = `Cannot require() ES Module ${filename} in a cycle.`;
+      if (parentFilename) {
+        message += ` (from ${parentFilename})`;
+      }
+      message += 'A cycle involving require(esm) is disallowed to maintain ';
+      message += 'invariants madated by the ECMAScript specification';
+      message += 'Try making at least part of the dependency in the graph lazily loaded.';
+      throw new ERR_REQUIRE_CYCLE_MODULE(message);
+
     }
     // TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the
     // cache here, or use a carrier object to carry the compiled module script

diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
@@ -240,19 +240,19 @@ class ModuleJob extends ModuleJobBase {
     }
   }
 
-  runSync() {
+  runSync(parent) {
     assert(this.module instanceof ModuleWrap);
     if (this.instantiated !== undefined) {
       return { __proto__: null, module: this.module };
     }
 
     this.module.instantiate();
     this.instantiated = PromiseResolve();
-    const timeout = -1;
-    const breakOnSigint = false;
     setHasStartedUserESMExecution();
-    this.module.evaluate(timeout, breakOnSigint);
-    return { __proto__: null, module: this.module };
+    const filename = urlToFilename(this.url);
+    const parentFilename = urlToFilename(parent?.filename);
+    const namespace = this.module.evaluateSync(filename, parentFilename);
+    return { __proto__: null, module: this.module, namespace };
   }
 
   async run() {

diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -24,6 +24,7 @@ using v8::Array;
 using v8::ArrayBufferView;
 using v8::Context;
 using v8::EscapableHandleScope;
+using v8::Exception;
 using v8::FixedArray;
 using v8::Function;
 using v8::FunctionCallbackInfo;
@@ -32,15 +33,22 @@ using v8::HandleScope;
 using v8::Int32;
 using v8::Integer;
 using v8::Isolate;
+using v8::JustVoid;
 using v8::Local;
+using v8::Maybe;
 using v8::MaybeLocal;
+using v8::Message;
 using v8::MicrotaskQueue;
 using v8::Module;
 using v8::ModuleRequest;
+using v8::Name;
+using v8::Nothing;
+using v8::Null;
 using v8::Object;
 using v8::ObjectTemplate;
 using v8::PrimitiveArray;
 using v8::Promise;
+using v8::PromiseRejectEvent;
 using v8::ScriptCompiler;
 using v8::ScriptOrigin;
 using v8::String;
@@ -584,6 +592,43 @@ void ModuleWrap::InstantiateSync(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(module->IsGraphAsync());
 }
 
+Maybe<void> ThrowIfPromiseRejected(Realm* realm, Local<Promise> promise) {
+  Isolate* isolate = realm->isolate();
+  Local<Context> context = realm->context();
+  if (promise->State() != Promise::PromiseState::kRejected) {
+    return JustVoid();
+  }
+  // The rejected promise is created by V8, so we don't get a chance to mark
+  // it as resolved before the rejection happens from evaluation. But we can
+  // tell the promise rejection callback to treat it as a promise rejected
+  // before handler was added which would remove it from the unhandled
+  // rejection handling, since we are converting it into an error and throw
+  // from here directly.
+  Local<Value> type =
+      Integer::New(isolate,
+                   static_cast<int32_t>(
+                       PromiseRejectEvent::kPromiseHandlerAddedAfterReject));
+  Local<Value> args[] = {type, promise, Undefined(isolate)};
+  if (realm->promise_reject_callback()
+          ->Call(context, Undefined(isolate), arraysize(args), args)
+          .IsEmpty()) {
+    return Nothing<void>();
+  }
+  Local<Value> exception = promise->Result();
+  Local<Message> message = Exception::CreateMessage(isolate, exception);
+  AppendExceptionLine(
+      realm->env(), exception, message, ErrorHandlingMode::MODULE_ERROR);
+  isolate->ThrowException(exception);
+  return Nothing<void>();
+}
+
+void ThrowIfPromiseRejected(const FunctionCallbackInfo<Value>& args) {
+  if (!args[0]->IsPromise()) {
+    return;
+  }
+  ThrowIfPromiseRejected(Realm::GetCurrent(args), args[0].As<Promise>());
+}
+
 void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
   Realm* realm = Realm::GetCurrent(args);
   Isolate* isolate = args.GetIsolate();
@@ -608,29 +653,7 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
 
   CHECK(result->IsPromise());
   Local<Promise> promise = result.As<Promise>();
-  if (promise->State() == Promise::PromiseState::kRejected) {
-    // The rejected promise is created by V8, so we don't get a chance to mark
-    // it as resolved before the rejection happens from evaluation. But we can
-    // tell the promise rejection callback to treat it as a promise rejected
-    // before handler was added which would remove it from the unhandled
-    // rejection handling, since we are converting it into an error and throw
-    // from here directly.
-    Local<Value> type = v8::Integer::New(
-        isolate,
-        static_cast<int32_t>(
-            v8::PromiseRejectEvent::kPromiseHandlerAddedAfterReject));
-    Local<Value> args[] = {type, promise, Undefined(isolate)};
-    if (env->promise_reject_callback()
-            ->Call(context, Undefined(isolate), arraysize(args), args)
-            .IsEmpty()) {
-      return;
-    }
-    Local<Value> exception = promise->Result();
-    Local<v8::Message> message =
-        v8::Exception::CreateMessage(isolate, exception);
-    AppendExceptionLine(
-        env, exception, message, ErrorHandlingMode::MODULE_ERROR);
-    isolate->ThrowException(exception);
+  if (ThrowIfPromiseRejected(realm, promise).IsNothing()) {
     return;
   }
 
@@ -1083,6 +1106,7 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data,
             target,
             "createRequiredModuleFacade",
             CreateRequiredModuleFacade);
+  SetMethod(isolate, target, "throwIfPromiseRejected", ThrowIfPromiseRejected);
 }
 
 void ModuleWrap::CreatePerContextProperties(Local<Object> target,
@@ -1127,6 +1151,7 @@ void ModuleWrap::RegisterExternalReferences(
 
   registry->Register(SetImportModuleDynamicallyCallback);
   registry->Register(SetInitializeImportMetaObjectCallback);
+  registry->Register(ThrowIfPromiseRejected);
 }
 }  // namespace loader
 }  // namespace node