Skip to content

Commit 91fdec3

Browse files
joyeecheungruyadorno
authored andcommitted
module: fix error thrown from require(esm) hitting TLA repeatedly
This tracks the asynchronicity in the ModuleWraps when they turn out to contain TLA after instantiation, and throw the right error (ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes the freezing of ModuleWraps since it's not meaningful to freeze this when the rest of the module loader is mutable, and we can record the asynchronicity in the ModuleWrap right after compilation after we get a V8 upgrade that contains v8::Module::HasTopLevelAwait() instead of searching through the module graph repeatedly which can be slow. PR-URL: #55520 Fixes: #55516 Refs: #52697 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>
1 parent 231fe7b commit 91fdec3

File tree

5 files changed

+40
-11
lines changed

5 files changed

+40
-11
lines changed

lib/internal/errors.js

+3
Original file line numberDiff line numberDiff line change
@@ -1652,6 +1652,9 @@ E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
16521652
E('ERR_QUIC_CONNECTION_FAILED', 'QUIC connection failed', Error);
16531653
E('ERR_QUIC_ENDPOINT_CLOSED', 'QUIC endpoint closed: %s (%d)', Error);
16541654
E('ERR_QUIC_OPEN_STREAM_FAILED', 'Failed to open QUIC stream', Error);
1655+
E('ERR_REQUIRE_ASYNC_MODULE', 'require() cannot be used on an ESM ' +
1656+
'graph with top-level await. Use import() instead. To see where the' +
1657+
' top-level await comes from, use --experimental-print-required-tla.', Error);
16551658
E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error);
16561659
E('ERR_REQUIRE_ESM',
16571660
function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) {

lib/internal/modules/esm/loader.js

+4
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ const { imported_cjs_symbol } = internalBinding('symbols');
2323

2424
const assert = require('internal/assert');
2525
const {
26+
ERR_REQUIRE_ASYNC_MODULE,
2627
ERR_REQUIRE_CYCLE_MODULE,
2728
ERR_REQUIRE_ESM,
2829
ERR_NETWORK_IMPORT_DISALLOWED,
@@ -302,6 +303,9 @@ class ModuleLoader {
302303
// evaluated at this point.
303304
if (job !== undefined) {
304305
mod[kRequiredModuleSymbol] = job.module;
306+
if (job.module.async) {
307+
throw new ERR_REQUIRE_ASYNC_MODULE();
308+
}
305309
if (job.module.getStatus() !== kEvaluated) {
306310
const parentFilename = urlToFilename(parent?.filename);
307311
let message = `Cannot require() ES Module ${filename} in a cycle.`;

lib/internal/modules/esm/module_job.js

+14-2
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,11 @@ const resolvedPromise = PromiseResolve();
3737
const {
3838
setHasStartedUserESMExecution,
3939
} = require('internal/modules/helpers');
40+
const { getOptionValue } = require('internal/options');
4041
const noop = FunctionPrototype;
41-
42+
const {
43+
ERR_REQUIRE_ASYNC_MODULE,
44+
} = require('internal/errors').codes;
4245
let hasPausedEntry = false;
4346

4447
const CJSGlobalLike = [
@@ -378,7 +381,16 @@ class ModuleJobSync extends ModuleJobBase {
378381

379382
runSync() {
380383
// TODO(joyeecheung): add the error decoration logic from the async instantiate.
381-
this.module.instantiateSync();
384+
this.module.async = this.module.instantiateSync();
385+
// If --experimental-print-required-tla is true, proceeds to evaluation even
386+
// if it's async because we want to search for the TLA and help users locate
387+
// them.
388+
// TODO(joyeecheung): track the asynchroniticy using v8::Module::HasTopLevelAwait()
389+
// and we'll be able to throw right after compilation of the modules, using acron
390+
// to find and print the TLA.
391+
if (this.module.async && !getOptionValue('--experimental-print-required-tla')) {
392+
throw new ERR_REQUIRE_ASYNC_MODULE();
393+
}
382394
setHasStartedUserESMExecution();
383395
const namespace = this.module.evaluateSync();
384396
return { __proto__: null, module: this.module, namespace };

src/module_wrap.cc

+3-9
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ using v8::FunctionTemplate;
3131
using v8::HandleScope;
3232
using v8::Int32;
3333
using v8::Integer;
34-
using v8::IntegrityLevel;
3534
using v8::Isolate;
3635
using v8::Local;
3736
using v8::MaybeLocal;
@@ -332,7 +331,6 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
332331

333332
obj->contextify_context_ = contextify_context;
334333

335-
that->SetIntegrityLevel(context, IntegrityLevel::kFrozen);
336334
args.GetReturnValue().Set(that);
337335
}
338336

@@ -635,13 +633,9 @@ void ModuleWrap::InstantiateSync(const FunctionCallbackInfo<Value>& args) {
635633
}
636634
}
637635

638-
// If --experimental-print-required-tla is true, proceeds to evaluation even
639-
// if it's async because we want to search for the TLA and help users locate
640-
// them.
641-
if (module->IsGraphAsync() && !env->options()->print_required_tla) {
642-
THROW_ERR_REQUIRE_ASYNC_MODULE(env);
643-
return;
644-
}
636+
// TODO(joyeecheung): record Module::HasTopLevelAwait() in every ModuleWrap
637+
// and infer the asynchronicity from a module's children during linking.
638+
args.GetReturnValue().Set(module->IsGraphAsync());
645639
}
646640

647641
void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// This tests that after failing to require an ESM that contains TLA,
2+
// retrying with require() still throws, and produces consistent results.
3+
'use strict';
4+
require('../common');
5+
const assert = require('assert');
6+
7+
assert.throws(() => {
8+
require('../fixtures/es-modules/tla/resolved.mjs');
9+
}, {
10+
code: 'ERR_REQUIRE_ASYNC_MODULE'
11+
});
12+
assert.throws(() => {
13+
require('../fixtures/es-modules/tla/resolved.mjs');
14+
}, {
15+
code: 'ERR_REQUIRE_ASYNC_MODULE'
16+
});

0 commit comments

Comments
 (0)