Skip to content

Commit 69df688

Browse files
joyeecheungaduh95
joyeecheung
authored and
aduh95
committed
module: fix sync hook short-circuit in require() in imported CJS
- For imported CJS, if it's not customized by asynchronous hooks, make sure it won't use the quirky re-invented require in all cases. - When the imported CJS module is customized by synchronous hooks, in the synthetic module evalutation step, avoid calling the respective default step again. - Make the branching of loadCJSModuleWithModuleLoad() and loadCJSModuleWithSpecialRequire() more explicit, and fold the tentative fs read in the 'commonjs' translator into the share createCJSModuleWrap() helper instead of checking it twice in the same path. Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com> PR-URL: #62920 Fixes: #63060 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
1 parent 30e71c7 commit 69df688

6 files changed

Lines changed: 265 additions & 89 deletions

File tree

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

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1095,20 +1095,26 @@ function defaultResolveImplForCJSLoading(specifier, parent, isMain, options) {
10951095
return wrapResolveFilename(specifier, parent, isMain, options);
10961096
}
10971097

1098+
/**
1099+
* @typedef {{
1100+
* resolved?: {url?: string, format?: string, filename: string},
1101+
* shouldSkipModuleHooks?: boolean,
1102+
* source?: string|ArrayBufferView|ArrayBuffer,
1103+
* requireResolveOptions?: ResolveFilenameOptions,
1104+
* }} CJSModuleLoadInternalOptions
1105+
*/
1106+
10981107
/**
10991108
* Resolve a module request for CommonJS, invoking hooks from module.registerHooks()
11001109
* if necessary.
11011110
* @param {string} specifier
11021111
* @param {Module|undefined} parent
11031112
* @param {boolean} isMain
1104-
* @param {object} internalResolveOptions
1105-
* @param {boolean} internalResolveOptions.shouldSkipModuleHooks Whether to skip module hooks.
1106-
* @param {ResolveFilenameOptions} internalResolveOptions.requireResolveOptions Options from require.resolve().
1107-
* Only used when it comes from require.resolve().
1113+
* @param {CJSModuleLoadInternalOptions} internalOptions
11081114
* @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
11091115
*/
1110-
function resolveForCJSWithHooks(specifier, parent, isMain, internalResolveOptions) {
1111-
const { requireResolveOptions, shouldSkipModuleHooks } = internalResolveOptions;
1116+
function resolveForCJSWithHooks(specifier, parent, isMain, internalOptions) {
1117+
const { requireResolveOptions, shouldSkipModuleHooks } = internalOptions;
11121118
const defaultResolveImpl = requireResolveOptions ?
11131119
wrapResolveFilename : defaultResolveImplForCJSLoading;
11141120
// Fast path: no hooks, just return simple results.
@@ -1255,10 +1261,10 @@ function loadBuiltinWithHooks(id, url, format) {
12551261
* @param {string} request Specifier of module to load via `require`
12561262
* @param {Module} parent Absolute path of the module importing the child
12571263
* @param {boolean} isMain Whether the module is the main entry point
1258-
* @param {object|undefined} internalResolveOptions Additional options for loading the module
1264+
* @param {CJSModuleLoadInternalOptions|undefined} internalOptions Additional options for loading the module
12591265
* @returns {object}
12601266
*/
1261-
Module._load = function(request, parent, isMain, internalResolveOptions = kEmptyObject) {
1267+
Module._load = function(request, parent, isMain, internalOptions = kEmptyObject) {
12621268
let relResolveCacheIdentifier;
12631269
if (parent) {
12641270
debug('Module._load REQUEST %s parent: %s', request, parent.id);
@@ -1282,7 +1288,10 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty
12821288
}
12831289
}
12841290

1285-
const resolveResult = resolveForCJSWithHooks(request, parent, isMain, internalResolveOptions);
1291+
// If the module has been resolved by a short-circuiting synchronous resolve hook,
1292+
// avoid running the default resolution from disk again.
1293+
const resolveResult = internalOptions.resolved ??
1294+
resolveForCJSWithHooks(request, parent, isMain, internalOptions);
12861295
let { format } = resolveResult;
12871296
const { url, filename } = resolveResult;
12881297

@@ -1370,6 +1379,14 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty
13701379
module[kLastModuleParent] = parent;
13711380
}
13721381

1382+
// The module source was provided by a short-circuiting synchronous hook,
1383+
// assign them into the module to avoid triggering the default load step again.
1384+
if (internalOptions.source !== undefined) {
1385+
module[kModuleSource] ??= internalOptions.source;
1386+
module[kURL] ??= url;
1387+
module[kFormat] ??= format;
1388+
}
1389+
13731390
if (parent !== undefined) {
13741391
relativeResolveCache[relResolveCacheIdentifier] = filename;
13751392
}

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

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
145145

146146
throwIfUnsupportedURLScheme(urlInstance, false);
147147

148-
let shouldBeReloadedByCJSLoader = false;
149148
if (urlInstance.protocol === 'node:') {
150149
source = null;
151150
format ??= 'builtin';
@@ -160,10 +159,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
160159

161160
// Now that we have the source for the module, run `defaultGetFormat` to detect its format.
162161
format ??= defaultGetFormat(urlInstance, context);
163-
164-
// For backward compatibility reasons, we need to let go through Module._load
165-
// again.
166-
shouldBeReloadedByCJSLoader = (format === 'commonjs');
167162
}
168163
validateAttributes(url, format, importAttributes);
169164

@@ -172,7 +167,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
172167
format,
173168
responseURL,
174169
source,
175-
shouldBeReloadedByCJSLoader,
176170
};
177171
}
178172

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

Lines changed: 100 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,19 @@ const { defaultLoadSync, throwUnknownModuleFormat } = require('internal/modules/
120120
*/
121121

122122
/**
123-
* @typedef {{ format: ModuleFormat, source: ModuleSource, translatorKey: string }} TranslateContext
123+
* @typedef {{format: string, url: string, isResolvedBySyncHooks: boolean}} ResolveResult
124+
*/
125+
126+
/**
127+
* @typedef {{
128+
* url: string,
129+
* format: ModuleFormat,
130+
* source: ModuleSource,
131+
* responseURL?: string,
132+
* translatorKey: string,
133+
* isResolvedBySyncHooks: boolean,
134+
* isSourceLoadedSynchronously: boolean,
135+
* }} TranslateContext
124136
*/
125137

126138
/**
@@ -382,19 +394,25 @@ class ModuleLoader {
382394
/**
383395
* Load a module and translate it into a ModuleWrap for require(esm).
384396
* This is run synchronously, and the translator always return a ModuleWrap synchronously.
385-
* @param {string} url URL of the module to be translated.
397+
* @param {ResolveResult} resolveResult Result from the resolve step.
386398
* @param {object} loadContext See {@link load}
387399
* @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
388400
* @param {ModuleRequest} request Module request.
389401
* @returns {ModuleWrap}
390402
*/
391-
loadAndTranslateForImportInRequiredESM(url, loadContext, parentURL, request) {
403+
loadAndTranslateForImportInRequiredESM(resolveResult, loadContext, parentURL, request) {
404+
const { url } = resolveResult;
392405
const loadResult = this.#loadSync(url, loadContext);
393406
// Use the synchronous commonjs translator which can deal with cycles.
394407
const formatFromLoad = loadResult.format;
395408
const translatorKey = (formatFromLoad === 'commonjs' || formatFromLoad === 'commonjs-typescript') ?
396409
'commonjs-sync' : formatFromLoad;
397-
const translateContext = { ...loadResult, translatorKey, __proto__: null };
410+
const translateContext = {
411+
...resolveResult,
412+
...loadResult,
413+
translatorKey,
414+
__proto__: null,
415+
};
398416
const wrap = this.#translate(url, translateContext, parentURL);
399417
assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
400418

@@ -443,12 +461,13 @@ class ModuleLoader {
443461
/**
444462
* Load a module and translate it into a ModuleWrap for require() in imported CJS.
445463
* This is run synchronously, and the translator always return a ModuleWrap synchronously.
446-
* @param {string} url URL of the module to be translated.
464+
* @param {ResolveResult} resolveResult Result from the resolve step.
447465
* @param {object} loadContext See {@link load}
448466
* @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
449467
* @returns {ModuleWrap}
450468
*/
451-
loadAndTranslateForRequireInImportedCJS(url, loadContext, parentURL) {
469+
loadAndTranslateForRequireInImportedCJS(resolveResult, loadContext, parentURL) {
470+
const { url } = resolveResult;
452471
const loadResult = this.#loadSync(url, loadContext);
453472
const formatFromLoad = loadResult.format;
454473

@@ -470,7 +489,12 @@ class ModuleLoader {
470489
translatorKey = 'require-commonjs-typescript';
471490
}
472491

473-
const translateContext = { ...loadResult, translatorKey, __proto__: null };
492+
const translateContext = {
493+
...resolveResult,
494+
...loadResult,
495+
translatorKey,
496+
__proto__: null,
497+
};
474498
const wrap = this.#translate(url, translateContext, parentURL);
475499
assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
476500
return wrap;
@@ -479,15 +503,21 @@ class ModuleLoader {
479503
/**
480504
* Load a module and translate it into a ModuleWrap for ordinary imported ESM.
481505
* This may be run asynchronously if there are asynchronous module loader hooks registered.
482-
* @param {string} url URL of the module to be translated.
506+
* @param {ResolveResult} resolveResult Result from the resolve step.
483507
* @param {object} loadContext See {@link load}
484508
* @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
485509
* @returns {Promise<ModuleWrap>|ModuleWrap}
486510
*/
487-
loadAndTranslate(url, loadContext, parentURL) {
511+
loadAndTranslate(resolveResult, loadContext, parentURL) {
512+
const { url } = resolveResult;
488513
const maybePromise = this.load(url, loadContext);
489514
const afterLoad = (loadResult) => {
490-
const translateContext = { ...loadResult, translatorKey: loadResult.format, __proto__: null };
515+
const translateContext = {
516+
...resolveResult,
517+
...loadResult,
518+
translatorKey: loadResult.format,
519+
__proto__: null,
520+
};
491521
return this.#translate(url, translateContext, parentURL);
492522
};
493523
if (isPromise(maybePromise)) {
@@ -503,7 +533,7 @@ class ModuleLoader {
503533
* the module should be linked by the time this returns. Otherwise it may still have
504534
* pending module requests.
505535
* @param {string} parentURL See {@link getOrCreateModuleJob}
506-
* @param {{format: string, url: string}} resolveResult
536+
* @param {ResolveResult} resolveResult
507537
* @param {ModuleRequest} request Module request.
508538
* @param {ModuleRequestType} requestType Type of the module request.
509539
* @returns {ModuleJobBase} The (possibly pending) module job
@@ -542,11 +572,11 @@ class ModuleLoader {
542572

543573
let moduleOrModulePromise;
544574
if (requestType === kRequireInImportedCJS) {
545-
moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(url, context, parentURL);
575+
moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(resolveResult, context, parentURL);
546576
} else if (requestType === kImportInRequiredESM) {
547-
moduleOrModulePromise = this.loadAndTranslateForImportInRequiredESM(url, context, parentURL, request);
577+
moduleOrModulePromise = this.loadAndTranslateForImportInRequiredESM(resolveResult, context, parentURL, request);
548578
} else {
549-
moduleOrModulePromise = this.loadAndTranslate(url, context, parentURL);
579+
moduleOrModulePromise = this.loadAndTranslate(resolveResult, context, parentURL);
550580
}
551581

552582
if (requestType === kImportInRequiredESM || requestType === kRequireInImportedCJS ||
@@ -660,7 +690,7 @@ class ModuleLoader {
660690
* @param {string} [parentURL] The URL of the module where the module request is initiated.
661691
* It's undefined if it's from the root module.
662692
* @param {ModuleRequest} request Module request.
663-
* @returns {Promise<{format: string, url: string}>|{format: string, url: string}}
693+
* @returns {Promise<ResolveResult>|ResolveResult}
664694
*/
665695
#resolve(parentURL, request) {
666696
if (this.isForAsyncLoaderHookWorker) {
@@ -696,15 +726,18 @@ class ModuleLoader {
696726
/**
697727
* This is the default resolve step for module.registerHooks(), which incorporates asynchronous hooks
698728
* from module.register() which are run in a blocking fashion for it to be synchronous.
729+
* @param {{isResolvedByDefaultResolve: boolean}} out Output object to track whether the default resolve was used
730+
* without polluting the user-visible resolve result.
699731
* @param {string|URL} specifier See {@link resolveSync}.
700732
* @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context
701733
* See {@link resolveSync}.
702734
* @returns {{ format: string, url: string }}
703735
*/
704-
#resolveAndMaybeBlockOnLoaderThread(specifier, context) {
736+
#resolveAndMaybeBlockOnLoaderThread(out, specifier, context) {
705737
if (this.#asyncLoaderHooks?.resolveSync) {
706738
return this.#asyncLoaderHooks.resolveSync(specifier, context.parentURL, context.importAttributes);
707739
}
740+
out.isResolvedByDefaultResolve = true;
708741
return this.#cachedDefaultResolve(specifier, context);
709742
}
710743

@@ -719,31 +752,45 @@ class ModuleLoader {
719752
* @param {boolean} [shouldSkipSyncHooks] Whether to skip the synchronous hooks registered by module.registerHooks().
720753
* This is used to maintain compatibility for the re-invented require.resolve (in imported CJS customized
721754
* by module.register()`) which invokes the CJS resolution separately from the hook chain.
722-
* @returns {{ format: string, url: string }}
755+
* @returns {ResolveResult}
723756
*/
724757
resolveSync(parentURL, request, shouldSkipSyncHooks = false) {
725758
const specifier = `${request.specifier}`;
726759
const importAttributes = request.attributes ?? kEmptyObject;
760+
// Use an output parameter to track the state and avoid polluting the user-visible resolve results.
761+
const out = { isResolvedByDefaultResolve: false, __proto__: null };
727762

763+
let result;
764+
let isResolvedBySyncHooks = false;
728765
if (!shouldSkipSyncHooks && syncResolveHooks.length) {
729766
// Has module.registerHooks() hooks, chain the asynchronous hooks in the default step.
730-
return resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
731-
this.#resolveAndMaybeBlockOnLoaderThread.bind(this));
767+
result = resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
768+
this.#resolveAndMaybeBlockOnLoaderThread.bind(this, out));
769+
// If the default step ran, sync hooks did not short-circuit the resolution.
770+
isResolvedBySyncHooks = !out.isResolvedByDefaultResolve;
771+
} else {
772+
const context = {
773+
...request,
774+
conditions: this.#defaultConditions,
775+
parentURL,
776+
importAttributes,
777+
__proto__: null,
778+
};
779+
result = this.#resolveAndMaybeBlockOnLoaderThread(out, specifier, context);
732780
}
733-
const context = {
734-
...request,
735-
conditions: this.#defaultConditions,
736-
parentURL,
737-
importAttributes,
738-
__proto__: null,
739-
};
740-
return this.#resolveAndMaybeBlockOnLoaderThread(specifier, context);
781+
result.isResolvedBySyncHooks = isResolvedBySyncHooks;
782+
return result;
741783
}
742784

743785
/**
744786
* Provide source that is understood by one of Node's translators. Handles customization hooks,
745787
* if any.
746-
* @typedef { {format: ModuleFormat, source: ModuleSource }} LoadResult
788+
* @typedef {{
789+
* format: ModuleFormat,
790+
* source: ModuleSource,
791+
* responseURL?: string,
792+
* isSourceLoadedSynchronously: boolean,
793+
* }} LoadResult
747794
* @param {string} url The URL of the module to be loaded.
748795
* @param {object} context Metadata about the module
749796
* @returns {Promise<LoadResult> | LoadResult}}
@@ -759,14 +806,19 @@ class ModuleLoader {
759806
/**
760807
* This is the default load step for module.registerHooks(), which incorporates asynchronous hooks
761808
* from module.register() which are run in a blocking fashion for it to be synchronous.
809+
* @param {{isSourceLoadedSynchronously: boolean}} out
810+
* Output object to track whether the source was loaded synchronously without polluting
811+
* the user-visible load result.
762812
* @param {string} url See {@link load}
763813
* @param {object} context See {@link load}
764814
* @returns {{ format: ModuleFormat, source: ModuleSource }}
765815
*/
766-
#loadAndMaybeBlockOnLoaderThread(url, context) {
816+
#loadAndMaybeBlockOnLoaderThread(out, url, context) {
767817
if (this.#asyncLoaderHooks?.loadSync) {
818+
out.isSourceLoadedSynchronously = false;
768819
return this.#asyncLoaderHooks.loadSync(url, context);
769820
}
821+
out.isSourceLoadedSynchronously = true;
770822
return defaultLoadSync(url, context);
771823
}
772824

@@ -777,17 +829,32 @@ class ModuleLoader {
777829
* This is here to support `require()` in imported CJS and `module.registerHooks()` hooks.
778830
* @param {string} url See {@link load}
779831
* @param {object} [context] See {@link load}
780-
* @returns {{ format: ModuleFormat, source: ModuleSource }}
832+
* @returns {LoadResult}
781833
*/
782834
#loadSync(url, context) {
835+
// Use an output parameter to track the state and avoid polluting the user-visible resolve results.
836+
const out = {
837+
isSourceLoadedSynchronously: true,
838+
__proto__: null,
839+
};
840+
let result;
783841
if (syncLoadHooks.length) {
784842
// Has module.registerHooks() hooks, chain the asynchronous hooks in the default step.
785843
// TODO(joyeecheung): construct the ModuleLoadContext in the loaders directly instead
786844
// of converting them from plain objects in the hooks.
787-
return loadWithSyncHooks(url, context.format, context.importAttributes, this.#defaultConditions,
788-
this.#loadAndMaybeBlockOnLoaderThread.bind(this), validateLoadSloppy);
845+
result = loadWithSyncHooks(
846+
url,
847+
context.format,
848+
context.importAttributes,
849+
this.#defaultConditions,
850+
this.#loadAndMaybeBlockOnLoaderThread.bind(this, out),
851+
validateLoadSloppy,
852+
);
853+
} else {
854+
result = this.#loadAndMaybeBlockOnLoaderThread(out, url, context);
789855
}
790-
return this.#loadAndMaybeBlockOnLoaderThread(url, context);
856+
result.isSourceLoadedSynchronously = out.isSourceLoadedSynchronously;
857+
return result;
791858
}
792859

793860
validateLoadResult(url, format) {

0 commit comments

Comments
 (0)