module: do not invoke resolve hooks twice for imported cjs

Previously the resolve hook can be invoked twice from the
synthetic module evaluation step of imported CJS in the extra
module._load() call that's invoked on the resolved full path.
Add an option to avoid it, since the resolution and loading
has already been done before.

PR-URL: #61529
Fixes: #57125
Refs: #55808
Refs: #56241
Reviewed-By: Jacob Smith <jacob@frende.me>

Author: joyeecheung

lib/internal/modules/cjs/loader.js

@@ -234,7 +234,7 @@ let statCache = null;
  *
  * See more {@link Module._load}
  */
-function wrapModuleLoad(request, parent, isMain) {
+function wrapModuleLoad(request, parent, isMain, options) {
   const logLabel = `[${parent?.id || ''}] [${request}]`;
   const traceLabel = `require('${request}')`;
   const channel = onRequire();
@@ -247,11 +247,11 @@ function wrapModuleLoad(request, parent, isMain) {
         __proto__: null,
         parentFilename: parent?.filename,
         id: request,
-      }, Module, request, parent, isMain);
+      }, Module, request, parent, isMain, options);
     }
     // No subscribers, skip the wrapping to avoid clobbering stack traces
     // and debugging steps.
-    return Module._load(request, parent, isMain);
+    return Module._load(request, parent, isMain, options);
   } finally {
     endTimer(logLabel, traceLabel);
   }
@@ -1015,9 +1015,10 @@ function getExportsForCircularRequire(module) {
  * @param {string} specifier
  * @param {Module|undefined} parent
  * @param {boolean} isMain
+ * @param {boolean} shouldSkipModuleHooks
  * @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
  */
-function resolveForCJSWithHooks(specifier, parent, isMain) {
+function resolveForCJSWithHooks(specifier, parent, isMain, shouldSkipModuleHooks) {
   let defaultResolvedURL;
   let defaultResolvedFilename;
   let format;
@@ -1040,7 +1041,7 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
   }
 
   // Fast path: no hooks, just return simple results.
-  if (!resolveHooks.length) {
+  if (!resolveHooks.length || shouldSkipModuleHooks) {
     const filename = defaultResolveImpl(specifier, parent, isMain);
     return { __proto__: null, url: defaultResolvedURL, filename, format };
   }
@@ -1093,7 +1094,7 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
   }
 
   const result = { __proto__: null, url, format, filename, parentURL };
-  debug('resolveForCJSWithHooks', specifier, parent?.id, '->', result);
+  debug('resolveForCJSWithHooks', specifier, parent?.id, isMain, shouldSkipModuleHooks, '->', result);
   return result;
 }
 
@@ -1186,8 +1187,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} options Additional options for loading the module
+ * @returns {object}
  */
-Module._load = function(request, parent, isMain) {
+Module._load = function(request, parent, isMain, options = kEmptyObject) {
   let relResolveCacheIdentifier;
   if (parent) {
     debug('Module._load REQUEST %s parent: %s', request, parent.id);
@@ -1210,7 +1213,7 @@ Module._load = function(request, parent, isMain) {
     }
   }
 
-  const resolveResult = resolveForCJSWithHooks(request, parent, isMain);
+  const resolveResult = resolveForCJSWithHooks(request, parent, isMain, options.shouldSkipModuleHooks);
   let { format } = resolveResult;
   const { url, filename } = resolveResult;
 

lib/internal/modules/esm/translators.js

@@ -111,6 +111,7 @@ translators.set('module', function moduleStrategy(url, translateContext, parentU
   return module;
 });
 
+const kShouldSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: true };
 /**
  * Loads a CommonJS module via the ESM Loader sync CommonJS translator.
  * This translator creates its own version of the `require` function passed into CommonJS modules.
@@ -144,7 +145,9 @@ function loadCJSModule(module, source, url, filename, isMain) {
           importAttributes = { __proto__: null, type: 'json' };
           break;
         case '.node':
-          return wrapModuleLoad(specifier, module);
+          // If it gets here in the translators, the hooks must have already been invoked
+          // in the loader. Skip them in the synthetic module evaluation step.
+          return wrapModuleLoad(specifier, module, false, kShouldSkipModuleHooks);
         default:
             // fall through
       }
@@ -282,7 +285,9 @@ function createCJSNoSourceModuleWrap(url, parentURL) {
     debug(`Loading CJSModule ${url}`);
 
     if (!module.loaded) {
-      wrapModuleLoad(filename, null, isMain);
+      // If it gets here in the translators, the hooks must have already been invoked
+      // in the loader. Skip them in the synthetic module evaluation step.
+      wrapModuleLoad(filename, null, isMain, kShouldSkipModuleHooks);
     }
 
     /** @type {import('./loader').ModuleExports} */
@@ -325,7 +330,9 @@ translators.set('require-commonjs-typescript', (url, translateContext, parentURL
 // This goes through Module._load to accommodate monkey-patchers.
 function loadCJSModuleWithModuleLoad(module, source, url, filename, isMain) {
   assert(module === CJSModule._cache[filename]);
-  wrapModuleLoad(filename, undefined, isMain);
+  // If it gets here in the translators, the hooks must have already been invoked
+  // in the loader. Skip them in the synthetic module evaluation step.
+  wrapModuleLoad(filename, undefined, isMain, kShouldSkipModuleHooks);
 }
 
 // Handle CommonJS modules referenced by `import` statements or expressions,

test/fixtures/value.cjs

@@ -0,0 +1 @@
+exports.value = 42;

test/module-hooks/test-module-hooks-load-import-cjs.js

@@ -0,0 +1,20 @@
+'use strict';
+// Test that load hook in imported CJS only gets invoked once.
+
+const common = require('../common');
+const assert = require('assert');
+const { registerHooks } = require('module');
+const path = require('path');
+const { pathToFileURL } = require('url');
+
+const hook = registerHooks({
+  load: common.mustCall(function(url, context, nextLoad) {
+    assert.strictEqual(url, pathToFileURL(path.resolve(__dirname, '../fixtures/value.cjs')).href);
+    return nextLoad(url, context);
+  }, 1),
+});
+
+import('../fixtures/value.cjs').then(common.mustCall((result) => {
+  assert.strictEqual(result.value, 42);
+  hook.deregister();
+}));

test/module-hooks/test-module-hooks-resolve-import-cjs.js

@@ -0,0 +1,18 @@
+'use strict';
+// Test that resolve hook in imported CJS only gets invoked once.
+
+const common = require('../common');
+const assert = require('assert');
+const { registerHooks } = require('module');
+
+const hook = registerHooks({
+  resolve: common.mustCall(function(specifier, context, nextResolve) {
+    assert.strictEqual(specifier, '../fixtures/value.cjs');
+    return nextResolve(specifier, context);
+  }, 1),
+});
+
+import('../fixtures/value.cjs').then(common.mustCall((result) => {
+  assert.strictEqual(result.value, 42);
+  hook.deregister();
+}));