From 4cc4195493eec9cedea81f74a1bc78a4dac42193 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 8 May 2025 22:10:32 +0200 Subject: [PATCH] module: handle instantiated async module jobs in require(esm) When require(esm) encounters a cached module job that is instantiated but not yet evaluated, run the evaluation. This catches an edge case previously missed in https://github.com/nodejs/node/pull/57187. PR-URL: https://github.com/nodejs/node/pull/58067 Fixes: https://github.com/nodejs/node/issues/58061 Reviewed-By: Jacob Smith --- lib/internal/modules/esm/loader.js | 2 +- lib/internal/modules/esm/module_job.js | 46 ++++++++++++++----- src/debug_utils.h | 1 + src/module_wrap.cc | 12 +++++ src/module_wrap.h | 1 + .../test-require-module-instantiated.mjs | 4 ++ .../require-module-instantiated/a.mjs | 2 + .../require-module-instantiated/b.cjs | 1 + .../require-module-instantiated/c.mjs | 3 ++ 9 files changed, 60 insertions(+), 12 deletions(-) create mode 100644 test/es-module/test-require-module-instantiated.mjs create mode 100644 test/fixtures/es-modules/require-module-instantiated/a.mjs create mode 100644 test/fixtures/es-modules/require-module-instantiated/b.cjs create mode 100644 test/fixtures/es-modules/require-module-instantiated/c.mjs diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index aff686577df..5a817ed0434 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -378,7 +378,7 @@ class ModuleLoader { throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); } const status = job.module.getStatus(); - debug('Module status', filename, status); + debug('Module status', job, status); if (status === kEvaluated) { return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) }; } else if (status === kInstantiated) { diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 20e57e1e353..d30e6fb5155 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -22,7 +22,14 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { debug = fn; }); -const { ModuleWrap, kEvaluationPhase, kInstantiated } = internalBinding('module_wrap'); +const { + ModuleWrap, + kErrored, + kEvaluated, + kEvaluationPhase, + kInstantiated, + kUninstantiated, +} = internalBinding('module_wrap'); const { privateSymbols: { entry_point_module_private_symbol, @@ -280,17 +287,34 @@ class ModuleJob extends ModuleJobBase { runSync(parent) { assert(this.phase === kEvaluationPhase); assert(this.module instanceof ModuleWrap); - if (this.instantiated !== undefined) { - return { __proto__: null, module: this.module }; - } + let status = this.module.getStatus(); - this.module.instantiate(); - this.instantiated = PromiseResolve(); - setHasStartedUserESMExecution(); - const filename = urlToFilename(this.url); - const parentFilename = urlToFilename(parent?.filename); - const namespace = this.module.evaluateSync(filename, parentFilename); - return { __proto__: null, module: this.module, namespace }; + debug('ModuleJob.runSync', this.module); + // FIXME(joyeecheung): this cannot fully handle < kInstantiated. Make the linking + // fully synchronous instead. + if (status === kUninstantiated) { + this.module.async = this.module.instantiateSync(); + status = this.module.getStatus(); + } + if (status === kInstantiated || status === kErrored) { + const filename = urlToFilename(this.url); + const parentFilename = urlToFilename(parent?.filename); + this.module.async ??= this.module.isGraphAsync(); + + if (this.module.async && !getOptionValue('--experimental-print-required-tla')) { + throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); + } + if (status === kInstantiated) { + setHasStartedUserESMExecution(); + const namespace = this.module.evaluateSync(filename, parentFilename); + return { __proto__: null, module: this.module, namespace }; + } + throw this.module.getError(); + + } else if (status === kEvaluated) { + return { __proto__: null, module: this.module, namespace: this.module.getNamespaceSync() }; + } + assert.fail(`Unexpected module status ${status}.`); } async run(isEntryPoint = false) { diff --git a/src/debug_utils.h b/src/debug_utils.h index 7f073e1ea8b..ab5cd08f7e4 100644 --- a/src/debug_utils.h +++ b/src/debug_utils.h @@ -52,6 +52,7 @@ void NODE_EXTERN_PRIVATE FWrite(FILE* file, const std::string& str); V(NGTCP2_DEBUG) \ V(SEA) \ V(WASI) \ + V(MODULE) \ V(MKSNAPSHOT) \ V(SNAPSHOT_SERDES) \ V(PERMISSION_MODEL) \ diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 885974f524d..cdd0ba00eb0 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -815,6 +815,16 @@ void ModuleWrap::GetStatus(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(module->GetStatus()); } +void ModuleWrap::IsGraphAsync(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + ModuleWrap* obj; + ASSIGN_OR_RETURN_UNWRAP(&obj, args.This()); + + Local module = obj->module_.Get(isolate); + + args.GetReturnValue().Set(module->IsGraphAsync()); +} + void ModuleWrap::GetError(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); ModuleWrap* obj; @@ -1171,6 +1181,7 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data, isolate, tpl, "createCachedData", CreateCachedData); SetProtoMethodNoSideEffect(isolate, tpl, "getNamespace", GetNamespace); SetProtoMethodNoSideEffect(isolate, tpl, "getStatus", GetStatus); + SetProtoMethodNoSideEffect(isolate, tpl, "isGraphAsync", IsGraphAsync); SetProtoMethodNoSideEffect(isolate, tpl, "getError", GetError); SetConstructorFunction(isolate, target, "ModuleWrap", tpl); isolate_data->set_module_wrap_constructor_template(tpl); @@ -1227,6 +1238,7 @@ void ModuleWrap::RegisterExternalReferences( registry->Register(GetNamespace); registry->Register(GetStatus); registry->Register(GetError); + registry->Register(IsGraphAsync); registry->Register(CreateRequiredModuleFacade); diff --git a/src/module_wrap.h b/src/module_wrap.h index 83b5793013c..ef4dfd1d6b0 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -111,6 +111,7 @@ class ModuleWrap : public BaseObject { static void Instantiate(const v8::FunctionCallbackInfo& args); static void Evaluate(const v8::FunctionCallbackInfo& args); static void GetNamespace(const v8::FunctionCallbackInfo& args); + static void IsGraphAsync(const v8::FunctionCallbackInfo& args); static void GetStatus(const v8::FunctionCallbackInfo& args); static void GetError(const v8::FunctionCallbackInfo& args); diff --git a/test/es-module/test-require-module-instantiated.mjs b/test/es-module/test-require-module-instantiated.mjs new file mode 100644 index 00000000000..9dd50f31d36 --- /dev/null +++ b/test/es-module/test-require-module-instantiated.mjs @@ -0,0 +1,4 @@ +import '../common/index.mjs'; +import assert from 'node:assert'; +import { b, c } from '../fixtures/es-modules/require-module-instantiated/a.mjs'; +assert.strictEqual(b, c); diff --git a/test/fixtures/es-modules/require-module-instantiated/a.mjs b/test/fixtures/es-modules/require-module-instantiated/a.mjs new file mode 100644 index 00000000000..2918d414621 --- /dev/null +++ b/test/fixtures/es-modules/require-module-instantiated/a.mjs @@ -0,0 +1,2 @@ +export { default as b } from './b.cjs'; +export { default as c } from './c.mjs'; diff --git a/test/fixtures/es-modules/require-module-instantiated/b.cjs b/test/fixtures/es-modules/require-module-instantiated/b.cjs new file mode 100644 index 00000000000..1e23a5d46d2 --- /dev/null +++ b/test/fixtures/es-modules/require-module-instantiated/b.cjs @@ -0,0 +1 @@ +module.exports = require('./c.mjs'); diff --git a/test/fixtures/es-modules/require-module-instantiated/c.mjs b/test/fixtures/es-modules/require-module-instantiated/c.mjs new file mode 100644 index 00000000000..a5b4faccf9a --- /dev/null +++ b/test/fixtures/es-modules/require-module-instantiated/c.mjs @@ -0,0 +1,3 @@ +const foo = 1; +export default foo; +export { foo as 'module.exports' };