From 27b6cfd996f84a129ab1be948b1664f47d3a8dae Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 23 Feb 2025 15:03:21 +0100 Subject: [PATCH 1/2] 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. - 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(). - 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(). --- lib/internal/modules/esm/loader.js | 76 +++++++++++++++++++------- lib/internal/modules/esm/module_job.js | 10 ++-- src/module_wrap.cc | 64 ++++++++++++++-------- 3 files changed, 102 insertions(+), 48 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 74d2f4062ba205..9013535d1d1c2f 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -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 @@ -346,35 +357,58 @@ 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. + 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})`; + } 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(job.module, raceMessage); } 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() }; + } + // 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); } - return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) }; + if (status !== kEvaluating) { + assert.fail(`Unexpected module status ${status}. ` + raceMessage); + } + 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 diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index bc76fc971b86ff..7c83e58f7b7ee6 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -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) { diff --git a/src/module_wrap.cc b/src/module_wrap.cc index dd0ae7a50617d6..54f9717ec39498 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -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& args) { args.GetReturnValue().Set(module->IsGraphAsync()); } +Maybe ThrowIfPromiseRejected(Realm* realm, Local promise) { + Isolate* isolate = realm->isolate(); + Local 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 type = + Integer::New(isolate, + static_cast( + PromiseRejectEvent::kPromiseHandlerAddedAfterReject)); + Local args[] = {type, promise, Undefined(isolate)}; + if (realm->promise_reject_callback() + ->Call(context, Undefined(isolate), arraysize(args), args) + .IsEmpty()) { + return Nothing(); + } + Local exception = promise->Result(); + Local message = Exception::CreateMessage(isolate, exception); + AppendExceptionLine( + realm->env(), exception, message, ErrorHandlingMode::MODULE_ERROR); + isolate->ThrowException(exception); + return Nothing(); +} + +void ThrowIfPromiseRejected(const FunctionCallbackInfo& args) { + if (!args[0]->IsPromise()) { + return; + } + ThrowIfPromiseRejected(Realm::GetCurrent(args), args[0].As()); +} + void ModuleWrap::EvaluateSync(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); Isolate* isolate = args.GetIsolate(); @@ -695,28 +734,7 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo& args) { CHECK(result->IsPromise()); Local promise = result.As(); - 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 type = - Integer::New(isolate, - static_cast( - PromiseRejectEvent::kPromiseHandlerAddedAfterReject)); - Local args[] = {type, promise, Undefined(isolate)}; - if (env->promise_reject_callback() - ->Call(context, Undefined(isolate), arraysize(args), args) - .IsEmpty()) { - return; - } - Local exception = promise->Result(); - Local 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 target, @@ -1326,6 +1345,7 @@ void ModuleWrap::RegisterExternalReferences( registry->Register(SetImportModuleDynamicallyCallback); registry->Register(SetInitializeImportMetaObjectCallback); + registry->Register(ThrowIfPromiseRejected); } } // namespace loader } // namespace node From be776fa0449d7b62dbed8eeaeb44781505d8681a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 3 Mar 2025 14:20:43 +0100 Subject: [PATCH 2/2] fixup! module: handle cached linked async jobs in require(esm) --- lib/internal/modules/esm/loader.js | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 9013535d1d1c2f..493e9881a466c3 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -97,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). @@ -362,14 +379,8 @@ class ModuleLoader { 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. - 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})`; - } if (!job.module) { - assert(job.module, raceMessage); + assert.fail(getRaceMessage(filename, parentFilename)); } if (job.module.async) { throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); @@ -398,7 +409,8 @@ class ModuleLoader { throwIfPromiseRejected(job.instantiated); } if (status !== kEvaluating) { - assert.fail(`Unexpected module status ${status}. ` + raceMessage); + assert.fail(`Unexpected module status ${status}. ` + + getRaceMessage(filename, parentFilename)); } let message = `Cannot require() ES Module ${filename} in a cycle.`; if (parentFilename) {