module: throw error when re-runing errored module jobs

Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs. PR-URL: #58957 Backport-PR-URL: #59504 Fixes: #58945 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Refs: #52697

Author: joyeecheung

lib/internal/modules/esm/loader.js

@@ -41,6 +41,7 @@ const {
  kEvaluated,
  kEvaluating,
  kInstantiated,
+ kErrored,
  throwIfPromiseRejected,
 } = internalBinding('module_wrap');
 const {
@@ -350,6 +351,9 @@ class ModuleLoader {
  mod[kRequiredModuleSymbol] = job.module;
  const { namespace } = job.runSync(parent);
  return { wrap: job.module, namespace: namespace || job.module.getNamespace() };
+ } else if (status === kErrored) {
+ // If the module was previously imported and errored, throw the error.
+ 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

lib/internal/modules/esm/module_job.js

@@ -281,7 +281,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) {
@@ -316,6 +316,7 @@ class ModuleJob extends ModuleJobBase {
  }
 
  async run() {
+ debug('ModuleJob.run()', this.module);
  await this.instantiate();
  const timeout = -1;
  const breakOnSigint = false;
@@ -392,7 +393,11 @@ class ModuleJobSync extends ModuleJobBase {
  async run() {
  // This path is hit by a require'd module that is imported again.
  const status = this.module.getStatus();
- if (status > kInstantiated) {
+ debug('ModuleJobSync.run()', status, this.module);
+ // If the module was previously required and errored, reject from import() again.
+ if (status === kErrored) {
+ throw this.module.getError();
+ } else if (status > kInstantiated) {
  if (this.evaluationPromise) {
  await this.evaluationPromise;
  }
@@ -413,6 +418,7 @@ class ModuleJobSync extends ModuleJobBase {
  }
 
  runSync(parent) {
+ debug('ModuleJobSync.runSync()', this.module);
  // TODO(joyeecheung): add the error decoration logic from the async instantiate.
  this.module.async = this.module.instantiateSync();
  // If --experimental-print-required-tla is true, proceeds to evaluation even

test/es-module/test-import-module-retry-require-errored.js

@@ -0,0 +1,17 @@
+// This tests that after failing to import an ESM that rejects,
+// retrying with require() still throws.
+
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+(async () => {
+ await assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), {
+ message: 'test',
+ });
+ assert.throws(() => {
+ require('../fixtures/es-modules/throw-error.mjs');
+ }, {
+ message: 'test',
+ });
+})().catch(common.mustNotCall());

test/es-module/test-require-module-retry-import-errored-2.js

@@ -0,0 +1,16 @@
+// This tests that after failing to require an ESM that throws,
+// retrying with import() still rejects.
+
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+assert.throws(() => {
+ require('../fixtures/es-modules/throw-error.mjs');
+}, {
+ message: 'test',
+});
+
+assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), {
+ message: 'test',
+}).catch(common.mustNotCall());