Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: handle cached linked async jobs in require(esm) #57187

Merged
merged 2 commits into from
Mar 4, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 67 additions & 21 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
@@ -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
@@ -86,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).
@@ -346,35 +374,53 @@ 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.
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.fail(getRaceMessage(filename, parentFilename));
}
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() };
}
return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) };
// 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);
}
if (status !== kEvaluating) {
assert.fail(`Unexpected module status ${status}. ` +
getRaceMessage(filename, parentFilename));
}
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
10 changes: 5 additions & 5 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
@@ -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) {
64 changes: 42 additions & 22 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
@@ -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<Value>& args) {
args.GetReturnValue().Set(module->IsGraphAsync());
}

Maybe<void> ThrowIfPromiseRejected(Realm* realm, Local<Promise> promise) {
Isolate* isolate = realm->isolate();
Local<Context> 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<Value> type =
Integer::New(isolate,
static_cast<int32_t>(
PromiseRejectEvent::kPromiseHandlerAddedAfterReject));
Local<Value> args[] = {type, promise, Undefined(isolate)};
if (realm->promise_reject_callback()
->Call(context, Undefined(isolate), arraysize(args), args)
.IsEmpty()) {
return Nothing<void>();
}
Local<Value> exception = promise->Result();
Local<Message> message = Exception::CreateMessage(isolate, exception);
AppendExceptionLine(
realm->env(), exception, message, ErrorHandlingMode::MODULE_ERROR);
isolate->ThrowException(exception);
return Nothing<void>();
}

void ThrowIfPromiseRejected(const FunctionCallbackInfo<Value>& args) {
if (!args[0]->IsPromise()) {
return;
}
ThrowIfPromiseRejected(Realm::GetCurrent(args), args[0].As<Promise>());
}

void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
Realm* realm = Realm::GetCurrent(args);
Isolate* isolate = args.GetIsolate();
@@ -695,28 +734,7 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {

CHECK(result->IsPromise());
Local<Promise> promise = result.As<Promise>();
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<Value> type =
Integer::New(isolate,
static_cast<int32_t>(
PromiseRejectEvent::kPromiseHandlerAddedAfterReject));
Local<Value> args[] = {type, promise, Undefined(isolate)};
if (env->promise_reject_callback()
->Call(context, Undefined(isolate), arraysize(args), args)
.IsEmpty()) {
return;
}
Local<Value> exception = promise->Result();
Local<Message> 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<Object> target,
@@ -1326,6 +1345,7 @@ void ModuleWrap::RegisterExternalReferences(

registry->Register(SetImportModuleDynamicallyCallback);
registry->Register(SetInitializeImportMetaObjectCallback);
registry->Register(ThrowIfPromiseRejected);
}
} // namespace loader
} // namespace node
Loading