From 410eb97bcea9bf8fb15c1b980a19c3a8daaa6824 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 22 Feb 2019 18:52:38 +0100 Subject: [PATCH] module: fix stat cache The current caching logic broke by [0] because it used destructuring on the module arguments. Since the exported property is a primitive counting it up or down would not have any effect anymore in the module that required that property. The original implementation would cache all stat calls caused during bootstrap. Afterwards it would clear the cache and lazy require calls during runtime would create a new cascading cache for the then loaded modules and clear the cache again. This behavior is now restored. This is difficult to test without exposing a lot of information and therfore the existing tests have been removed (as they could not detect the issue). With the broken implementation it caused each module compilation to reset the cache and therefore minimizing the effect drastically. [0] https://github.com/nodejs/node/pull/19177 PR-URL: https://github.com/nodejs/node/pull/26266 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- lib/internal/modules/cjs/helpers.js | 8 +------ lib/internal/modules/cjs/loader.js | 27 +++++++++++----------- test/fixtures/module-require-depth/one.js | 11 --------- test/fixtures/module-require-depth/two.js | 11 --------- test/parallel/test-module-require-depth.js | 16 ------------- 5 files changed, 15 insertions(+), 58 deletions(-) delete mode 100644 test/fixtures/module-require-depth/one.js delete mode 100644 test/fixtures/module-require-depth/two.js delete mode 100644 test/parallel/test-module-require-depth.js diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 874805beccd39c..df727ff3c88a45 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -11,12 +11,7 @@ function makeRequireFunction(mod) { const Module = mod.constructor; function require(path) { - try { - exports.requireDepth += 1; - return mod.require(path); - } finally { - exports.requireDepth -= 1; - } + return mod.require(path); } function resolve(request, options) { @@ -139,7 +134,6 @@ module.exports = exports = { builtinLibs, makeRequireFunction, normalizeReferrerURL, - requireDepth: 0, stripBOM, stripShebang }; diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 8da55acd9bec50..46b101f3a77df6 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -37,7 +37,6 @@ const { safeGetenv } = internalBinding('credentials'); const { makeRequireFunction, normalizeReferrerURL, - requireDepth, stripBOM, stripShebang } = require('internal/modules/cjs/helpers'); @@ -85,18 +84,17 @@ const { const isWindows = process.platform === 'win32'; +let requireDepth = 0; +let statCache = new Map(); function stat(filename) { filename = path.toNamespacedPath(filename); - const cache = stat.cache; - if (cache !== null) { - const result = cache.get(filename); - if (result !== undefined) return result; - } - const result = internalModuleStat(filename); - if (cache !== null) cache.set(filename, result); + if (statCache === null) statCache = new Map(); + let result = statCache.get(filename); + if (result !== undefined) return result; + result = internalModuleStat(filename); + statCache.set(filename, result); return result; } -stat.cache = null; function updateChildren(parent, child, scan) { var children = parent && parent.children; @@ -710,7 +708,12 @@ Module.prototype.require = function(id) { throw new ERR_INVALID_ARG_VALUE('id', id, 'must be a non-empty string'); } - return Module._load(id, this, /* isMain */ false); + requireDepth++; + try { + return Module._load(id, this, /* isMain */ false); + } finally { + requireDepth--; + } }; @@ -793,8 +796,6 @@ Module.prototype._compile = function(content, filename) { } var dirname = path.dirname(filename); var require = makeRequireFunction(this); - var depth = requireDepth; - if (depth === 0) stat.cache = new Map(); var result; var exports = this.exports; var thisValue = exports; @@ -806,7 +807,7 @@ Module.prototype._compile = function(content, filename) { result = compiledWrapper.call(thisValue, exports, require, module, filename, dirname); } - if (depth === 0) stat.cache = null; + if (requireDepth === 0) statCache = null; return result; }; diff --git a/test/fixtures/module-require-depth/one.js b/test/fixtures/module-require-depth/one.js deleted file mode 100644 index 02b451465bb76c..00000000000000 --- a/test/fixtures/module-require-depth/one.js +++ /dev/null @@ -1,11 +0,0 @@ -// Flags: --expose_internals -'use strict'; -const assert = require('assert'); -const { - requireDepth -} = require('internal/modules/cjs/helpers'); - -exports.requireDepth = requireDepth; -assert.strictEqual(requireDepth, 1); -assert.deepStrictEqual(require('./two'), { requireDepth: 2 }); -assert.strictEqual(requireDepth, 1); diff --git a/test/fixtures/module-require-depth/two.js b/test/fixtures/module-require-depth/two.js deleted file mode 100644 index 5c94c4c89aa9ef..00000000000000 --- a/test/fixtures/module-require-depth/two.js +++ /dev/null @@ -1,11 +0,0 @@ -// Flags: --expose_internals -'use strict'; -const assert = require('assert'); -const { - requireDepth -} = require('internal/modules/cjs/helpers'); - -exports.requireDepth = requireDepth; -assert.strictEqual(requireDepth, 2); -assert.deepStrictEqual(require('./one'), { requireDepth: 1 }); -assert.strictEqual(requireDepth, 2); diff --git a/test/parallel/test-module-require-depth.js b/test/parallel/test-module-require-depth.js deleted file mode 100644 index 0a3fc2826c71f4..00000000000000 --- a/test/parallel/test-module-require-depth.js +++ /dev/null @@ -1,16 +0,0 @@ -// Flags: --expose_internals -'use strict'; -require('../common'); -const fixtures = require('../common/fixtures'); -const assert = require('assert'); -const { - requireDepth -} = require('internal/modules/cjs/helpers'); - -// Module one loads two too so the expected depth for two is, well, two. -assert.strictEqual(requireDepth, 0); -const one = require(fixtures.path('module-require-depth', 'one')); -const two = require(fixtures.path('module-require-depth', 'two')); -assert.deepStrictEqual(one, { requireDepth: 1 }); -assert.deepStrictEqual(two, { requireDepth: 2 }); -assert.strictEqual(requireDepth, 0);