Repository: nodejs/node
Pull request: 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. It's similar to how we handle the case when the two are both asynchronous.

- 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(). The evaluation, being synchronous, is guaranteed to complete by the time the other run() is about to start evaluation, the other task would just get the cached evaluation results instead of re-evaluating it, similar to what would happen if both are asynchronously imported.
- 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().

Refs: https://github.com/shufo/prettier-plugin-blade/issues/311
Refs: https://github.com/fisker/prettier-plugin-blade-311
Refs: https://github.com/mochajs/mocha/issues/5290
Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic
Fixes: https://github.com/nodejs/node/issues/57172
<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: lib/internal/modules/esm/loader.js
patch: @@ -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,15 @@ const {
   forceDefaultLoader,
 } = require('internal/modules/esm/utils');
 const { kImplicitTypeAttribute } = require('internal/modules/esm/assert');
-const { ModuleWrap, kEvaluating, kEvaluated, kEvaluationPhase, kSourcePhase } = internalBinding('module_wrap');
+const {
+  ModuleWrap,
+  kEvaluated,
+  kEvaluating,
+  kEvaluationPhase,
+  kInstantiated,
+  kSourcePhase,
+  throwIfPromiseRejected,
+} = internalBinding('module_wrap');
 const {
   urlToFilename,
 } = require('internal/modules/helpers');
@@ -53,6 +60,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 +97,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).
@@ -346,35 +374,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

filename: lib/internal/modules/esm/module_job.js
patch: @@ -274,7 +274,7 @@ class ModuleJob extends ModuleJobBase {
     }
   }
 
-  runSync() {
+  runSync(parent) {
     assert(this.phase === kEvaluationPhase);
     assert(this.module instanceof ModuleWrap);
     if (this.instantiated !== undefined) {
@@ -283,11 +283,11 @@ class ModuleJob extends ModuleJobBase {
 
     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) {

filename: src/module_wrap.cc
patch: @@ -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;
@@ -47,6 +48,7 @@ using v8::Module;
 using v8::ModuleImportPhase;
 using v8::ModuleRequest;
 using v8::Name;
+using v8::Nothing;
 using v8::Null;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -671,6 +673,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();
@@ -695,28 +734,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;
   }
 
@@ -1277,6 +1295,7 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data,
             target,
             "createRequiredModuleFacade",
             CreateRequiredModuleFacade);
+  SetMethod(isolate, target, "throwIfPromiseRejected", ThrowIfPromiseRejected);
 }
 
 void ModuleWrap::CreatePerContextProperties(Local<Object> target,
@@ -1326,6 +1345,7 @@ void ModuleWrap::RegisterExternalReferences(
 
   registry->Register(SetImportModuleDynamicallyCallback);
   registry->Register(SetInitializeImportMetaObjectCallback);
+  registry->Register(ThrowIfPromiseRejected);
 }
 }  // namespace loader
 }  // namespace node