module: cache negative stat results in the CJS loader

benchmark/module/module-resolve-misses.js

@@ -0,0 +1,58 @@
+'use strict';
+
+const fs = require('fs');
+const path = require('path');
+const { spawnSync } = require('child_process');
+const common = require('../common.js');
+
+const tmpdir = require('../../test/common/tmpdir');
+const benchmarkDirectory = tmpdir.resolve('nodejs-benchmark-module');
+
+const bench = common.createBenchmark(main, {
+  depth: [4, 12],
+  deps: [200],
+  n: [30],
+});
+
+function main({ depth, deps, n }) {
+  tmpdir.refresh();
+
+  // The only `node_modules` that exists: at the root, above the requirer.
+  const nodeModules = path.join(benchmarkDirectory, 'node_modules');
+  for (let i = 0; i < deps; i++) {
+    const dep = path.join(nodeModules, `dep${i}`);
+    fs.mkdirSync(dep, { recursive: true });
+    fs.writeFileSync(
+      path.join(dep, 'package.json'),
+      `{"name":"dep${i}","main":"index.js"}`,
+    );
+    fs.writeFileSync(path.join(dep, 'index.js'), 'module.exports = {};');
+  }
+
+  // The requirer, nested `depth` levels down. Resolution walks up from here and
+  // finds no `node_modules` until the root.
+  const nested = path.join(benchmarkDirectory, ...'x'.repeat(depth).split(''));
+  fs.mkdirSync(nested, { recursive: true });
+
+  let entrySource = '';
+  for (let i = 0; i < deps; i++) {
+    entrySource += `require('dep${i}');\n`;
+  }
+  const entry = path.join(nested, 'entry.js');
+  fs.writeFileSync(entry, entrySource);
+
+  const cmd = process.execPath || process.argv[0];
+  const warmup = 3;
+  for (let i = -warmup; i < n; i++) {
+    if (i === 0) {
+      bench.start();
+    }
+    const child = spawnSync(cmd, [entry]);
+    if (child.status !== 0) {
+      throw new Error(`Child process stopped with exit code ${child.status}`);
+    }
+  }
+  bench.end(n);
+
+  tmpdir.refresh();
+}

lib/internal/modules/cjs/loader.js

@@ -266,21 +266,38 @@ function wrapModuleLoad(request, parent, isMain, options) {
 
 /**
  * Get a path's properties, using an in-memory cache to minimize lookups.
+ *
+ * Successful results (0 = file, 1 = directory) are always cached. Negative
+ * results (libuv error codes, e.g. -ENOENT) are only cached for speculative
+ * probes, i.e. paths that resolution guesses at rather than paths the user
+ * actually asked for: the extension candidates tried by `tryExtensions` and
+ * the `node_modules` ancestor directories walked for bare specifiers. Those
+ * misses are numerous and repeat across sibling and descendant modules, so
+ * caching them is where the win is.
+ *
+ * A cached negative is also only *read back* by a speculative probe. A stat of
+ * a path the user named directly ignores it and re-stats. That keeps the
+ * behaviour of https://github.com/nodejs/node/pull/36642 intact: a module that
+ * is missing on a failed `require()` and then created can still be picked up by
+ * a later `require()` in the same tree.
  * @param {string} filename Absolute path to the file
+ * @param {object} [options]
+ * @param {boolean} [options.isSpeculativeProbe] Whether this is a resolution
+ *   guess rather than a path the user named, which makes the negative result
+ *   cacheable and lets a previously cached negative be reused.
  * @returns {number}
  */
-function stat(filename) {
+function stat(filename, { isSpeculativeProbe = false } = kEmptyObject) {
   // Guard against internal bugs where a non-string filename is passed in by mistake.
   assert(typeof filename === 'string');
 
   filename = path.toNamespacedPath(filename);
   if (statCache !== null) {
     const result = statCache.get(filename);
-    if (result !== undefined) { return result; }
+    if (result !== undefined && (result >= 0 || isSpeculativeProbe)) { return result; }
   }
   const result = internalFsBinding.internalModuleStat(filename);
-  if (statCache !== null && result >= 0) {
-    // Only set cache when `internalModuleStat(filename)` succeeds.
+  if (statCache !== null && (result >= 0 || isSpeculativeProbe)) {
     statCache.set(filename, result);
   }
   return result;
@@ -557,7 +574,7 @@ function tryPackage(requestPath, exts, isMain, originalPath) {
   }
 
   const filename = path.resolve(requestPath, pkg);
-  let actual = tryFile(filename, isMain) ||
+  let actual = tryFile(filename, { isMain }) ||
     tryExtensions(filename, exts, isMain) ||
     tryExtensions(path.resolve(filename, 'index'), exts, isMain);
   if (actual === false) {
@@ -589,11 +606,14 @@ function tryPackage(requestPath, exts, isMain, originalPath) {
  * Check if the file exists and is not a directory if using `--preserve-symlinks` and `isMain` is false or
  * `--preserve-symlinks-main` and `isMain` is true , keep symlinks intact, otherwise resolve to the absolute realpath.
  * @param {string} requestPath The path to the file to load.
- * @param {boolean} isMain Whether the file is the main module.
+ * @param {object} [options]
+ * @param {boolean} [options.isMain] Whether the file is the main module.
+ * @param {boolean} [options.isSpeculativeProbe] Whether `requestPath` is a
+ *   resolution guess rather than a path the user named. See {@link stat}.
  * @returns {string|undefined}
  */
-function tryFile(requestPath, isMain) {
-  const rc = _stat(requestPath);
+function tryFile(requestPath, { isMain = false, isSpeculativeProbe = false } = kEmptyObject) {
+  const rc = _stat(requestPath, { isSpeculativeProbe });
   if (rc !== 0) { return; }
   if (getOptionValue(isMain ? '--preserve-symlinks-main' : '--preserve-symlinks')) {
     return path.resolve(requestPath);
@@ -603,14 +623,16 @@ function tryFile(requestPath, isMain) {
 
 /**
  * Given a path, check if the file exists with any of the set extensions.
+ * Each candidate is a speculative probe: the extension is appended by
+ * resolution, not named by the user, so its misses are cacheable.
  * @param {string} basePath The path and filename without extension
  * @param {string[]} exts The extensions to try
  * @param {boolean} isMain Whether the module is the main module
  * @returns {string|false}
  */
 function tryExtensions(basePath, exts, isMain) {
   for (let i = 0; i < exts.length; i++) {
-    const filename = tryFile(basePath + exts[i], isMain);
+    const filename = tryFile(basePath + exts[i], { isMain, isSpeculativeProbe: true });
 
     if (filename) {
       return filename;
@@ -821,7 +843,10 @@ Module._findPath = function(request, paths, isMain, conditions = getCjsCondition
     if (typeof curPath !== 'string') {
       throw new ERR_INVALID_ARG_TYPE('paths', 'array of strings', paths);
     }
-    if (insidePath && curPath && _stat(curPath) < 1) {
+    // A candidate lookup directory (typically a `node_modules` ancestor) is a
+    // speculative probe: most of the chain does not exist, and every bare
+    // specifier in the tree re-walks the same missing ancestors.
+    if (insidePath && curPath && _stat(curPath, { isSpeculativeProbe: true }) < 1) {
       continue;
     }
 

test/parallel/test-module-negative-stat-cache.js

@@ -0,0 +1,70 @@
+'use strict';
+require('../common');
+
+// This tests that the CommonJS loader's per-require-tree stat cache caches
+// negative (not-found) results for *speculative* probes: the extension
+// candidates tried by `tryExtensions` and the `node_modules` ancestor
+// directories walked for bare specifiers.
+//
+// A path the user named directly is not negatively cached, so the behaviour of
+// https://github.com/nodejs/node/pull/36642 is preserved: a module that is
+// missing on a failed `require()` and then created is still picked up by a
+// later `require()` in the same tree. That case is covered by
+// test-module-cache.js and asserted again at the end of this file.
+//
+// The stat cache is populated and read internally by the loader, so it is not
+// directly observable from user code. These tests make it observable by
+// mutating the filesystem between two probes of the same path within one
+// require tree.
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+// An extensionless specifier is resolved by probing `<name>.js`, `<name>.json`,
+// `<name>.node`, ... in order. Those candidate paths are appended by resolution
+// rather than named by the user, so their misses are cached for the rest of the
+// tree.
+{
+  const dir = tmpdir.resolve('speculative');
+  fs.mkdirSync(dir);
+
+  const specifier = path.join(dir, 'mod');
+
+  // Nothing exists yet: every extension candidate misses and is cached.
+  assert.throws(
+    () => require(specifier),
+    { code: 'MODULE_NOT_FOUND' },
+    'expected the module to be missing before it is created',
+  );
+
+  // Create one of the candidates that was just probed and missed.
+  fs.writeFileSync(`${specifier}.js`, 'module.exports = "late";');
+
+  // Resolving the same extensionless specifier probes `mod.js` again, but that
+  // negative result is cached, so the freshly-created file is not observed.
+  assert.throws(
+    () => require(specifier),
+    { code: 'MODULE_NOT_FOUND' },
+    'a negative result for a speculative extension probe must be cached',
+  );
+}
+
+// A path the user named directly is not negatively cached, so creating the file
+// mid-tree makes a later require in the same tree resolve it.
+{
+  const explicit = tmpdir.resolve('explicit.js');
+
+  assert.throws(
+    () => require(explicit),
+    { code: 'MODULE_NOT_FOUND' },
+  );
+
+  fs.writeFileSync(explicit, 'module.exports = "created";');
+
+  // A negative result for a user-named path must not be cached.
+  assert.strictEqual(require(explicit), 'created');
+}