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 <[email protected]>

Author: joyeecheung

lib/internal/modules/cjs/loader.js

@@ -1094,20 +1094,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.
@@ -1254,10 +1260,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);
@@ -1281,7 +1287,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;
 
@@ -1369,6 +1378,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
@@ -545,11 +575,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 ||
@@ -663,7 +693,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) {
@@ -699,15 +729,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);
   }
 
@@ -722,31 +755,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}}
@@ -762,14 +809,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);
   }
 
@@ -780,17 +832,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) {