`require(esm)` with top level error taints future `await import(esm)` swallowing errors

[samuelhnrq]

Version

Reproductible in v24.1.0 and v24.2.0

Platform

Linux xnvwjs 6.1.43 #1 SMP PREEMPT_DYNAMIC Sun Aug  6 20:05:33 UTC 2023 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Code sandbox with MRE: https://codesandbox.io/p/devbox/xnvwjs

but here it goes anyway:

mkdir repro
(
    cd repro
    echo 'throw globalThis.err; export const foo=2' > bad-esm.mjs
    cat -> entry.cjs <<'EOF'
    'use strict';
    const assert = require('node:assert');
    globalThis.err = new Error;
    assert.throws(() => require('./bad-esm.mjs'), globalThis.err);
    assert.rejects(import('./bad-esm.mjs').then(console.log), globalThis.err);
EOF
    node entry.cjs
)
rm -rf repro

How often does it reproduce? Is there a required condition?

100% of the times, no preconditons

What is the expected behavior? Why is that the expected behavior?

require(esm) should not affect future await import(esm), if given file throws error on import it should keep throwing again, regardless of times or the way its imported. As documented here

What do you see instead?

[Module: null prototype] { foo: <uninitialized> }
node:internal/process/promises:332
    triggerUncaughtException(err, true /* fromPromise */);
    ^

[AssertionError [ERR_ASSERTION]: Missing expected rejection (Error).]

Calling require(ESM) on a file with a top level throw/error makes all future import(esm) of the same file fail silently and return garbage (partial module definition I think)

Additional information

I found the error while investigating mochajs/mocha#5396

[aduh95]

I'm able to reproduce on main.

Also we have an ERR_INTERNAL_ASSERTION if the require call happens after the import() has settled:

mkdir repro
(
    cd repro
    echo 'throw globalThis.err; export const foo=2' > bad-esm.mjs
    cat -> entry.cjs <<'EOF'
    'use strict';
    const assert = require('node:assert');
    globalThis.err = new Error;
    (async () => {
    await assert.rejects(import('./bad-esm.mjs'), globalThis.err);
    require('./bad-esm.mjs')
    })();
EOF
    node entry.cjs
)
rm -rf repro

Error [ERR_INTERNAL_ASSERTION]: Unexpected module status 5. Cannot require() ES Module …/repro/bad-esm.mjs because it is not yet fully loaded. This may be caused by a race condition if the module is simultaneously dynamically import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead. (from …/repro/entry.cjs)
This is caused by either a bug in Node.js or incorrect usage of Node.js internals.

/cc @joyeecheung

[joyeecheung]

Thanks for reporting - looks like another caching inconsistency and we need to keep the rejection for future root evaluations (or if V8 supports it, just re-evaluate to get the rejection again) to surface it.

Intuitively it makes sense to me to actually evaluate the code in the modules again instead of not even attempting to run the (non-erroring) code on the second try. Though I guess for a retry strategy like what mocha is doing they might need to be careful about the code being run twice.

[samuelhnrq]

Thanks for confirming and finding out the extra edge case @aduh95 🙏

Hey @joyeecheung I'd like to have a shot at fixing this, what is the ideal/expected behavior here?

1. Properly populating the ESM import() cache with the same exception so the future import() just re-throws the same exception generated from the original require(esm)
   • But then require(esm) would be touching import module cache not sure if there is any problem with that...
2. Make sure to cleanup the side effects of the require(esm) so the next import actually evaluates the module again?
   • But then we then expose of running top level side effects twice

The first seems cleaner functionality wise in exchange for a little of internal mess 🤷

[joyeecheung]

diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
index 2d45f404a68..33481cd93cf 100644
--- a/lib/internal/modules/esm/loader.js
+++ b/lib/internal/modules/esm/loader.js
@@ -43,6 +43,7 @@ const {
   kEvaluating,
   kEvaluationPhase,
   kInstantiated,
+  kErrored,
   kSourcePhase,
   throwIfPromiseRejected,
 } = internalBinding('module_wrap');
@@ -402,6 +403,8 @@ class ModuleLoader {
         mod[kRequiredModuleSymbol] = job.module;
         const { namespace } = job.runSync(parent);
         return { wrap: job.module, namespace: namespace || job.module.getNamespace() };
+      } else if (status === kErrored) {
+        throw job.module.getError();
       }
       // When the cached async job have already encountered a linking
       // error that gets wrapped into a rejection, but is still later
diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
index a7a65e50a16..8f665362384 100644
--- a/lib/internal/modules/esm/module_job.js
+++ b/lib/internal/modules/esm/module_job.js
@@ -325,7 +325,7 @@ class ModuleJob extends ModuleJobBase {
     assert(this.module instanceof ModuleWrap);
     let status = this.module.getStatus();

-    debug('ModuleJob.runSync', this.module);
+    debug('ModuleJob.runSync()', status, this.module);
     // FIXME(joyeecheung): this cannot fully handle < kInstantiated. Make the linking
     // fully synchronous instead.
     if (status === kUninstantiated) {
@@ -360,6 +360,7 @@ class ModuleJob extends ModuleJobBase {
   }

   async run(isEntryPoint = false) {
+    debug('ModuleJob.run()', this.module);
     assert(this.phase === kEvaluationPhase);
     await this.#instantiate();
     if (isEntryPoint) {
@@ -465,7 +466,9 @@ class ModuleJobSync extends ModuleJobBase {
     assert(this.phase === kEvaluationPhase);
     // This path is hit by a require'd module that is imported again.
     const status = this.module.getStatus();
-    if (status > kInstantiated) {
+    if (status === kErrored) {
+      throw this.module.getError();
+    } else if (status > kInstantiated) {
       if (this.evaluationPromise) {
         await this.evaluationPromise;
       }
@@ -486,6 +489,7 @@ class ModuleJobSync extends ModuleJobBase {
   }

   runSync(parent) {
+    debug('ModuleJobSync.runSync()', this.module);
     assert(this.phase === kEvaluationPhase);
     // TODO(joyeecheung): add the error decoration logic from the async instantiate.
     this.module.async = this.module.instantiateSync();

Locally this diff fixes both but I'll need to think a bit about whether the code should be run again..

[joyeecheung]

Actually for an errored root module there is no way to run the code again, V8 would just cache the error to throw on the second evaluation, which I think is from the spec. So we should just throw.