modules: fix sync hook short-circuit in require() in imported CJS

lib/internal/modules/cjs/loader.js

@@ -1072,20 +1072,26 @@ function defaultResolveImplForCJSLoading(specifier, parent, isMain, options) {
   return wrapResolveFilename(specifier, parent, isMain, options);
 }
 
+/**
+ * @typedef {{
+ *   resolved?: {url?: string, format?: string, filename: string},
+ *   shouldSkipModuleHooks?: boolean,
+ *   source?: string|ArrayBufferView|ArrayBuffer,
+ *   requireResolveOptions?: ResolveFilenameOptions,
+ * }} CJSModuleLoadInternalOptions
+ */
+
 /**
  * Resolve a module request for CommonJS, invoking hooks from module.registerHooks()
  * if necessary.
  * @param {string} specifier
  * @param {Module|undefined} parent
  * @param {boolean} isMain
- * @param {object} internalResolveOptions
- * @param {boolean} internalResolveOptions.shouldSkipModuleHooks Whether to skip module hooks.
- * @param {ResolveFilenameOptions} internalResolveOptions.requireResolveOptions Options from require.resolve().
- *   Only used when it comes from require.resolve().
+ * @param {CJSModuleLoadInternalOptions} internalOptions
  * @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
  */
-function resolveForCJSWithHooks(specifier, parent, isMain, internalResolveOptions) {
-  const { requireResolveOptions, shouldSkipModuleHooks } = internalResolveOptions;
+function resolveForCJSWithHooks(specifier, parent, isMain, internalOptions) {
+  const { requireResolveOptions, shouldSkipModuleHooks } = internalOptions;
   const defaultResolveImpl = requireResolveOptions ?
     wrapResolveFilename : defaultResolveImplForCJSLoading;
   // Fast path: no hooks, just return simple results.
@@ -1232,10 +1238,10 @@ function loadBuiltinWithHooks(id, url, format) {
  * @param {string} request Specifier of module to load via `require`
  * @param {Module} parent Absolute path of the module importing the child
  * @param {boolean} isMain Whether the module is the main entry point
- * @param {object|undefined} internalResolveOptions Additional options for loading the module
+ * @param {CJSModuleLoadInternalOptions|undefined} internalOptions Additional options for loading the module
  * @returns {object}
  */
-Module._load = function(request, parent, isMain, internalResolveOptions = kEmptyObject) {
+Module._load = function(request, parent, isMain, internalOptions = kEmptyObject) {
   let relResolveCacheIdentifier;
   if (parent) {
     debug('Module._load REQUEST %s parent: %s', request, parent.id);
@@ -1258,7 +1264,10 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty
     }
   }
 
-  const resolveResult = resolveForCJSWithHooks(request, parent, isMain, internalResolveOptions);
+  // If the module has been resolved by a short-circuiting synchronous resolve hook,
+  // avoid running the default resolution from disk again.
+  const resolveResult = internalOptions.resolved ??
+    resolveForCJSWithHooks(request, parent, isMain, internalOptions);
   let { format } = resolveResult;
   const { url, filename } = resolveResult;
 
@@ -1345,6 +1354,14 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty
     module[kLastModuleParent] = parent;
   }
 
+  // The module source was provided by a short-circuiting synchronous hook,
+  // assign them into the module to avoid triggering the default load step again.
+  if (internalOptions.source !== undefined) {
+    module[kModuleSource] ??= internalOptions.source;
+    module[kURL] ??= url;
+    module[kFormat] ??= format;
+  }
+
   if (parent !== undefined) {
     relativeResolveCache[relResolveCacheIdentifier] = filename;
   }

lib/internal/modules/esm/load.js

@@ -145,7 +145,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
 
   throwIfUnsupportedURLScheme(urlInstance, false);
 
-  let shouldBeReloadedByCJSLoader = false;
   if (urlInstance.protocol === 'node:') {
     source = null;
     format ??= 'builtin';
@@ -160,10 +159,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
 
     // 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);
 
@@ -172,7 +167,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
     format,
     responseURL,
     source,
-    shouldBeReloadedByCJSLoader,
   };
 }
 

lib/internal/modules/esm/loader.js

@@ -124,7 +124,19 @@ const { defaultLoadSync, throwUnknownModuleFormat } = require('internal/modules/
  */
 
 /**
- * @typedef {{ format: ModuleFormat, source: ModuleSource, translatorKey: string }} TranslateContext
+ * @typedef {{format: string, url: string, isResolvedBySyncHooks: boolean}} ResolveResult
+ */
+
+/**
+ * @typedef {{
+ *   url: string,
+ *   format: ModuleFormat,
+ *   source: ModuleSource,
+ *   responseURL?: string,
+ *   translatorKey: string,
+ *   isResolvedBySyncHooks: boolean,
+ *   isSourceLoadedSynchronously: boolean,
+ * }} TranslateContext
  */
 
 /**
@@ -385,19 +397,25 @@ class ModuleLoader {
   /**
    * Load a module and translate it into a ModuleWrap for require(esm).
    * This is run synchronously, and the translator always return a ModuleWrap synchronously.
-   * @param {string} url URL of the module to be translated.
+   * @param {ResolveResult} resolveResult Result from the resolve step.
    * @param {object} loadContext See {@link load}
    * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
    * @param {ModuleRequest} request Module request.
    * @returns {ModuleWrap}
    */
-  loadAndTranslateForImportInRequiredESM(url, loadContext, parentURL, request) {
+  loadAndTranslateForImportInRequiredESM(resolveResult, loadContext, parentURL, request) {
+    const { url } = resolveResult;
     const loadResult = this.#loadSync(url, loadContext);
     // Use the synchronous commonjs translator which can deal with cycles.
     const formatFromLoad = loadResult.format;
     const translatorKey = (formatFromLoad === 'commonjs' || formatFromLoad === 'commonjs-typescript') ?
       'commonjs-sync' : formatFromLoad;
-    const translateContext = { ...loadResult, translatorKey, __proto__: null };
+    const translateContext = {
+      ...resolveResult,
+      ...loadResult,
+      translatorKey,
+      __proto__: null,
+    };
     const wrap = this.#translate(url, translateContext, parentURL);
     assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
 
@@ -446,12 +464,13 @@ class ModuleLoader {
   /**
    * Load a module and translate it into a ModuleWrap for require() in imported CJS.
    * This is run synchronously, and the translator always return a ModuleWrap synchronously.
-   * @param {string} url URL of the module to be translated.
+   * @param {ResolveResult} resolveResult Result from the resolve step.
    * @param {object} loadContext See {@link load}
    * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
    * @returns {ModuleWrap}
    */
-  loadAndTranslateForRequireInImportedCJS(url, loadContext, parentURL) {
+  loadAndTranslateForRequireInImportedCJS(resolveResult, loadContext, parentURL) {
+    const { url } = resolveResult;
     const loadResult = this.#loadSync(url, loadContext);
     const formatFromLoad = loadResult.format;
 
@@ -473,7 +492,12 @@ class ModuleLoader {
       translatorKey = 'require-commonjs-typescript';
     }
 
-    const translateContext = { ...loadResult, translatorKey, __proto__: null };
+    const translateContext = {
+      ...resolveResult,
+      ...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;
@@ -482,15 +506,21 @@ class ModuleLoader {
   /**
    * Load a module and translate it into a ModuleWrap for ordinary imported ESM.
    * This may be run asynchronously if there are asynchronous module loader hooks registered.
-   * @param {string} url URL of the module to be translated.
+   * @param {ResolveResult} resolveResult Result from the resolve step.
    * @param {object} loadContext See {@link load}
    * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
    * @returns {Promise<ModuleWrap>|ModuleWrap}
    */
-  loadAndTranslate(url, loadContext, parentURL) {
+  loadAndTranslate(resolveResult, loadContext, parentURL) {
+    const { url } = resolveResult;
     const maybePromise = this.load(url, loadContext);
     const afterLoad = (loadResult) => {
-      const translateContext = { ...loadResult, translatorKey: loadResult.format, __proto__: null };
+      const translateContext = {
+        ...resolveResult,
+        ...loadResult,
+        translatorKey: loadResult.format,
+        __proto__: null,
+      };
       return this.#translate(url, translateContext, parentURL);
     };
     if (isPromise(maybePromise)) {
@@ -506,7 +536,7 @@ class ModuleLoader {
    * the module should be linked by the time this returns. Otherwise it may still have
    * pending module requests.
    * @param {string} parentURL See {@link getOrCreateModuleJob}
-   * @param {{format: string, url: string}} resolveResult
+   * @param {ResolveResult} resolveResult
    * @param {ModuleRequest} request Module request.
    * @param {ModuleRequestType} requestType Type of the module request.
    * @returns {ModuleJobBase} The (possibly pending) module job
@@ -535,11 +565,11 @@ class ModuleLoader {
 
     let moduleOrModulePromise;
     if (requestType === kRequireInImportedCJS) {
-      moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(url, context, parentURL);
+      moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(resolveResult, context, parentURL);
     } else if (requestType === kImportInRequiredESM) {
-      moduleOrModulePromise = this.loadAndTranslateForImportInRequiredESM(url, context, parentURL, request);
+      moduleOrModulePromise = this.loadAndTranslateForImportInRequiredESM(resolveResult, context, parentURL, request);
     } else {
-      moduleOrModulePromise = this.loadAndTranslate(url, context, parentURL);
+      moduleOrModulePromise = this.loadAndTranslate(resolveResult, context, parentURL);
     }
 
     if (requestType === kImportInRequiredESM || requestType === kRequireInImportedCJS ||
@@ -653,7 +683,7 @@ class ModuleLoader {
    * @param {string} [parentURL] The URL of the module where the module request is initiated.
    *   It's undefined if it's from the root module.
    * @param {ModuleRequest} request Module request.
-   * @returns {Promise<{format: string, url: string}>|{format: string, url: string}}
+   * @returns {Promise<ResolveResult>|ResolveResult}
    */
   #resolve(parentURL, request) {
     if (this.isForAsyncLoaderHookWorker) {
@@ -689,15 +719,18 @@ class ModuleLoader {
   /**
    * This is the default resolve step for module.registerHooks(), which incorporates asynchronous hooks
    * from module.register() which are run in a blocking fashion for it to be synchronous.
+   * @param {{isResolvedByDefaultResolve: boolean}} out Output object to track whether the default resolve was used
+   *   without polluting the user-visible resolve result.
    * @param {string|URL} specifier See {@link resolveSync}.
    * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context
    *   See {@link resolveSync}.
    * @returns {{ format: string, url: string }}
    */
-  #resolveAndMaybeBlockOnLoaderThread(specifier, context) {
+  #resolveAndMaybeBlockOnLoaderThread(out, specifier, context) {
     if (this.#asyncLoaderHooks?.resolveSync) {
       return this.#asyncLoaderHooks.resolveSync(specifier, context.parentURL, context.importAttributes);
     }
+    out.isResolvedByDefaultResolve = true;
     return this.#cachedDefaultResolve(specifier, context);
   }
 
@@ -712,31 +745,45 @@ class ModuleLoader {
    * @param {boolean} [shouldSkipSyncHooks] Whether to skip the synchronous hooks registered by module.registerHooks().
    *   This is used to maintain compatibility for the re-invented require.resolve (in imported CJS customized
    *   by module.register()`) which invokes the CJS resolution separately from the hook chain.
-   * @returns {{ format: string, url: string }}
+   * @returns {ResolveResult}
    */
   resolveSync(parentURL, request, shouldSkipSyncHooks = false) {
     const specifier = `${request.specifier}`;
     const importAttributes = request.attributes ?? kEmptyObject;
+    // Use an output parameter to track the state and avoid polluting the user-visible resolve results.
+    const out = { isResolvedByDefaultResolve: false, __proto__: null };
 
+    let result;
+    let isResolvedBySyncHooks = false;
     if (!shouldSkipSyncHooks && syncResolveHooks.length) {
       // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step.
-      return resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
-                                  this.#resolveAndMaybeBlockOnLoaderThread.bind(this));
+      result = resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
+                                    this.#resolveAndMaybeBlockOnLoaderThread.bind(this, out));
+      // If the default step ran, sync hooks did not short-circuit the resolution.
+      isResolvedBySyncHooks = !out.isResolvedByDefaultResolve;
+    } else {
+      const context = {
+        ...request,
+        conditions: this.#defaultConditions,
+        parentURL,
+        importAttributes,
+        __proto__: null,
+      };
+      result = this.#resolveAndMaybeBlockOnLoaderThread(out, specifier, context);
     }
-    const context = {
-      ...request,
-      conditions: this.#defaultConditions,
-      parentURL,
-      importAttributes,
-      __proto__: null,
-    };
-    return this.#resolveAndMaybeBlockOnLoaderThread(specifier, context);
+    result.isResolvedBySyncHooks = isResolvedBySyncHooks;
+    return result;
   }
 
   /**
    * Provide source that is understood by one of Node's translators. Handles customization hooks,
    * if any.
-   * @typedef { {format: ModuleFormat, source: ModuleSource }} LoadResult
+   * @typedef {{
+   *   format: ModuleFormat,
+   *   source: ModuleSource,
+   *   responseURL?: string,
+   *   isSourceLoadedSynchronously: boolean,
+   * }} LoadResult
    * @param {string} url The URL of the module to be loaded.
    * @param {object} context Metadata about the module
    * @returns {Promise<LoadResult> | LoadResult}}
@@ -752,14 +799,19 @@ class ModuleLoader {
   /**
    * This is the default load step for module.registerHooks(), which incorporates asynchronous hooks
    * from module.register() which are run in a blocking fashion for it to be synchronous.
+   * @param {{isSourceLoadedSynchronously: boolean}} out
+   *   Output object to track whether the source was loaded synchronously without polluting
+   *   the user-visible load result.
    * @param {string} url See {@link load}
    * @param {object} context See {@link load}
    * @returns {{ format: ModuleFormat, source: ModuleSource }}
    */
-  #loadAndMaybeBlockOnLoaderThread(url, context) {
+  #loadAndMaybeBlockOnLoaderThread(out, url, context) {
     if (this.#asyncLoaderHooks?.loadSync) {
+      out.isSourceLoadedSynchronously = false;
       return this.#asyncLoaderHooks.loadSync(url, context);
     }
+    out.isSourceLoadedSynchronously = true;
     return defaultLoadSync(url, context);
   }
 
@@ -770,17 +822,32 @@ class ModuleLoader {
    * This is here to support `require()` in imported CJS and `module.registerHooks()` hooks.
    * @param {string} url See {@link load}
    * @param {object} [context] See {@link load}
-   * @returns {{ format: ModuleFormat, source: ModuleSource }}
+   * @returns {LoadResult}
    */
   #loadSync(url, context) {
+    // Use an output parameter to track the state and avoid polluting the user-visible resolve results.
+    const out = {
+      isSourceLoadedSynchronously: true,
+      __proto__: null,
+    };
+    let result;
     if (syncLoadHooks.length) {
       // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step.
       // TODO(joyeecheung): construct the ModuleLoadContext in the loaders directly instead
       // of converting them from plain objects in the hooks.
-      return loadWithSyncHooks(url, context.format, context.importAttributes, this.#defaultConditions,
-                               this.#loadAndMaybeBlockOnLoaderThread.bind(this), validateLoadSloppy);
+      result = loadWithSyncHooks(
+        url,
+        context.format,
+        context.importAttributes,
+        this.#defaultConditions,
+        this.#loadAndMaybeBlockOnLoaderThread.bind(this, out),
+        validateLoadSloppy,
+      );
+    } else {
+      result = this.#loadAndMaybeBlockOnLoaderThread(out, url, context);
     }
-    return this.#loadAndMaybeBlockOnLoaderThread(url, context);
+    result.isSourceLoadedSynchronously = out.isSourceLoadedSynchronously;
+    return result;
   }
 
   validateLoadResult(url, format) {