Skip to content

Commit 939ecf8

Browse files
joyeecheungmarco-ippolito
authored andcommitted
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(). PR-URL: #57187 Backport-PR-URL: #59504 Fixes: #57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Refs: #52697
1 parent ba7f8a0 commit 939ecf8

File tree

3 files changed

+118
-48
lines changed

3 files changed

+118
-48
lines changed

lib/internal/modules/esm/loader.js

Lines changed: 65 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,22 @@ const {
3636
getDefaultConditions,
3737
} = require('internal/modules/esm/utils');
3838
const { kImplicitAssertType } = require('internal/modules/esm/assert');
39-
const { ModuleWrap, kEvaluating, kEvaluated } = internalBinding('module_wrap');
39+
const {
40+
ModuleWrap,
41+
kEvaluated,
42+
kEvaluating,
43+
kInstantiated,
44+
throwIfPromiseRejected,
45+
} = internalBinding('module_wrap');
4046
const {
4147
urlToFilename,
4248
} = require('internal/modules/helpers');
4349
let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;
4450

51+
let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
52+
debug = fn;
53+
});
54+
4555
/**
4656
* @typedef {import('./hooks.js').HooksProxy} HooksProxy
4757
* @typedef {import('./module_job.js').ModuleJobBase} ModuleJobBase
@@ -75,6 +85,23 @@ function getTranslators() {
7585
return translators;
7686
}
7787

88+
/**
89+
* Generate message about potential race condition caused by requiring a cached module that has started
90+
* async linking.
91+
* @param {string} filename Filename of the module being required.
92+
* @param {string|undefined} parentFilename Filename of the module calling require().
93+
* @returns {string} Error message.
94+
*/
95+
function getRaceMessage(filename, parentFilename) {
96+
let raceMessage = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `;
97+
raceMessage += 'This may be caused by a race condition if the module is simultaneously dynamically ';
98+
raceMessage += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.';
99+
if (parentFilename) {
100+
raceMessage += ` (from ${parentFilename})`;
101+
}
102+
return raceMessage;
103+
}
104+
78105
/**
79106
* @type {HooksProxy}
80107
* Multiple loader instances exist for various, specific reasons (see code comments at site).
@@ -297,35 +324,53 @@ class ModuleLoader {
297324
// evaluated at this point.
298325
// TODO(joyeecheung): add something similar to CJS loader's requireStack to help
299326
// debugging the the problematic links in the graph for import.
327+
debug('importSyncForRequire', parent?.filename, '->', filename, job);
300328
if (job !== undefined) {
301329
mod[kRequiredModuleSymbol] = job.module;
302330
const parentFilename = urlToFilename(parent?.filename);
303331
// TODO(node:55782): this race may stop to happen when the ESM resolution and loading become synchronous.
304332
if (!job.module) {
305-
let message = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `;
306-
message += 'This may be caused by a race condition if the module is simultaneously dynamically ';
307-
message += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.';
308-
if (parentFilename) {
309-
message += ` (from ${parentFilename})`;
310-
}
311-
assert(job.module, message);
333+
assert.fail(getRaceMessage(filename, parentFilename));
312334
}
313335
if (job.module.async) {
314336
throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename);
315337
}
316-
// job.module may be undefined if it's asynchronously loaded. Which means
317-
// there is likely a cycle.
318-
if (job.module.getStatus() !== kEvaluated) {
319-
let message = `Cannot require() ES Module ${filename} in a cycle.`;
320-
if (parentFilename) {
321-
message += ` (from ${parentFilename})`;
322-
}
323-
message += 'A cycle involving require(esm) is disallowed to maintain ';
324-
message += 'invariants madated by the ECMAScript specification';
325-
message += 'Try making at least part of the dependency in the graph lazily loaded.';
326-
throw new ERR_REQUIRE_CYCLE_MODULE(message);
338+
const status = job.module.getStatus();
339+
debug('Module status', filename, status);
340+
if (status === kEvaluated) {
341+
return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) };
342+
} else if (status === kInstantiated) {
343+
// When it's an async job cached by another import request,
344+
// which has finished linking but has not started its
345+
// evaluation because the async run() task would be later
346+
// in line. Then start the evaluation now with runSync(), which
347+
// is guaranteed to finish by the time the other run() get to it,
348+
// and the other task would just get the cached evaluation results,
349+
// similar to what would happen when both are async.
350+
mod[kRequiredModuleSymbol] = job.module;
351+
const { namespace } = job.runSync(parent);
352+
return { wrap: job.module, namespace: namespace || job.module.getNamespace() };
327353
}
328-
return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) };
354+
// When the cached async job have already encountered a linking
355+
// error that gets wrapped into a rejection, but is still later
356+
// in line to throw on it, just unwrap and throw the linking error
357+
// from require().
358+
if (job.instantiated) {
359+
throwIfPromiseRejected(job.instantiated);
360+
}
361+
if (status !== kEvaluating) {
362+
assert.fail(`Unexpected module status ${status}. ` +
363+
getRaceMessage(filename, parentFilename));
364+
}
365+
let message = `Cannot require() ES Module ${filename} in a cycle.`;
366+
if (parentFilename) {
367+
message += ` (from ${parentFilename})`;
368+
}
369+
message += 'A cycle involving require(esm) is disallowed to maintain ';
370+
message += 'invariants madated by the ECMAScript specification';
371+
message += 'Try making at least part of the dependency in the graph lazily loaded.';
372+
throw new ERR_REQUIRE_CYCLE_MODULE(message);
373+
329374
}
330375
// TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the
331376
// cache here, or use a carrier object to carry the compiled module script

lib/internal/modules/esm/module_job.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -240,19 +240,19 @@ class ModuleJob extends ModuleJobBase {
240240
}
241241
}
242242

243-
runSync() {
243+
runSync(parent) {
244244
assert(this.module instanceof ModuleWrap);
245245
if (this.instantiated !== undefined) {
246246
return { __proto__: null, module: this.module };
247247
}
248248

249249
this.module.instantiate();
250250
this.instantiated = PromiseResolve();
251-
const timeout = -1;
252-
const breakOnSigint = false;
253251
setHasStartedUserESMExecution();
254-
this.module.evaluate(timeout, breakOnSigint);
255-
return { __proto__: null, module: this.module };
252+
const filename = urlToFilename(this.url);
253+
const parentFilename = urlToFilename(parent?.filename);
254+
const namespace = this.module.evaluateSync(filename, parentFilename);
255+
return { __proto__: null, module: this.module, namespace };
256256
}
257257

258258
async run() {

src/module_wrap.cc

Lines changed: 48 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ using v8::Array;
2424
using v8::ArrayBufferView;
2525
using v8::Context;
2626
using v8::EscapableHandleScope;
27+
using v8::Exception;
2728
using v8::FixedArray;
2829
using v8::Function;
2930
using v8::FunctionCallbackInfo;
@@ -32,15 +33,22 @@ using v8::HandleScope;
3233
using v8::Int32;
3334
using v8::Integer;
3435
using v8::Isolate;
36+
using v8::JustVoid;
3537
using v8::Local;
38+
using v8::Maybe;
3639
using v8::MaybeLocal;
40+
using v8::Message;
3741
using v8::MicrotaskQueue;
3842
using v8::Module;
3943
using v8::ModuleRequest;
44+
using v8::Name;
45+
using v8::Nothing;
46+
using v8::Null;
4047
using v8::Object;
4148
using v8::ObjectTemplate;
4249
using v8::PrimitiveArray;
4350
using v8::Promise;
51+
using v8::PromiseRejectEvent;
4452
using v8::ScriptCompiler;
4553
using v8::ScriptOrigin;
4654
using v8::String;
@@ -584,6 +592,43 @@ void ModuleWrap::InstantiateSync(const FunctionCallbackInfo<Value>& args) {
584592
args.GetReturnValue().Set(module->IsGraphAsync());
585593
}
586594

595+
Maybe<void> ThrowIfPromiseRejected(Realm* realm, Local<Promise> promise) {
596+
Isolate* isolate = realm->isolate();
597+
Local<Context> context = realm->context();
598+
if (promise->State() != Promise::PromiseState::kRejected) {
599+
return JustVoid();
600+
}
601+
// The rejected promise is created by V8, so we don't get a chance to mark
602+
// it as resolved before the rejection happens from evaluation. But we can
603+
// tell the promise rejection callback to treat it as a promise rejected
604+
// before handler was added which would remove it from the unhandled
605+
// rejection handling, since we are converting it into an error and throw
606+
// from here directly.
607+
Local<Value> type =
608+
Integer::New(isolate,
609+
static_cast<int32_t>(
610+
PromiseRejectEvent::kPromiseHandlerAddedAfterReject));
611+
Local<Value> args[] = {type, promise, Undefined(isolate)};
612+
if (realm->promise_reject_callback()
613+
->Call(context, Undefined(isolate), arraysize(args), args)
614+
.IsEmpty()) {
615+
return Nothing<void>();
616+
}
617+
Local<Value> exception = promise->Result();
618+
Local<Message> message = Exception::CreateMessage(isolate, exception);
619+
AppendExceptionLine(
620+
realm->env(), exception, message, ErrorHandlingMode::MODULE_ERROR);
621+
isolate->ThrowException(exception);
622+
return Nothing<void>();
623+
}
624+
625+
void ThrowIfPromiseRejected(const FunctionCallbackInfo<Value>& args) {
626+
if (!args[0]->IsPromise()) {
627+
return;
628+
}
629+
ThrowIfPromiseRejected(Realm::GetCurrent(args), args[0].As<Promise>());
630+
}
631+
587632
void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
588633
Realm* realm = Realm::GetCurrent(args);
589634
Isolate* isolate = args.GetIsolate();
@@ -608,29 +653,7 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
608653

609654
CHECK(result->IsPromise());
610655
Local<Promise> promise = result.As<Promise>();
611-
if (promise->State() == Promise::PromiseState::kRejected) {
612-
// The rejected promise is created by V8, so we don't get a chance to mark
613-
// it as resolved before the rejection happens from evaluation. But we can
614-
// tell the promise rejection callback to treat it as a promise rejected
615-
// before handler was added which would remove it from the unhandled
616-
// rejection handling, since we are converting it into an error and throw
617-
// from here directly.
618-
Local<Value> type = v8::Integer::New(
619-
isolate,
620-
static_cast<int32_t>(
621-
v8::PromiseRejectEvent::kPromiseHandlerAddedAfterReject));
622-
Local<Value> args[] = {type, promise, Undefined(isolate)};
623-
if (env->promise_reject_callback()
624-
->Call(context, Undefined(isolate), arraysize(args), args)
625-
.IsEmpty()) {
626-
return;
627-
}
628-
Local<Value> exception = promise->Result();
629-
Local<v8::Message> message =
630-
v8::Exception::CreateMessage(isolate, exception);
631-
AppendExceptionLine(
632-
env, exception, message, ErrorHandlingMode::MODULE_ERROR);
633-
isolate->ThrowException(exception);
656+
if (ThrowIfPromiseRejected(realm, promise).IsNothing()) {
634657
return;
635658
}
636659

@@ -1083,6 +1106,7 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data,
10831106
target,
10841107
"createRequiredModuleFacade",
10851108
CreateRequiredModuleFacade);
1109+
SetMethod(isolate, target, "throwIfPromiseRejected", ThrowIfPromiseRejected);
10861110
}
10871111

10881112
void ModuleWrap::CreatePerContextProperties(Local<Object> target,
@@ -1127,6 +1151,7 @@ void ModuleWrap::RegisterExternalReferences(
11271151

11281152
registry->Register(SetImportModuleDynamicallyCallback);
11291153
registry->Register(SetInitializeImportMetaObjectCallback);
1154+
registry->Register(ThrowIfPromiseRejected);
11301155
}
11311156
} // namespace loader
11321157
} // namespace node

0 commit comments

Comments
 (0)