Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d3be3da

Browse filesBrowse files
joyeecheungRafaelGSS
joyeecheung
authored and
RafaelGSS
committed
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 b3971bb commit d3be3da
Copy full SHA for d3be3da

File tree

Expand file treeCollapse file tree

5 files changed

+40
-11
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+40
-11
lines changed
Open diff view settings
Collapse file

‎lib/internal/errors.js‎

Copy file name to clipboardExpand all lines: lib/internal/errors.js
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1650,6 +1650,9 @@ E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
16501650
E('ERR_QUIC_CONNECTION_FAILED', 'QUIC connection failed', Error);
16511651
E('ERR_QUIC_ENDPOINT_CLOSED', 'QUIC endpoint closed: %s (%d)', Error);
16521652
E('ERR_QUIC_OPEN_STREAM_FAILED', 'Failed to open QUIC stream', Error);
1653+
E('ERR_REQUIRE_ASYNC_MODULE', 'require() cannot be used on an ESM ' +
1654+
'graph with top-level await. Use import() instead. To see where the' +
1655+
' top-level await comes from, use --experimental-print-required-tla.', Error);
16531656
E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error);
16541657
E('ERR_REQUIRE_ESM',
16551658
function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) {
Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/loader.js
+4Lines changed: 4 additions & 0 deletions
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.`;
Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/module_job.js
+14-2Lines changed: 14 additions & 2 deletions
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 };
Collapse file

‎src/module_wrap.cc‎

Copy file name to clipboardExpand all lines: src/module_wrap.cc
+3-9Lines changed: 3 additions & 9 deletions
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) {
Collapse file

‎test/es-module/test-require-module-tla-retry-require.js‎

Copy file name to clipboard
+16Lines changed: 16 additions & 0 deletions
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.