Skip to content

Commit

Permalink
module: fix stat cache
Browse files Browse the repository at this point in the history
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] #19177

PR-URL: #26266
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
BridgeAR committed Mar 1, 2019
1 parent 3fce5cf commit 410eb97
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 58 deletions.
8 changes: 1 addition & 7 deletions lib/internal/modules/cjs/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -139,7 +134,6 @@ module.exports = exports = {
builtinLibs,
makeRequireFunction,
normalizeReferrerURL,
requireDepth: 0,
stripBOM,
stripShebang
};
27 changes: 14 additions & 13 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ const { safeGetenv } = internalBinding('credentials');
const {
makeRequireFunction,
normalizeReferrerURL,
requireDepth,
stripBOM,
stripShebang
} = require('internal/modules/cjs/helpers');
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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--;
}
};


Expand Down Expand Up @@ -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;
Expand All @@ -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;
};

Expand Down
11 changes: 0 additions & 11 deletions test/fixtures/module-require-depth/one.js

This file was deleted.

11 changes: 0 additions & 11 deletions test/fixtures/module-require-depth/two.js

This file was deleted.

16 changes: 0 additions & 16 deletions test/parallel/test-module-require-depth.js

This file was deleted.

0 comments on commit 410eb97

Please sign in to comment.