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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

Author: joyeecheung

lib/internal/modules/esm/loader.js

@@ -18,7 +18,6 @@ const {
   kIsExecuting,
   kRequiredModuleSymbol,
 } = require('internal/modules/cjs/loader');
-
 const { imported_cjs_symbol } = internalBinding('symbols');
 
 const assert = require('internal/assert');
@@ -38,7 +37,13 @@ const {
   forceDefaultLoader,
 } = require('internal/modules/esm/utils');
 const { kImplicitTypeAttribute } = 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');
@@ -53,6 +58,10 @@ let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;
 const { tracingChannel } = require('diagnostics_channel');
 const onImport = tracingChannel('module.import');
 
+let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
+  debug = fn;
+});
+
 /**
  * @typedef {import('./hooks.js').HooksProxy} HooksProxy
  * @typedef {import('./module_job.js').ModuleJobBase} ModuleJobBase
@@ -86,6 +95,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).
@@ -340,35 +366,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

lib/internal/modules/esm/module_job.js

@@ -22,7 +22,7 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
   debug = fn;
 });
 
-const { ModuleWrap, kInstantiated } = internalBinding('module_wrap');
+const { ModuleWrap, kEvaluationPhase, kInstantiated } = internalBinding('module_wrap');
 const {
   privateSymbols: {
     entry_point_module_private_symbol,
@@ -246,19 +246,20 @@ class ModuleJob extends ModuleJobBase {
     }
   }
 
-  runSync() {
+  runSync(parent) {
+    assert(this.phase === kEvaluationPhase);
     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(isEntryPoint = false) {

src/module_wrap.cc

@@ -36,6 +36,7 @@ using v8::Int32;
 using v8::Integer;
 using v8::Isolate;
 using v8::Just;
+using v8::JustVoid;
 using v8::Local;
 using v8::LocalVector;
 using v8::Maybe;
@@ -46,6 +47,7 @@ using v8::MicrotaskQueue;
 using v8::Module;
 using v8::ModuleRequest;
 using v8::Name;
+using v8::Nothing;
 using v8::Null;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -653,6 +655,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();
@@ -677,28 +716,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 =
-        Integer::New(isolate,
-                     static_cast<int32_t>(
-                         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<Message> message = Exception::CreateMessage(isolate, exception);
-    AppendExceptionLine(
-        env, exception, message, ErrorHandlingMode::MODULE_ERROR);
-    isolate->ThrowException(exception);
+  if (ThrowIfPromiseRejected(realm, promise).IsNothing()) {
     return;
   }
 
@@ -1152,6 +1170,7 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data,
             target,
             "createRequiredModuleFacade",
             CreateRequiredModuleFacade);
+  SetMethod(isolate, target, "throwIfPromiseRejected", ThrowIfPromiseRejected);
 }
 
 void ModuleWrap::CreatePerContextProperties(Local<Object> target,
@@ -1196,6 +1215,7 @@ void ModuleWrap::RegisterExternalReferences(
 
   registry->Register(SetImportModuleDynamicallyCallback);
   registry->Register(SetInitializeImportMetaObjectCallback);
+  registry->Register(ThrowIfPromiseRejected);
 }
 }  // namespace loader
 }  // namespace node