module: only set cache when finding module succeeds

zhangyongsheng · zhangyongsheng · commit aecd5ebf49c9 · 2021-02-12T22:08:40.000+08:00
PR-URL: #36642 Fixes: #36638 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/benchmark/module/module-require.js b/benchmark/module/module-require.js
@@ -0,0 +1,80 @@
+'use strict';
+
+const fs = require('fs');
+const path = require('path');
+const common = require('../common.js');
+const tmpdir = require('../../test/common/tmpdir');
+const benchmarkDirectory = path.join(tmpdir.path, 'nodejs-benchmark-module');
+
+const bench = common.createBenchmark(main, {
+  type: ['.js', '.json', 'dir'],
+  n: [1e4],
+});
+
+function main({ type, n }) {
+  tmpdir.refresh();
+  createEntryPoint(n);
+
+  switch (type) {
+    case '.js':
+      measureJSFile(n);
+      break;
+    case '.json':
+      measureJSONFile(n);
+      break;
+    case 'dir':
+      measureDir(n);
+  }
+
+  tmpdir.refresh();
+}
+
+function measureJSFile(n) {
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    require(`${benchmarkDirectory}/${i}`);
+  }
+  bench.end(n);
+}
+
+function measureJSONFile(n) {
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    require(`${benchmarkDirectory}/json_${i}.json`);
+  }
+  bench.end(n);
+}
+
+function measureDir(n) {
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    require(`${benchmarkDirectory}${i}`);
+  }
+  bench.end(n);
+}
+
+function createEntryPoint(n) {
+  fs.mkdirSync(benchmarkDirectory);
+
+  const JSFileContent = 'module.exports = [];';
+  const JSONFileContent = '[]';
+
+  for (let i = 0; i < n; i++) {
+    // JS file.
+    fs.writeFileSync(`${benchmarkDirectory}/${i}.js`, JSFileContent);
+
+    // JSON file.
+    fs.writeFileSync(`${benchmarkDirectory}/json_${i}.json`, JSONFileContent);
+
+    // Dir.
+    fs.mkdirSync(`${benchmarkDirectory}${i}`);
+    fs.writeFileSync(
+      `${benchmarkDirectory}${i}/package.json`,
+      '{"main": "index.js"}'
+    );
+    fs.writeFileSync(
+      `${benchmarkDirectory}${i}/index.js`,
+      JSFileContent
+    );
+  }
+}
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -148,7 +148,10 @@ function stat(filename) {
     if (result !== undefined) return result;
   }
   const result = internalModuleStat(filename);
-  if (statCache !== null) statCache.set(filename, result);
+  if (statCache !== null && result >= 0) {
+    // Only set cache when `internalModuleStat(filename)` succeeds.
+    statCache.set(filename, result);
+  }
   return result;
 }
 
diff --git a/test/parallel/test-module-cache.js b/test/parallel/test-module-cache.js
@@ -0,0 +1,19 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+const filePath = path.join(tmpdir.path, 'test-module-cache.json');
+assert.throws(
+  () => require(filePath),
+  { code: 'MODULE_NOT_FOUND' }
+);
+
+fs.writeFileSync(filePath, '[]');
+
+const content = require(filePath);
+assert.strictEqual(Array.isArray(content), true);
+assert.strictEqual(content.length, 0);

Original file line number	Diff line number	Diff line change
`@@ -148,7 +148,10 @@ function stat(filename) {`
`148`	`148`	`if (result !== undefined) return result;`
`149`	`149`	`}`
`150`	`150`	`const result = internalModuleStat(filename);`
`151`		`- if (statCache !== null) statCache.set(filename, result);`
	`151`	`+ if (statCache !== null && result >= 0) {`
	`152`	+ // Only set cache when `internalModuleStat(filename)` succeeds.
	`153`	`+ statCache.set(filename, result);`
	`154`	`+ }`
`152`	`155`	`return result;`
`153`	`156`	`}`
`154`	`157`