Skip to content

Commit 00412cd

Browse files
joyeecheungjoyeecheung
committed
module: handle null source from async loader hooks in sync hooks
This relaxes the validation in sync hooks so that it accepts the quirky nullish source returned by the default step of the async loader when the module being loaded is CommonJS. When there are no customization hooks registered, a saner synchronous default load step is used to use a property instead of a reset nullish source to signify that the module should go through the CJS monkey patching routes and reduce excessive reloading from disk. PR-URL: nodejs#59929 Fixes: nodejs#59384 Fixes: nodejs#57327 Refs: nodejs#59666 Refs: https://github.com/dygabo/load_module_test Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>
1 parent 9b0b024 commit 00412cd

14 files changed

Lines changed: 228 additions & 108 deletions

File tree

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ const {
177177
registerHooks,
178178
resolveHooks,
179179
resolveWithHooks,
180+
validateLoadStrict,
180181
} = require('internal/modules/customization_hooks');
181182
const { stripTypeScriptModuleTypes } = require('internal/modules/typescript');
182183
const packageJsonReader = require('internal/modules/package_json_reader');
@@ -1150,7 +1151,7 @@ function loadBuiltinWithHooks(id, url, format) {
11501151
url ??= `node:${id}`;
11511152
// TODO(joyeecheung): do we really want to invoke the load hook for the builtins?
11521153
const loadResult = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined,
1153-
getCjsConditionsArray(), getDefaultLoad(url, id));
1154+
getCjsConditionsArray(), getDefaultLoad(url, id), validateLoadStrict);
11541155
if (loadResult.format && loadResult.format !== 'builtin') {
11551156
return undefined; // Format has been overridden, return undefined for the caller to continue loading.
11561157
}
@@ -1759,10 +1760,9 @@ function loadSource(mod, filename, formatFromNode) {
17591760
mod[kURL] = convertCJSFilenameToURL(filename);
17601761
}
17611762

1763+
const defaultLoad = getDefaultLoad(mod[kURL], filename);
17621764
const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined,
1763-
getCjsConditionsArray(),
1764-
getDefaultLoad(mod[kURL], filename));
1765-
1765+
getCjsConditionsArray(), defaultLoad, validateLoadStrict);
17661766
// Reset the module properties with load hook results.
17671767
if (loadResult.format !== undefined) {
17681768
mod[kFormat] = loadResult.format;

‎lib/internal/modules/customization_hooks.js‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -262,29 +262,56 @@ function validateResolve(specifier, context, result) {
262262
*/
263263

264264
/**
265-
* Validate the result returned by a chain of resolve hook.
265+
* Validate the result returned by a chain of load hook.
266266
* @param {string} url URL passed into the hooks.
267267
* @param {ModuleLoadContext} context Context passed into the hooks.
268268
* @param {ModuleLoadResult} result Result produced by load hooks.
269269
* @returns {ModuleLoadResult}
270270
*/
271-
function validateLoad(url, context, result) {
271+
function validateLoadStrict(url, context, result) {
272+
validateSourceStrict(url, context, result);
273+
validateFormat(url, context, result);
274+
return result;
275+
}
276+
277+
function validateLoadSloppy(url, context, result) {
278+
validateSourcePermissive(url, context, result);
279+
validateFormat(url, context, result);
280+
return result;
281+
}
282+
283+
function validateSourceStrict(url, context, result) {
272284
const { source, format } = result;
273285
// To align with module.register(), the load hooks are still invoked for
274286
// the builtins even though the default load step only provides null as source,
275287
// and any source content for builtins provided by the user hooks are ignored.
276288
if (!StringPrototypeStartsWith(url, 'node:') &&
277289
typeof result.source !== 'string' &&
278290
!isAnyArrayBuffer(source) &&
279-
!isArrayBufferView(source)) {
291+
!isArrayBufferView(source) &&
292+
format !== 'addon') {
280293
throw new ERR_INVALID_RETURN_PROPERTY_VALUE(
281294
'a string, an ArrayBuffer, or a TypedArray',
282295
'load',
283296
'source',
284297
source,
285298
);
286299
}
300+
}
287301

302+
function validateSourcePermissive(url, context, result) {
303+
const { source, format } = result;
304+
if (format === 'commonjs' && source == null) {
305+
// Accommodate the quirk in defaultLoad used by asynchronous loader hooks
306+
// which sets source to null for commonjs.
307+
// See: https://github.com/nodejs/node/issues/57327#issuecomment-2701382020
308+
return;
309+
}
310+
validateSourceStrict(url, context, result);
311+
}
312+
313+
function validateFormat(url, context, result) {
314+
const { format } = result;
288315
if (typeof format !== 'string' && format !== undefined) {
289316
throw new ERR_INVALID_RETURN_PROPERTY_VALUE(
290317
'a string',
@@ -293,12 +320,6 @@ function validateLoad(url, context, result) {
293320
format,
294321
);
295322
}
296-
297-
return {
298-
__proto__: null,
299-
format,
300-
source,
301-
};
302323
}
303324

304325
class ModuleResolveContext {
@@ -338,9 +359,10 @@ let decoder;
338359
* @param {ImportAttributes|undefined} importAttributes
339360
* @param {string[]} conditions
340361
* @param {(url: string, context: ModuleLoadContext) => ModuleLoadResult} defaultLoad
362+
* @param {(url: string, context: ModuleLoadContext, result: ModuleLoadResult) => ModuleLoadResult} validateLoad
341363
* @returns {ModuleLoadResult}
342364
*/
343-
function loadWithHooks(url, originalFormat, importAttributes, conditions, defaultLoad) {
365+
function loadWithHooks(url, originalFormat, importAttributes, conditions, defaultLoad, validateLoad) {
344366
debug('loadWithHooks', url, originalFormat);
345367
const context = new ModuleLoadContext(originalFormat, importAttributes, conditions);
346368
if (loadHooks.length === 0) {
@@ -403,4 +425,6 @@ module.exports = {
403425
registerHooks,
404426
resolveHooks,
405427
resolveWithHooks,
428+
validateLoadStrict,
429+
validateLoadSloppy,
406430
};

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ const {
6161
SHARED_MEMORY_BYTE_LENGTH,
6262
WORKER_TO_MAIN_THREAD_NOTIFICATION,
6363
} = require('internal/modules/esm/shared_constants');
64-
let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
64+
let debug = require('internal/util/debuglog').debuglog('async_loader_worker', (fn) => {
6565
debug = fn;
6666
});
6767
let importMetaInitializer;

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

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -145,22 +145,34 @@ function defaultLoadSync(url, context = kEmptyObject) {
145145

146146
throwIfUnsupportedURLScheme(urlInstance, false);
147147

148+
let shouldBeReloadedByCJSLoader = false;
148149
if (urlInstance.protocol === 'node:') {
149150
source = null;
150-
} else if (source == null) {
151-
({ responseURL, source } = getSourceSync(urlInstance, context));
152-
context.source = source;
153-
}
151+
format ??= 'builtin';
152+
} else if (format === 'addon') {
153+
// Skip loading addon file content. It must be loaded with dlopen from file system.
154+
source = null;
155+
} else {
156+
if (source == null) {
157+
({ responseURL, source } = getSourceSync(urlInstance, context));
158+
context = { __proto__: context, source };
159+
}
154160

155-
format ??= defaultGetFormat(urlInstance, context);
161+
// Now that we have the source for the module, run `defaultGetFormat` to detect its format.
162+
format ??= defaultGetFormat(urlInstance, context);
156163

164+
// For backward compatibility reasons, we need to let go through Module._load
165+
// again.
166+
shouldBeReloadedByCJSLoader = (format === 'commonjs');
167+
}
157168
validateAttributes(url, format, importAttributes);
158169

159170
return {
160171
__proto__: null,
161172
format,
162173
responseURL,
163174
source,
175+
shouldBeReloadedByCJSLoader,
164176
};
165177
}
166178

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

Lines changed: 35 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,9 @@ const {
5353
resolveWithHooks,
5454
loadHooks,
5555
loadWithHooks,
56+
validateLoadSloppy,
5657
} = require('internal/modules/customization_hooks');
57-
let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;
58+
let defaultResolve, defaultLoadSync, importMetaInitializer;
5859

5960
const { tracingChannel } = require('diagnostics_channel');
6061
const onImport = tracingChannel('module.import');
@@ -144,6 +145,10 @@ let hooksProxy;
144145
* @typedef {ArrayBuffer|TypedArray|string} ModuleSource
145146
*/
146147

148+
/**
149+
* @typedef {{ format: ModuleFormat, source: ModuleSource, translatorKey: string }} TranslateContext
150+
*/
151+
147152
/**
148153
* This class covers the base machinery of module loading. To add custom
149154
* behavior you can pass a customizations object and this object will be
@@ -492,18 +497,19 @@ class ModuleLoader {
492497

493498
const loadResult = this.#loadSync(url, { format, importAttributes });
494499

500+
const formatFromLoad = loadResult.format;
495501
// Use the synchronous commonjs translator which can deal with cycles.
496-
const finalFormat =
497-
loadResult.format === 'commonjs' ||
498-
loadResult.format === 'commonjs-typescript' ? 'commonjs-sync' : loadResult.format;
502+
const translatorKey = (formatFromLoad === 'commonjs' || formatFromLoad === 'commonjs-typescript') ?
503+
'commonjs-sync' : formatFromLoad;
499504

500-
if (finalFormat === 'wasm') {
505+
if (translatorKey === 'wasm') {
501506
assert.fail('WASM is currently unsupported by require(esm)');
502507
}
503508

504509
const { source } = loadResult;
505510
const isMain = (parentURL === undefined);
506-
const wrap = this.#translate(url, finalFormat, source, parentURL);
511+
const translateContext = { format: formatFromLoad, source, translatorKey, __proto__: null };
512+
const wrap = this.#translate(url, translateContext, parentURL);
507513
assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
508514

509515
if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) {
@@ -512,7 +518,7 @@ class ModuleLoader {
512518

513519
const cjsModule = wrap[imported_cjs_symbol];
514520
if (cjsModule) {
515-
assert(finalFormat === 'commonjs-sync');
521+
assert(translatorKey === 'commonjs-sync');
516522
// Check if the ESM initiating import CJS is being required by the same CJS module.
517523
if (cjsModule?.[kIsExecuting]) {
518524
const parentFilename = urlToFilename(parentURL);
@@ -536,22 +542,22 @@ class ModuleLoader {
536542
* Translate a loaded module source into a ModuleWrap. This is run synchronously,
537543
* but the translator may return the ModuleWrap in a Promise.
538544
* @param {string} url URL of the module to be translated.
539-
* @param {string} format Format of the module to be translated. This is used to find
540-
* matching translators.
541-
* @param {ModuleSource} source Source of the module to be translated.
542-
* @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
545+
* @param {TranslateContext} translateContext Context for the translator
546+
* @param {string|undefined} parentURL URL of the module initiating the module loading for the first time.
547+
* Undefined if it's the entry point.
543548
* @returns {ModuleWrap}
544549
*/
545-
#translate(url, format, source, parentURL) {
550+
#translate(url, translateContext, parentURL) {
551+
const { translatorKey, format } = translateContext;
546552
this.validateLoadResult(url, format);
547-
const translator = getTranslators().get(format);
553+
const translator = getTranslators().get(translatorKey);
548554

549555
if (!translator) {
550-
throw new ERR_UNKNOWN_MODULE_FORMAT(format, url);
556+
throw new ERR_UNKNOWN_MODULE_FORMAT(translatorKey, url);
551557
}
552558

553-
const result = FunctionPrototypeCall(translator, this, url, source, parentURL === undefined);
554-
assert(result instanceof ModuleWrap);
559+
const result = FunctionPrototypeCall(translator, this, url, translateContext, parentURL);
560+
assert(result instanceof ModuleWrap, `The ${format} module returned is not a ModuleWrap`);
555561
return result;
556562
}
557563

@@ -564,7 +570,8 @@ class ModuleLoader {
564570
* @returns {ModuleWrap}
565571
*/
566572
loadAndTranslateForRequireInImportedCJS(url, loadContext, parentURL) {
567-
const { format: formatFromLoad, source } = this.#loadSync(url, loadContext);
573+
const loadResult = this.#loadSync(url, loadContext);
574+
const formatFromLoad = loadResult.format;
568575

569576
if (formatFromLoad === 'wasm') { // require(wasm) is not supported.
570577
throw new ERR_UNKNOWN_MODULE_FORMAT(formatFromLoad, url);
@@ -576,15 +583,16 @@ class ModuleLoader {
576583
}
577584
}
578585

579-
let finalFormat = formatFromLoad;
586+
let translatorKey = formatFromLoad;
580587
if (formatFromLoad === 'commonjs') {
581-
finalFormat = 'require-commonjs';
588+
translatorKey = 'require-commonjs';
582589
}
583590
if (formatFromLoad === 'commonjs-typescript') {
584-
finalFormat = 'require-commonjs-typescript';
591+
translatorKey = 'require-commonjs-typescript';
585592
}
586593

587-
const wrap = this.#translate(url, finalFormat, source, parentURL);
594+
const translateContext = { ...loadResult, translatorKey, __proto__: null };
595+
const wrap = this.#translate(url, translateContext, parentURL);
588596
assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
589597
return wrap;
590598
}
@@ -599,8 +607,9 @@ class ModuleLoader {
599607
*/
600608
loadAndTranslate(url, loadContext, parentURL) {
601609
const maybePromise = this.load(url, loadContext);
602-
const afterLoad = ({ format, source }) => {
603-
return this.#translate(url, format, source, parentURL);
610+
const afterLoad = (loadResult) => {
611+
const translateContext = { ...loadResult, translatorKey: loadResult.format, __proto__: null };
612+
return this.#translate(url, translateContext, parentURL);
604613
};
605614
if (isPromise(maybePromise)) {
606615
return maybePromise.then(afterLoad);
@@ -818,8 +827,8 @@ class ModuleLoader {
818827
return this.#customizations.load(url, context);
819828
}
820829

821-
defaultLoad ??= require('internal/modules/esm/load').defaultLoad;
822-
return defaultLoad(url, context);
830+
defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync;
831+
return defaultLoadSync(url, context);
823832
}
824833

825834
/**
@@ -854,7 +863,7 @@ class ModuleLoader {
854863
// TODO(joyeecheung): construct the ModuleLoadContext in the loaders directly instead
855864
// of converting them from plain objects in the hooks.
856865
return loadWithHooks(url, context.format, context.importAttributes, this.#defaultConditions,
857-
this.#loadAndMaybeBlockOnLoaderThread.bind(this));
866+
this.#loadAndMaybeBlockOnLoaderThread.bind(this), validateLoadSloppy);
858867
}
859868
return this.#loadAndMaybeBlockOnLoaderThread(url, context);
860869
}

0 commit comments

Comments
 (0)