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 <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>

Author: joyeecheung

lib/internal/modules/cjs/loader.js

@@ -177,6 +177,7 @@ const {
   registerHooks,
   resolveHooks,
   resolveWithHooks,
+  validateLoadStrict,
 } = require('internal/modules/customization_hooks');
 const { stripTypeScriptModuleTypes } = require('internal/modules/typescript');
 const packageJsonReader = require('internal/modules/package_json_reader');
@@ -1175,7 +1176,7 @@ function loadBuiltinWithHooks(id, url, format) {
     url ??= `node:${id}`;
     // TODO(joyeecheung): do we really want to invoke the load hook for the builtins?
     const loadResult = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined,
-                                     getCjsConditionsArray(), getDefaultLoad(url, id));
+                                     getCjsConditionsArray(), getDefaultLoad(url, id), validateLoadStrict);
     if (loadResult.format && loadResult.format !== 'builtin') {
       return undefined;  // Format has been overridden, return undefined for the caller to continue loading.
     }
@@ -1791,10 +1792,9 @@ function loadSource(mod, filename, formatFromNode) {
     mod[kURL] = convertCJSFilenameToURL(filename);
   }
 
+  const defaultLoad = getDefaultLoad(mod[kURL], filename);
   const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined,
-                                   getCjsConditionsArray(),
-                                   getDefaultLoad(mod[kURL], filename));
-
+                                   getCjsConditionsArray(), defaultLoad, validateLoadStrict);
   // Reset the module properties with load hook results.
   if (loadResult.format !== undefined) {
     mod[kFormat] = loadResult.format;

lib/internal/modules/customization_hooks.js

@@ -262,29 +262,56 @@ function validateResolve(specifier, context, result) {
  */
 
 /**
- * Validate the result returned by a chain of resolve hook.
+ * Validate the result returned by a chain of load hook.
  * @param {string} url URL passed into the hooks.
  * @param {ModuleLoadContext} context Context passed into the hooks.
  * @param {ModuleLoadResult} result Result produced by load hooks.
  * @returns {ModuleLoadResult}
  */
-function validateLoad(url, context, result) {
+function validateLoadStrict(url, context, result) {
+  validateSourceStrict(url, context, result);
+  validateFormat(url, context, result);
+  return result;
+}
+
+function validateLoadSloppy(url, context, result) {
+  validateSourcePermissive(url, context, result);
+  validateFormat(url, context, result);
+  return result;
+}
+
+function validateSourceStrict(url, context, result) {
   const { source, format } = result;
   // To align with module.register(), the load hooks are still invoked for
   // the builtins even though the default load step only provides null as source,
   // and any source content for builtins provided by the user hooks are ignored.
   if (!StringPrototypeStartsWith(url, 'node:') &&
     typeof result.source !== 'string' &&
       !isAnyArrayBuffer(source) &&
-      !isArrayBufferView(source)) {
+      !isArrayBufferView(source) &&
+      format !== 'addon') {
     throw new ERR_INVALID_RETURN_PROPERTY_VALUE(
       'a string, an ArrayBuffer, or a TypedArray',
       'load',
       'source',
       source,
     );
   }
+}
 
+function validateSourcePermissive(url, context, result) {
+  const { source, format } = result;
+  if (format === 'commonjs' && source == null) {
+    // Accommodate the quirk in defaultLoad used by asynchronous loader hooks
+    // which sets source to null for commonjs.
+    // See: https://github.com/nodejs/node/issues/57327#issuecomment-2701382020
+    return;
+  }
+  validateSourceStrict(url, context, result);
+}
+
+function validateFormat(url, context, result) {
+  const { format } = result;
   if (typeof format !== 'string' && format !== undefined) {
     throw new ERR_INVALID_RETURN_PROPERTY_VALUE(
       'a string',
@@ -293,12 +320,6 @@ function validateLoad(url, context, result) {
       format,
     );
   }
-
-  return {
-    __proto__: null,
-    format,
-    source,
-  };
 }
 
 class ModuleResolveContext {
@@ -338,9 +359,10 @@ let decoder;
  * @param {ImportAttributes|undefined} importAttributes
  * @param {string[]} conditions
  * @param {(url: string, context: ModuleLoadContext) => ModuleLoadResult} defaultLoad
+ * @param {(url: string, context: ModuleLoadContext, result: ModuleLoadResult) => ModuleLoadResult} validateLoad
  * @returns {ModuleLoadResult}
  */
-function loadWithHooks(url, originalFormat, importAttributes, conditions, defaultLoad) {
+function loadWithHooks(url, originalFormat, importAttributes, conditions, defaultLoad, validateLoad) {
   debug('loadWithHooks', url, originalFormat);
   const context = new ModuleLoadContext(originalFormat, importAttributes, conditions);
   if (loadHooks.length === 0) {
@@ -403,4 +425,6 @@ module.exports = {
   registerHooks,
   resolveHooks,
   resolveWithHooks,
+  validateLoadStrict,
+  validateLoadSloppy,
 };

lib/internal/modules/esm/hooks.js

@@ -61,7 +61,7 @@ const {
   SHARED_MEMORY_BYTE_LENGTH,
   WORKER_TO_MAIN_THREAD_NOTIFICATION,
 } = require('internal/modules/esm/shared_constants');
-let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
+let debug = require('internal/util/debuglog').debuglog('async_loader_worker', (fn) => {
   debug = fn;
 });
 let importMetaInitializer;

lib/internal/modules/esm/load.js

@@ -141,22 +141,34 @@ function defaultLoadSync(url, context = kEmptyObject) {
 
   throwIfUnsupportedURLScheme(urlInstance, false);
 
+  let shouldBeReloadedByCJSLoader = false;
   if (urlInstance.protocol === 'node:') {
     source = null;
-  } else if (source == null) {
-    ({ responseURL, source } = getSourceSync(urlInstance, context));
-    context.source = source;
-  }
+    format ??= 'builtin';
+  } else if (format === 'addon') {
+    // Skip loading addon file content. It must be loaded with dlopen from file system.
+    source = null;
+  } else {
+    if (source == null) {
+      ({ responseURL, source } = getSourceSync(urlInstance, context));
+      context = { __proto__: context, source };
+    }
 
-  format ??= defaultGetFormat(urlInstance, context);
+    // Now that we have the source for the module, run `defaultGetFormat` to detect its format.
+    format ??= defaultGetFormat(urlInstance, context);
 
+    // For backward compatibility reasons, we need to let go through Module._load
+    // again.
+    shouldBeReloadedByCJSLoader = (format === 'commonjs');
+  }
   validateAttributes(url, format, importAttributes);
 
   return {
     __proto__: null,
     format,
     responseURL,
     source,
+    shouldBeReloadedByCJSLoader,
   };
 }
 

lib/internal/modules/esm/loader.js

@@ -55,8 +55,9 @@ const {
   resolveWithHooks,
   loadHooks,
   loadWithHooks,
+  validateLoadSloppy,
 } = require('internal/modules/customization_hooks');
-let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;
+let defaultResolve, defaultLoadSync, importMetaInitializer;
 
 const { tracingChannel } = require('diagnostics_channel');
 const onImport = tracingChannel('module.import');
@@ -146,6 +147,10 @@ let hooksProxy;
  * @typedef {ArrayBuffer|TypedArray|string} ModuleSource
  */
 
+/**
+ * @typedef {{ format: ModuleFormat, source: ModuleSource, translatorKey: string }} TranslateContext
+ */
+
 /**
  * This class covers the base machinery of module loading. To add custom
  * behavior you can pass a customizations object and this object will be
@@ -503,18 +508,19 @@ class ModuleLoader {
 
     const loadResult = this.#loadSync(url, { format, importAttributes });
 
+    const formatFromLoad = loadResult.format;
     // Use the synchronous commonjs translator which can deal with cycles.
-    const finalFormat =
-      loadResult.format === 'commonjs' ||
-      loadResult.format === 'commonjs-typescript' ? 'commonjs-sync' : loadResult.format;
+    const translatorKey = (formatFromLoad === 'commonjs' || formatFromLoad === 'commonjs-typescript') ?
+      'commonjs-sync' : formatFromLoad;
 
-    if (finalFormat === 'wasm') {
+    if (translatorKey === 'wasm') {
       assert.fail('WASM is currently unsupported by require(esm)');
     }
 
     const { source } = loadResult;
     const isMain = (parentURL === undefined);
-    const wrap = this.#translate(url, finalFormat, source, parentURL);
+    const translateContext = { format: formatFromLoad, source, translatorKey, __proto__: null };
+    const wrap = this.#translate(url, translateContext, parentURL);
     assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
 
     if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) {
@@ -523,7 +529,7 @@ class ModuleLoader {
 
     const cjsModule = wrap[imported_cjs_symbol];
     if (cjsModule) {
-      assert(finalFormat === 'commonjs-sync');
+      assert(translatorKey === 'commonjs-sync');
       // Check if the ESM initiating import CJS is being required by the same CJS module.
       if (cjsModule?.[kIsExecuting]) {
         const parentFilename = urlToFilename(parentURL);
@@ -547,22 +553,22 @@ class ModuleLoader {
    * Translate a loaded module source into a ModuleWrap. This is run synchronously,
    * but the translator may return the ModuleWrap in a Promise.
    * @param {string} url URL of the module to be translated.
-   * @param {string} format Format of the module to be translated. This is used to find
-   *   matching translators.
-   * @param {ModuleSource} source Source of the module to be translated.
-   * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
+   * @param {TranslateContext} translateContext Context for the translator
+   * @param {string|undefined} parentURL URL of the module initiating the module loading for the first time.
+   *   Undefined if it's the entry point.
    * @returns {ModuleWrap}
    */
-  #translate(url, format, source, parentURL) {
+  #translate(url, translateContext, parentURL) {
+    const { translatorKey, format } = translateContext;
     this.validateLoadResult(url, format);
-    const translator = getTranslators().get(format);
+    const translator = getTranslators().get(translatorKey);
 
     if (!translator) {
-      throw new ERR_UNKNOWN_MODULE_FORMAT(format, url);
+      throw new ERR_UNKNOWN_MODULE_FORMAT(translatorKey, url);
     }
 
-    const result = FunctionPrototypeCall(translator, this, url, source, parentURL === undefined);
-    assert(result instanceof ModuleWrap);
+    const result = FunctionPrototypeCall(translator, this, url, translateContext, parentURL);
+    assert(result instanceof ModuleWrap, `The ${format} module returned is not a ModuleWrap`);
     return result;
   }
 
@@ -575,7 +581,8 @@ class ModuleLoader {
    * @returns {ModuleWrap}
    */
   loadAndTranslateForRequireInImportedCJS(url, loadContext, parentURL) {
-    const { format: formatFromLoad, source } = this.#loadSync(url, loadContext);
+    const loadResult = this.#loadSync(url, loadContext);
+    const formatFromLoad = loadResult.format;
 
     if (formatFromLoad === 'wasm') {  // require(wasm) is not supported.
       throw new ERR_UNKNOWN_MODULE_FORMAT(formatFromLoad, url);
@@ -587,15 +594,16 @@ class ModuleLoader {
       }
     }
 
-    let finalFormat = formatFromLoad;
+    let translatorKey = formatFromLoad;
     if (formatFromLoad === 'commonjs') {
-      finalFormat = 'require-commonjs';
+      translatorKey = 'require-commonjs';
     }
     if (formatFromLoad === 'commonjs-typescript') {
-      finalFormat = 'require-commonjs-typescript';
+      translatorKey = 'require-commonjs-typescript';
     }
 
-    const wrap = this.#translate(url, finalFormat, source, parentURL);
+    const translateContext = { ...loadResult, translatorKey, __proto__: null };
+    const wrap = this.#translate(url, translateContext, parentURL);
     assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
     return wrap;
   }
@@ -610,8 +618,9 @@ class ModuleLoader {
    */
   loadAndTranslate(url, loadContext, parentURL) {
     const maybePromise = this.load(url, loadContext);
-    const afterLoad = ({ format, source }) => {
-      return this.#translate(url, format, source, parentURL);
+    const afterLoad = (loadResult) => {
+      const translateContext = { ...loadResult, translatorKey: loadResult.format, __proto__: null };
+      return this.#translate(url, translateContext, parentURL);
     };
     if (isPromise(maybePromise)) {
       return maybePromise.then(afterLoad);
@@ -837,8 +846,8 @@ class ModuleLoader {
       return this.#customizations.load(url, context);
     }
 
-    defaultLoad ??= require('internal/modules/esm/load').defaultLoad;
-    return defaultLoad(url, context);
+    defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync;
+    return defaultLoadSync(url, context);
   }
 
   /**
@@ -873,7 +882,7 @@ class ModuleLoader {
       // TODO(joyeecheung): construct the ModuleLoadContext in the loaders directly instead
       // of converting them from plain objects in the hooks.
       return loadWithHooks(url, context.format, context.importAttributes, this.#defaultConditions,
-                           this.#loadAndMaybeBlockOnLoaderThread.bind(this));
+                           this.#loadAndMaybeBlockOnLoaderThread.bind(this), validateLoadSloppy);
     }
     return this.#loadAndMaybeBlockOnLoaderThread(url, context);
   }