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

module: fix sync hook short-circuit in require() in imported CJS · nodejs/node@96f19a1 · GitHub

/ node Public

Commit 96f19a1

Browse files
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 20f40c2 commit 96f19a1

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
@@ -1097,20 +1097,26 @@ function defaultResolveImplForCJSLoading(specifier, parent, isMain, options) {
10971097
return wrapResolveFilename(specifier, parent, isMain, options);
10981098
}
10991099

1100+
/**
1101+
* @typedef {{
1102+
* resolved?: {url?: string, format?: string, filename: string},
1103+
* shouldSkipModuleHooks?: boolean,
1104+
* source?: string|ArrayBufferView|ArrayBuffer,
1105+
* requireResolveOptions?: ResolveFilenameOptions,
1106+
* }} CJSModuleLoadInternalOptions
1107+
*/
1108+
11001109
/**
11011110
* Resolve a module request for CommonJS, invoking hooks from module.registerHooks()
11021111
* if necessary.
11031112
* @param {string} specifier
11041113
* @param {Module|undefined} parent
11051114
* @param {boolean} isMain
1106-
* @param {object} internalResolveOptions
1107-
* @param {boolean} internalResolveOptions.shouldSkipModuleHooks Whether to skip module hooks.
1108-
* @param {ResolveFilenameOptions} internalResolveOptions.requireResolveOptions Options from require.resolve().
1109-
* Only used when it comes from require.resolve().
1115+
* @param {CJSModuleLoadInternalOptions} internalOptions
11101116
* @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
11111117
*/
1112-
function resolveForCJSWithHooks(specifier, parent, isMain, internalResolveOptions) {
1113-
const { requireResolveOptions, shouldSkipModuleHooks } = internalResolveOptions;
1118+
function resolveForCJSWithHooks(specifier, parent, isMain, internalOptions) {
1119+
const { requireResolveOptions, shouldSkipModuleHooks } = internalOptions;
11141120
const defaultResolveImpl = requireResolveOptions ?
11151121
wrapResolveFilename : defaultResolveImplForCJSLoading;
11161122
// Fast path: no hooks, just return simple results.
@@ -1257,10 +1263,10 @@ function loadBuiltinWithHooks(id, url, format) {
12571263
* @param {string} request Specifier of module to load via `require`
12581264
* @param {Module} parent Absolute path of the module importing the child
12591265
* @param {boolean} isMain Whether the module is the main entry point
1260-
* @param {object|undefined} internalResolveOptions Additional options for loading the module
1266+
* @param {CJSModuleLoadInternalOptions|undefined} internalOptions Additional options for loading the module
12611267
* @returns {object}
12621268
*/
1263-
Module._load = function(request, parent, isMain, internalResolveOptions = kEmptyObject) {
1269+
Module._load = function(request, parent, isMain, internalOptions = kEmptyObject) {
12641270
let relResolveCacheIdentifier;
12651271
if (parent) {
12661272
debug('Module._load REQUEST %s parent: %s', request, parent.id);
@@ -1284,7 +1290,10 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty
12841290
}
12851291
}
12861292

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

@@ -1372,6 +1381,14 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty
13721381
module[kLastModuleParent] = parent;
13731382
}
13741383

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

‎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
@@ -124,7 +124,19 @@ const { defaultLoadSync, throwUnknownModuleFormat } = require('internal/modules/
124124
*/
125125

126126
/**
127-
* @typedef {{ format: ModuleFormat, source: ModuleSource, translatorKey: string }} TranslateContext
127+
* @typedef {{format: string, url: string, isResolvedBySyncHooks: boolean}} ResolveResult
128+
*/
129+
130+
/**
131+
* @typedef {{
132+
* url: string,
133+
* format: ModuleFormat,
134+
* source: ModuleSource,
135+
* responseURL?: string,
136+
* translatorKey: string,
137+
* isResolvedBySyncHooks: boolean,
138+
* isSourceLoadedSynchronously: boolean,
139+
* }} TranslateContext
128140
*/
129141

130142
/**
@@ -385,19 +397,25 @@ class ModuleLoader {
385397
/**
386398
* Load a module and translate it into a ModuleWrap for require(esm).
387399
* This is run synchronously, and the translator always return a ModuleWrap synchronously.
388-
* @param {string} url URL of the module to be translated.
400+
* @param {ResolveResult} resolveResult Result from the resolve step.
389401
* @param {object} loadContext See {@link load}
390402
* @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
391403
* @param {ModuleRequest} request Module request.
392404
* @returns {ModuleWrap}
393405
*/
394-
loadAndTranslateForImportInRequiredESM(url, loadContext, parentURL, request) {
406+
loadAndTranslateForImportInRequiredESM(resolveResult, loadContext, parentURL, request) {
407+
const { url } = resolveResult;
395408
const loadResult = this.#loadSync(url, loadContext);
396409
// Use the synchronous commonjs translator which can deal with cycles.
397410
const formatFromLoad = loadResult.format;
398411
const translatorKey = (formatFromLoad === 'commonjs' || formatFromLoad === 'commonjs-typescript') ?
399412
'commonjs-sync' : formatFromLoad;
400-
const translateContext = { ...loadResult, translatorKey, __proto__: null };
413+
const translateContext = {
414+
...resolveResult,
415+
...loadResult,
416+
translatorKey,
417+
__proto__: null,
418+
};
401419
const wrap = this.#translate(url, translateContext, parentURL);
402420
assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
403421

@@ -446,12 +464,13 @@ class ModuleLoader {
446464
/**
447465
* Load a module and translate it into a ModuleWrap for require() in imported CJS.
448466
* This is run synchronously, and the translator always return a ModuleWrap synchronously.
449-
* @param {string} url URL of the module to be translated.
467+
* @param {ResolveResult} resolveResult Result from the resolve step.
450468
* @param {object} loadContext See {@link load}
451469
* @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
452470
* @returns {ModuleWrap}
453471
*/
454-
loadAndTranslateForRequireInImportedCJS(url, loadContext, parentURL) {
472+
loadAndTranslateForRequireInImportedCJS(resolveResult, loadContext, parentURL) {
473+
const { url } = resolveResult;
455474
const loadResult = this.#loadSync(url, loadContext);
456475
const formatFromLoad = loadResult.format;
457476

@@ -473,7 +492,12 @@ class ModuleLoader {
473492
translatorKey = 'require-commonjs-typescript';
474493
}
475494

476-
const translateContext = { ...loadResult, translatorKey, __proto__: null };
495+
const translateContext = {
496+
...resolveResult,
497+
...loadResult,
498+
translatorKey,
499+
__proto__: null,
500+
};
477501
const wrap = this.#translate(url, translateContext, parentURL);
478502
assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
479503
return wrap;
@@ -482,15 +506,21 @@ class ModuleLoader {
482506
/**
483507
* Load a module and translate it into a ModuleWrap for ordinary imported ESM.
484508
* This may be run asynchronously if there are asynchronous module loader hooks registered.
485-
* @param {string} url URL of the module to be translated.
509+
* @param {ResolveResult} resolveResult Result from the resolve step.
486510
* @param {object} loadContext See {@link load}
487511
* @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
488512
* @returns {Promise<ModuleWrap>|ModuleWrap}
489513
*/
490-
loadAndTranslate(url, loadContext, parentURL) {
514+
loadAndTranslate(resolveResult, loadContext, parentURL) {
515+
const { url } = resolveResult;
491516
const maybePromise = this.load(url, loadContext);
492517
const afterLoad = (loadResult) => {
493-
const translateContext = { ...loadResult, translatorKey: loadResult.format, __proto__: null };
518+
const translateContext = {
519+
...resolveResult,
520+
...loadResult,
521+
translatorKey: loadResult.format,
522+
__proto__: null,
523+
};
494524
return this.#translate(url, translateContext, parentURL);
495525
};
496526
if (isPromise(maybePromise)) {
@@ -506,7 +536,7 @@ class ModuleLoader {
506536
* the module should be linked by the time this returns. Otherwise it may still have
507537
* pending module requests.
508538
* @param {string} parentURL See {@link getOrCreateModuleJob}
509-
* @param {{format: string, url: string}} resolveResult
539+
* @param {ResolveResult} resolveResult
510540
* @param {ModuleRequest} request Module request.
511541
* @param {ModuleRequestType} requestType Type of the module request.
512542
* @returns {ModuleJobBase} The (possibly pending) module job
@@ -545,11 +575,11 @@ class ModuleLoader {
545575

546576
let moduleOrModulePromise;
547577
if (requestType === kRequireInImportedCJS) {
548-
moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(url, context, parentURL);
578+
moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(resolveResult, context, parentURL);
549579
} else if (requestType === kImportInRequiredESM) {
550-
moduleOrModulePromise = this.loadAndTranslateForImportInRequiredESM(url, context, parentURL, request);
580+
moduleOrModulePromise = this.loadAndTranslateForImportInRequiredESM(resolveResult, context, parentURL, request);
551581
} else {
552-
moduleOrModulePromise = this.loadAndTranslate(url, context, parentURL);
582+
moduleOrModulePromise = this.loadAndTranslate(resolveResult, context, parentURL);
553583
}
554584

555585
if (requestType === kImportInRequiredESM || requestType === kRequireInImportedCJS ||
@@ -663,7 +693,7 @@ class ModuleLoader {
663693
* @param {string} [parentURL] The URL of the module where the module request is initiated.
664694
* It's undefined if it's from the root module.
665695
* @param {ModuleRequest} request Module request.
666-
* @returns {Promise<{format: string, url: string}>|{format: string, url: string}}
696+
* @returns {Promise<ResolveResult>|ResolveResult}
667697
*/
668698
#resolve(parentURL, request) {
669699
if (this.isForAsyncLoaderHookWorker) {
@@ -699,15 +729,18 @@ class ModuleLoader {
699729
/**
700730
* This is the default resolve step for module.registerHooks(), which incorporates asynchronous hooks
701731
* from module.register() which are run in a blocking fashion for it to be synchronous.
732+
* @param {{isResolvedByDefaultResolve: boolean}} out Output object to track whether the default resolve was used
733+
* without polluting the user-visible resolve result.
702734
* @param {string|URL} specifier See {@link resolveSync}.
703735
* @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context
704736
* See {@link resolveSync}.
705737
* @returns {{ format: string, url: string }}
706738
*/
707-
#resolveAndMaybeBlockOnLoaderThread(specifier, context) {
739+
#resolveAndMaybeBlockOnLoaderThread(out, specifier, context) {
708740
if (this.#asyncLoaderHooks?.resolveSync) {
709741
return this.#asyncLoaderHooks.resolveSync(specifier, context.parentURL, context.importAttributes);
710742
}
743+
out.isResolvedByDefaultResolve = true;
711744
return this.#cachedDefaultResolve(specifier, context);
712745
}
713746

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

766+
let result;
767+
let isResolvedBySyncHooks = false;
731768
if (!shouldSkipSyncHooks && syncResolveHooks.length) {
732769
// Has module.registerHooks() hooks, chain the asynchronous hooks in the default step.
733-
return resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
734-
this.#resolveAndMaybeBlockOnLoaderThread.bind(this));
770+
result = resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
771+
this.#resolveAndMaybeBlockOnLoaderThread.bind(this, out));
772+
// If the default step ran, sync hooks did not short-circuit the resolution.
773+
isResolvedBySyncHooks = !out.isResolvedByDefaultResolve;
774+
} else {
775+
const context = {
776+
...request,
777+
conditions: this.#defaultConditions,
778+
parentURL,
779+
importAttributes,
780+
__proto__: null,
781+
};
782+
result = this.#resolveAndMaybeBlockOnLoaderThread(out, specifier, context);
735783
}
736-
const context = {
737-
...request,
738-
conditions: this.#defaultConditions,
739-
parentURL,
740-
importAttributes,
741-
__proto__: null,
742-
};
743-
return this.#resolveAndMaybeBlockOnLoaderThread(specifier, context);
784+
result.isResolvedBySyncHooks = isResolvedBySyncHooks;
785+
return result;
744786
}
745787

746788
/**
747789
* Provide source that is understood by one of Node's translators. Handles customization hooks,
748790
* if any.
749-
* @typedef { {format: ModuleFormat, source: ModuleSource }} LoadResult
791+
* @typedef {{
792+
* format: ModuleFormat,
793+
* source: ModuleSource,
794+
* responseURL?: string,
795+
* isSourceLoadedSynchronously: boolean,
796+
* }} LoadResult
750797
* @param {string} url The URL of the module to be loaded.
751798
* @param {object} context Metadata about the module
752799
* @returns {Promise<LoadResult> | LoadResult}}
@@ -762,14 +809,19 @@ class ModuleLoader {
762809
/**
763810
* This is the default load step for module.registerHooks(), which incorporates asynchronous hooks
764811
* from module.register() which are run in a blocking fashion for it to be synchronous.
812+
* @param {{isSourceLoadedSynchronously: boolean}} out
813+
* Output object to track whether the source was loaded synchronously without polluting
814+
* the user-visible load result.
765815
* @param {string} url See {@link load}
766816
* @param {object} context See {@link load}
767817
* @returns {{ format: ModuleFormat, source: ModuleSource }}
768818
*/
769-
#loadAndMaybeBlockOnLoaderThread(url, context) {
819+
#loadAndMaybeBlockOnLoaderThread(out, url, context) {
770820
if (this.#asyncLoaderHooks?.loadSync) {
821+
out.isSourceLoadedSynchronously = false;
771822
return this.#asyncLoaderHooks.loadSync(url, context);
772823
}
824+
out.isSourceLoadedSynchronously = true;
773825
return defaultLoadSync(url, context);
774826
}
775827

@@ -780,17 +832,32 @@ class ModuleLoader {
780832
* This is here to support `require()` in imported CJS and `module.registerHooks()` hooks.
781833
* @param {string} url See {@link load}
782834
* @param {object} [context] See {@link load}
783-
* @returns {{ format: ModuleFormat, source: ModuleSource }}
835+
* @returns {LoadResult}
784836
*/
785837
#loadSync(url, context) {
838+
// Use an output parameter to track the state and avoid polluting the user-visible resolve results.
839+
const out = {
840+
isSourceLoadedSynchronously: true,
841+
__proto__: null,
842+
};
843+
let result;
786844
if (syncLoadHooks.length) {
787845
// Has module.registerHooks() hooks, chain the asynchronous hooks in the default step.
788846
// TODO(joyeecheung): construct the ModuleLoadContext in the loaders directly instead
789847
// of converting them from plain objects in the hooks.
790-
return loadWithSyncHooks(url, context.format, context.importAttributes, this.#defaultConditions,
791-
this.#loadAndMaybeBlockOnLoaderThread.bind(this), validateLoadSloppy);
848+
result = loadWithSyncHooks(
849+
url,
850+
context.format,
851+
context.importAttributes,
852+
this.#defaultConditions,
853+
this.#loadAndMaybeBlockOnLoaderThread.bind(this, out),
854+
validateLoadSloppy,
855+
);
856+
} else {
857+
result = this.#loadAndMaybeBlockOnLoaderThread(out, url, context);
792858
}
793-
return this.#loadAndMaybeBlockOnLoaderThread(url, context);
859+
result.isSourceLoadedSynchronously = out.isSourceLoadedSynchronously;
860+
return result;
794861
}
795862

796863
validateLoadResult(url, format) {

0 commit comments

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