module: handle null source from async loader hooks in sync hooks · nodejs/node@be1b84f · GitHub
Skip to content

Commit be1b84f

Browse files
joyeecheungaduh95
authored andcommitted
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: #59929 Fixes: #59384 Fixes: #57327 Refs: #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 4395fe1 commit be1b84f

14 files changed

Lines changed: 227 additions & 107 deletions

File tree

lib/internal/modules/cjs/loader.js

Lines changed: 4 additions & 4 deletions

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
@@ -141,22 +141,34 @@ function defaultLoadSync(url, context = kEmptyObject) {
141141

142142
throwIfUnsupportedURLScheme(urlInstance, false);
143143

144+
let shouldBeReloadedByCJSLoader = false;
144145
if (urlInstance.protocol === 'node:') {
145146
source = null;
146-
} else if (source == null) {
147-
({ responseURL, source } = getSourceSync(urlInstance, context));
148-
context.source = source;
149-
}
147+
format ??= 'builtin';
148+
} else if (format === 'addon') {
149+
// Skip loading addon file content. It must be loaded with dlopen from file system.
150+
source = null;
151+
} else {
152+
if (source == null) {
153+
({ responseURL, source } = getSourceSync(urlInstance, context));
154+
context = { __proto__: context, source };
155+
}
150156

151-
format ??= defaultGetFormat(urlInstance, context);
157+
// Now that we have the source for the module, run `defaultGetFormat` to detect its format.
158+
format ??= defaultGetFormat(urlInstance, context);
152159

160+
// For backward compatibility reasons, we need to let go through Module._load
161+
// again.
162+
shouldBeReloadedByCJSLoader = (format === 'commonjs');
163+
}
153164
validateAttributes(url, format, importAttributes);
154165

155166
return {
156167
__proto__: null,
157168
format,
158169
responseURL,
159170
source,
171+
shouldBeReloadedByCJSLoader,
160172
};
161173
}
162174

lib/internal/modules/esm/loader.js

Lines changed: 35 additions & 26 deletions

0 commit comments

Comments
 (0)