From bbd4cb487a526762b403db7aeb5349e73b102f12 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 23 Oct 2024 18:26:43 +0200 Subject: [PATCH 1/2] module: allow ESM that failed to be required to be re-imported When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. --- lib/internal/modules/esm/module_job.js | 19 +++++++++++--- .../test-require-module-tla-retry-import-2.js | 26 +++++++++++++++++++ .../test-require-module-tla-retry-import.js | 25 ++++++++++++++++++ test/fixtures/es-modules/tla/export-async.mjs | 2 ++ 4 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-require-module-tla-retry-import-2.js create mode 100644 test/es-module/test-require-module-tla-retry-import.js create mode 100644 test/fixtures/es-modules/tla/export-async.mjs diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 413fcd27703e3e..e9e77e7f80e60d 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -22,7 +22,7 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { debug = fn; }); -const { ModuleWrap, kEvaluated } = internalBinding('module_wrap'); +const { ModuleWrap, kEvaluated, kInstantiated } = internalBinding('module_wrap'); const { privateSymbols: { entry_point_module_private_symbol, @@ -354,10 +354,21 @@ class ModuleJobSync extends ModuleJobBase { } async run() { + // This path is hit by a require'd module that is imported again. const status = this.module.getStatus(); - assert(status === kEvaluated, - `A require()-d module that is imported again must be evaluated. Status = ${status}`); - return { __proto__: null, module: this.module }; + if (status === kEvaluated) { + return { __proto__: null, module: this.module }; + } else if (status === kInstantiated) { + // The evaluation may have been canceled because instantiateSync() detected TLA first. + // But when it is imported again, it's fine to re-evaluate it asynchronously. + const timeout = -1; + const breakOnSigint = false; + await this.module.evaluate(timeout, breakOnSigint); + return { __proto__: null, module: this.module }; + } + + assert.fail('Unexpected status of a module that is imported again after being required. ' + + `Status = ${status}`); } runSync() { diff --git a/test/es-module/test-require-module-tla-retry-import-2.js b/test/es-module/test-require-module-tla-retry-import-2.js new file mode 100644 index 00000000000000..aa9c344dd398d7 --- /dev/null +++ b/test/es-module/test-require-module-tla-retry-import-2.js @@ -0,0 +1,26 @@ +// This tests that after loading a ESM with import() and then retrying +// with require(), it errors as expected, and produces consistent results. +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +let ns; +async function test() { + const newNs = await import('../fixtures/es-modules/tla/export-async.mjs'); + if (ns === undefined) { + ns = newNs; + } else { + // Check that re-evalaution is returning the same namespace. + assert.strictEqual(ns, newNs); + } + assert.strictEqual(ns.hello, 'world'); + + assert.throws(() => { + require('../fixtures/es-modules/tla/export-async.mjs'); + }, { + code: 'ERR_REQUIRE_ASYNC_MODULE' + }); +} + +// Run the test twice to check consistency after caching. +test().then(common.mustCall(test)).catch(common.mustNotCall()); diff --git a/test/es-module/test-require-module-tla-retry-import.js b/test/es-module/test-require-module-tla-retry-import.js new file mode 100644 index 00000000000000..70f918fa4f463f --- /dev/null +++ b/test/es-module/test-require-module-tla-retry-import.js @@ -0,0 +1,25 @@ +// This tests that after failing to require an ESM that contains TLA, +// retrying with import() still works, and produces consistent results. +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +let ns; +async function test() { + assert.throws(() => { + require('../fixtures/es-modules/tla/export-async.mjs'); + }, { + code: 'ERR_REQUIRE_ASYNC_MODULE' + }); + const newNs = await import('../fixtures/es-modules/tla/export-async.mjs'); + if (ns === undefined) { + ns = newNs; + } else { + // Check that re-evalaution is returning the same namespace. + assert.strictEqual(ns, newNs); + } + assert.strictEqual(ns.hello, 'world'); +} + +// Run the test twice to check consistency after caching. +test().then(common.mustCall(test)).catch(common.mustNotCall()); diff --git a/test/fixtures/es-modules/tla/export-async.mjs b/test/fixtures/es-modules/tla/export-async.mjs new file mode 100644 index 00000000000000..b6de9a5a5f68e9 --- /dev/null +++ b/test/fixtures/es-modules/tla/export-async.mjs @@ -0,0 +1,2 @@ +let hello = await Promise.resolve('world'); +export { hello }; From b0c551c065e15abeaaa314a0c143a7672d3e0bfb Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 24 Oct 2024 14:14:41 +0200 Subject: [PATCH 2/2] fixup! module: allow ESM that failed to be required to be re-imported --- lib/internal/modules/esm/module_job.js | 11 ++++-- ...est-require-module-retry-import-errored.js | 35 +++++++++++++++++++ ...-require-module-retry-import-evaluating.js | 32 +++++++++++++++++ .../es-modules/tla/await-export-promise.mjs | 4 +++ .../es-modules/tla/export-promise.mjs | 8 +++++ 5 files changed, 87 insertions(+), 3 deletions(-) create mode 100644 test/es-module/test-require-module-retry-import-errored.js create mode 100644 test/es-module/test-require-module-retry-import-evaluating.js create mode 100644 test/fixtures/es-modules/tla/await-export-promise.mjs create mode 100644 test/fixtures/es-modules/tla/export-promise.mjs diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index e9e77e7f80e60d..ece30c3864d6a7 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -22,7 +22,7 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { debug = fn; }); -const { ModuleWrap, kEvaluated, kInstantiated } = internalBinding('module_wrap'); +const { ModuleWrap, kInstantiated } = internalBinding('module_wrap'); const { privateSymbols: { entry_point_module_private_symbol, @@ -356,14 +356,19 @@ 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 === kEvaluated) { + if (status > kInstantiated) { + if (this.evaluationPromise) { + await this.evaluationPromise; + } return { __proto__: null, module: this.module }; } else if (status === kInstantiated) { // The evaluation may have been canceled because instantiateSync() detected TLA first. // But when it is imported again, it's fine to re-evaluate it asynchronously. const timeout = -1; const breakOnSigint = false; - await this.module.evaluate(timeout, breakOnSigint); + this.evaluationPromise = this.module.evaluate(timeout, breakOnSigint); + await this.evaluationPromise; + this.evaluationPromise = undefined; return { __proto__: null, module: this.module }; } diff --git a/test/es-module/test-require-module-retry-import-errored.js b/test/es-module/test-require-module-retry-import-errored.js new file mode 100644 index 00000000000000..4736087d2624a5 --- /dev/null +++ b/test/es-module/test-require-module-retry-import-errored.js @@ -0,0 +1,35 @@ +// This tests that after failing to require an ESM that contains TLA, +// retrying with import() still works, and produces consistent results. +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +const { exportedReject } = require('../fixtures/es-modules/tla/export-promise.mjs'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/await-export-promise.mjs'); +}, { + code: 'ERR_REQUIRE_ASYNC_MODULE' +}); + +const interval = setInterval(() => {}, 1000); // Keep the test running, because await alone doesn't. +const err = new Error('rejected'); + +const p1 = import('../fixtures/es-modules/tla/await-export-promise.mjs') + .then(common.mustNotCall(), common.mustCall((e) => { + assert.strictEqual(e, err); + })); + +const p2 = import('../fixtures/es-modules/tla/await-export-promise.mjs') + .then(common.mustNotCall(), common.mustCall((e) => { + assert.strictEqual(e, err); + })); + +const p3 = import('../fixtures/es-modules/tla/await-export-promise.mjs') + .then(common.mustNotCall(), common.mustCall((e) => { + assert.strictEqual(e, err); + })); + +exportedReject(err); + +Promise.all([p1, p2, p3]).then(common.mustCall(() => { clearInterval(interval); })); diff --git a/test/es-module/test-require-module-retry-import-evaluating.js b/test/es-module/test-require-module-retry-import-evaluating.js new file mode 100644 index 00000000000000..df70432f1136e4 --- /dev/null +++ b/test/es-module/test-require-module-retry-import-evaluating.js @@ -0,0 +1,32 @@ +// This tests that after failing to require an ESM that contains TLA, +// retrying with import() still works, and produces consistent results. +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +const { exportedResolve } = require('../fixtures/es-modules/tla/export-promise.mjs'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/await-export-promise.mjs'); +}, { + code: 'ERR_REQUIRE_ASYNC_MODULE' +}); + +const interval = setInterval(() => {}, 1000); // Keep the test running, because await alone doesn't. +const value = { hello: 'world' }; + +const p1 = import('../fixtures/es-modules/tla/await-export-promise.mjs').then(common.mustCall((ns) => { + assert.strictEqual(ns.default, value); +}), common.mustNotCall()); + +const p2 = import('../fixtures/es-modules/tla/await-export-promise.mjs').then(common.mustCall((ns) => { + assert.strictEqual(ns.default, value); +}), common.mustNotCall()); + +const p3 = import('../fixtures/es-modules/tla/await-export-promise.mjs').then(common.mustCall((ns) => { + assert.strictEqual(ns.default, value); +}), common.mustNotCall()); + +exportedResolve(value); + +Promise.all([p1, p2, p3]).then(common.mustCall(() => { clearInterval(interval); })); diff --git a/test/fixtures/es-modules/tla/await-export-promise.mjs b/test/fixtures/es-modules/tla/await-export-promise.mjs new file mode 100644 index 00000000000000..0129793e42b54a --- /dev/null +++ b/test/fixtures/es-modules/tla/await-export-promise.mjs @@ -0,0 +1,4 @@ +import promise from './export-promise.mjs'; +let result; +result = await promise; +export default result; diff --git a/test/fixtures/es-modules/tla/export-promise.mjs b/test/fixtures/es-modules/tla/export-promise.mjs new file mode 100644 index 00000000000000..74864d232e3c35 --- /dev/null +++ b/test/fixtures/es-modules/tla/export-promise.mjs @@ -0,0 +1,8 @@ +let exportedResolve; +let exportedReject; +const promise = new Promise((resolve, reject) => { + exportedResolve = resolve; + exportedReject = reject; +}); +export default promise; +export { exportedResolve, exportedReject };