FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: handle cached linked async jobs in require(esm) · nodejs/node@46b06be · GitHub

/ node Public

Commit 46b06be

Browse files
joyeecheung
authored and
aduh95
committed
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 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>
1 parent b402799 commit 46b06be

3 files changed

Lines changed: 114 additions & 49 deletions

File tree

‎lib/internal/modules/esm/loader.js‎

Lines changed: 65 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ const {
1818
kIsExecuting,
1919
kRequiredModuleSymbol,
2020
} = require('internal/modules/cjs/loader');
21-
2221
const { imported_cjs_symbol } = internalBinding('symbols');
2322

2423
const assert = require('internal/assert');
@@ -38,7 +37,13 @@ const {
3837
forceDefaultLoader,
3938
} = require('internal/modules/esm/utils');
4039
const { kImplicitTypeAttribute } = require('internal/modules/esm/assert');
41-
const { ModuleWrap, kEvaluating, kEvaluated } = internalBinding('module_wrap');
40+
const {
41+
ModuleWrap,
42+
kEvaluated,
43+
kEvaluating,
44+
kInstantiated,
45+
throwIfPromiseRejected,
46+
} = internalBinding('module_wrap');
4247
const {
4348
urlToFilename,
4449
} = require('internal/modules/helpers');
@@ -53,6 +58,10 @@ let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;
5358
const { tracingChannel } = require('diagnostics_channel');
5459
const onImport = tracingChannel('module.import');
5560

61+
let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
62+
debug = fn;
63+
});
64+
5665
/**
5766
* @typedef {import('./hooks.js').HooksProxy} HooksProxy
5867
* @typedef {import('./module_job.js').ModuleJobBase} ModuleJobBase
@@ -86,6 +95,23 @@ function getTranslators() {
8695
return translators;
8796
}
8897

98+
/**
99+
* Generate message about potential race condition caused by requiring a cached module that has started
100+
* async linking.
101+
* @param {string} filename Filename of the module being required.
102+
* @param {string|undefined} parentFilename Filename of the module calling require().
103+
* @returns {string} Error message.
104+
*/
105+
function getRaceMessage(filename, parentFilename) {
106+
let raceMessage = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `;
107+
raceMessage += 'This may be caused by a race condition if the module is simultaneously dynamically ';
108+
raceMessage += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.';
109+
if (parentFilename) {
110+
raceMessage += ` (from ${parentFilename})`;
111+
}
112+
return raceMessage;
113+
}
114+
89115
/**
90116
* @type {HooksProxy}
91117
* Multiple loader instances exist for various, specific reasons (see code comments at site).
@@ -340,35 +366,53 @@ class ModuleLoader {
340366
// evaluated at this point.
341367
// TODO(joyeecheung): add something similar to CJS loader's requireStack to help
342368
// debugging the the problematic links in the graph for import.
369+
debug('importSyncForRequire', parent?.filename, '->', filename, job);
343370
if (job !== undefined) {
344371
mod[kRequiredModuleSymbol] = job.module;
345372
const parentFilename = urlToFilename(parent?.filename);
346373
// TODO(node:55782): this race may stop to happen when the ESM resolution and loading become synchronous.
347374
if (!job.module) {
348-
let message = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `;
349-
message += 'This may be caused by a race condition if the module is simultaneously dynamically ';
350-
message += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.';
351-
if (parentFilename) {
352-
message += ` (from ${parentFilename})`;
353-
}
354-
assert(job.module, message);
375+
assert.fail(getRaceMessage(filename, parentFilename));
355376
}
356377
if (job.module.async) {
357378
throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename);
358379
}
359-
// job.module may be undefined if it's asynchronously loaded. Which means
360-
// there is likely a cycle.
361-
if (job.module.getStatus() !== kEvaluated) {
362-
let message = `Cannot require() ES Module ${filename} in a cycle.`;
363-
if (parentFilename) {
364-
message += ` (from ${parentFilename})`;
365-
}
366-
message += 'A cycle involving require(esm) is disallowed to maintain ';
367-
message += 'invariants madated by the ECMAScript specification';
368-
message += 'Try making at least part of the dependency in the graph lazily loaded.';
369-
throw new ERR_REQUIRE_CYCLE_MODULE(message);
380+
const status = job.module.getStatus();
381+
debug('Module status', filename, status);
382+
if (status === kEvaluated) {
383+
return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) };
384+
} else if (status === kInstantiated) {
385+
// When it's an async job cached by another import request,
386+
// which has finished linking but has not started its
387+
// evaluation because the async run() task would be later
388+
// in line. Then start the evaluation now with runSync(), which
389+
// is guaranteed to finish by the time the other run() get to it,
390+
// and the other task would just get the cached evaluation results,
391+
// similar to what would happen when both are async.
392+
mod[kRequiredModuleSymbol] = job.module;
393+
const { namespace } = job.runSync(parent);
394+
return { wrap: job.module, namespace: namespace || job.module.getNamespace() };
370395
}
371-
return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) };
396+
// When the cached async job have already encountered a linking
397+
// error that gets wrapped into a rejection, but is still later
398+
// in line to throw on it, just unwrap and throw the linking error
399+
// from require().
400+
if (job.instantiated) {
401+
throwIfPromiseRejected(job.instantiated);
402+
}
403+
if (status !== kEvaluating) {
404+
assert.fail(`Unexpected module status ${status}. ` +
405+
getRaceMessage(filename, parentFilename));
406+
}
407+
let message = `Cannot require() ES Module ${filename} in a cycle.`;
408+
if (parentFilename) {
409+
message += ` (from ${parentFilename})`;
410+
}
411+
message += 'A cycle involving require(esm) is disallowed to maintain ';
412+
message += 'invariants madated by the ECMAScript specification';
413+
message += 'Try making at least part of the dependency in the graph lazily loaded.';
414+
throw new ERR_REQUIRE_CYCLE_MODULE(message);
415+
372416
}
373417
// TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the
374418
// cache here, or use a carrier object to carry the compiled module script

‎lib/internal/modules/esm/module_job.js‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
2222
debug = fn;
2323
});
2424

25-
const { ModuleWrap, kInstantiated } = internalBinding('module_wrap');
25+
const { ModuleWrap, kEvaluationPhase, kInstantiated } = internalBinding('module_wrap');
2626
const {
2727
privateSymbols: {
2828
entry_point_module_private_symbol,
@@ -246,19 +246,20 @@ class ModuleJob extends ModuleJobBase {
246246
}
247247
}
248248

249-
runSync() {
249+
runSync(parent) {
250+
assert(this.phase === kEvaluationPhase);
250251
assert(this.module instanceof ModuleWrap);
251252
if (this.instantiated !== undefined) {
252253
return { __proto__: null, module: this.module };
253254
}
254255

255256
this.module.instantiate();
256257
this.instantiated = PromiseResolve();
257-
const timeout = -1;
258-
const breakOnSigint = false;
259258
setHasStartedUserESMExecution();
260-
this.module.evaluate(timeout, breakOnSigint);
261-
return { __proto__: null, module: this.module };
259+
const filename = urlToFilename(this.url);
260+
const parentFilename = urlToFilename(parent?.filename);
261+
const namespace = this.module.evaluateSync(filename, parentFilename);
262+
return { __proto__: null, module: this.module, namespace };
262263
}
263264

264265
async run(isEntryPoint = false) {

‎src/module_wrap.cc‎

Lines changed: 42 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ using v8::Int32;
3636
using v8::Integer;
3737
using v8::Isolate;
3838
using v8::Just;
39+
using v8::JustVoid;
3940
using v8::Local;
4041
using v8::LocalVector;
4142
using v8::Maybe;
@@ -46,6 +47,7 @@ using v8::MicrotaskQueue;
4647
using v8::Module;
4748
using v8::ModuleRequest;
4849
using v8::Name;
50+
using v8::Nothing;
4951
using v8::Null;
5052
using v8::Object;
5153
using v8::ObjectTemplate;
@@ -653,6 +655,43 @@ void ModuleWrap::InstantiateSync(const FunctionCallbackInfo<Value>& args) {
653655
args.GetReturnValue().Set(module->IsGraphAsync());
654656
}
655657

658+
Maybe<void> ThrowIfPromiseRejected(Realm* realm, Local<Promise> promise) {
659+
Isolate* isolate = realm->isolate();
660+
Local<Context> context = realm->context();
661+
if (promise->State() != Promise::PromiseState::kRejected) {
662+
return JustVoid();
663+
}
664+
// The rejected promise is created by V8, so we don't get a chance to mark
665+
// it as resolved before the rejection happens from evaluation. But we can
666+
// tell the promise rejection callback to treat it as a promise rejected
667+
// before handler was added which would remove it from the unhandled
668+
// rejection handling, since we are converting it into an error and throw
669+
// from here directly.
670+
Local<Value> type =
671+
Integer::New(isolate,
672+
static_cast<int32_t>(
673+
PromiseRejectEvent::kPromiseHandlerAddedAfterReject));
674+
Local<Value> args[] = {type, promise, Undefined(isolate)};
675+
if (realm->promise_reject_callback()
676+
->Call(context, Undefined(isolate), arraysize(args), args)
677+
.IsEmpty()) {
678+
return Nothing<void>();
679+
}
680+
Local<Value> exception = promise->Result();
681+
Local<Message> message = Exception::CreateMessage(isolate, exception);
682+
AppendExceptionLine(
683+
realm->env(), exception, message, ErrorHandlingMode::MODULE_ERROR);
684+
isolate->ThrowException(exception);
685+
return Nothing<void>();
686+
}
687+
688+
void ThrowIfPromiseRejected(const FunctionCallbackInfo<Value>& args) {
689+
if (!args[0]->IsPromise()) {
690+
return;
691+
}
692+
ThrowIfPromiseRejected(Realm::GetCurrent(args), args[0].As<Promise>());
693+
}
694+
656695
void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
657696
Realm* realm = Realm::GetCurrent(args);
658697
Isolate* isolate = args.GetIsolate();
@@ -677,28 +716,7 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
677716

678717
CHECK(result->IsPromise());
679718
Local<Promise> promise = result.As<Promise>();
680-
if (promise->State() == Promise::PromiseState::kRejected) {
681-
// The rejected promise is created by V8, so we don't get a chance to mark
682-
// it as resolved before the rejection happens from evaluation. But we can
683-
// tell the promise rejection callback to treat it as a promise rejected
684-
// before handler was added which would remove it from the unhandled
685-
// rejection handling, since we are converting it into an error and throw
686-
// from here directly.
687-
Local<Value> type =
688-
Integer::New(isolate,
689-
static_cast<int32_t>(
690-
PromiseRejectEvent::kPromiseHandlerAddedAfterReject));
691-
Local<Value> args[] = {type, promise, Undefined(isolate)};
692-
if (env->promise_reject_callback()
693-
->Call(context, Undefined(isolate), arraysize(args), args)
694-
.IsEmpty()) {
695-
return;
696-
}
697-
Local<Value> exception = promise->Result();
698-
Local<Message> message = Exception::CreateMessage(isolate, exception);
699-
AppendExceptionLine(
700-
env, exception, message, ErrorHandlingMode::MODULE_ERROR);
701-
isolate->ThrowException(exception);
719+
if (ThrowIfPromiseRejected(realm, promise).IsNothing()) {
702720
return;
703721
}
704722

@@ -1152,6 +1170,7 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data,
11521170
target,
11531171
"createRequiredModuleFacade",
11541172
CreateRequiredModuleFacade);
1173+
SetMethod(isolate, target, "throwIfPromiseRejected", ThrowIfPromiseRejected);
11551174
}
11561175

11571176
void ModuleWrap::CreatePerContextProperties(Local<Object> target,
@@ -1196,6 +1215,7 @@ void ModuleWrap::RegisterExternalReferences(
11961215

11971216
registry->Register(SetImportModuleDynamicallyCallback);
11981217
registry->Register(SetInitializeImportMetaObjectCallback);
1218+
registry->Register(ThrowIfPromiseRejected);
11991219
}
12001220
} // namespace loader
12011221
} // namespace node

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL