Skip to content

Commit

Permalink
module: warn on using unfinished circular dependency
Browse files Browse the repository at this point in the history
Warn when a non-existent property of an unfinished module.exports
object is being accessed, as that very often indicates the presence
of a hard-to-detect and hard-to-debug problem.

This mechanism is only used if `module.exports` is still a
regular object at the point at which the second, circular `require()`
happens.

The downside is that, temporarily, `module.exports` will have a
prototype other than `Object.prototype`, and that there may
be valid uses of accessing non-existent properties of unfinished
`module.exports` objects.

Performance of circular require calls in general is not
noticeably impacted.

                                               confidence improvement accuracy (*)   (**)  (***)
     module/module-loader-circular.js n=10000                 3.96 %       ±5.12% ±6.82% ±8.89%

Example:

    $ cat a.js
    'use strict';
    const b = require('./b.js');

    exports.fn = () => {};
    $ cat b.js
    'use strict';
    const a = require('./a.js');

    a.fn();
    $ node a.js
    (node:1617) Warning: Accessing non-existent property 'fn' of module exports inside circular dependency
    /tmp/b.js:4
    a.fn();
      ^

    TypeError: a.fn is not a function
        at Object.<anonymous> (/tmp/b.js:4:3)
        [...]

PR-URL: #29935
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
addaleax committed Nov 1, 2019
1 parent fc02cf5 commit d7452b7
Show file tree
Hide file tree
Showing 11 changed files with 143 additions and 0 deletions.
34 changes: 34 additions & 0 deletions benchmark/module/module-loader-circular.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
'use strict';
const fs = require('fs');
const path = require('path');
const common = require('../common.js');

const tmpdir = require('../../test/common/tmpdir');
const benchmarkDirectory =
path.resolve(tmpdir.path, 'benchmark-module-circular');

const bench = common.createBenchmark(main, {
n: [1e4]
});

function main({ n }) {
tmpdir.refresh();

const aDotJS = path.join(benchmarkDirectory, 'a.js');
const bDotJS = path.join(benchmarkDirectory, 'b.js');

fs.mkdirSync(benchmarkDirectory);
fs.writeFileSync(aDotJS, 'require("./b.js");');
fs.writeFileSync(bDotJS, 'require("./a.js");');

bench.start();
for (let i = 0; i < n; i++) {
require(aDotJS);
require(bDotJS);
delete require.cache[aDotJS];
delete require.cache[bDotJS];
}
bench.end(n);

tmpdir.refresh();
}
54 changes: 54 additions & 0 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,52 @@ Module._resolveLookupPaths = function(request, parent) {
return parentDir;
};

function emitCircularRequireWarning(prop) {
process.emitWarning(
`Accessing non-existent property '${String(prop)}' of module exports ` +
'inside circular dependency',
'Warning',
undefined, // code
undefined, // ctor
true); // emit now
}

// A Proxy that can be used as the prototype of a module.exports object and
// warns when non-existend properties are accessed.
const CircularRequirePrototypeWarningProxy = new Proxy({}, {
get(target, prop) {
if (prop in target) return target[prop];
emitCircularRequireWarning(prop);
return undefined;
},

getOwnPropertyDescriptor(target, prop) {
if (ObjectPrototype.hasOwnProperty(target, prop))
return Object.getOwnPropertyDescriptor(target, prop);
emitCircularRequireWarning(prop);
return undefined;
}
});

// Object.prototype and ObjectProtoype refer to our 'primordials' versions
// and are not identical to the versions on the global object.
const PublicObjectPrototype = global.Object.prototype;

function getExportsForCircularRequire(module) {
if (module.exports &&
Object.getPrototypeOf(module.exports) === PublicObjectPrototype &&
// Exclude transpiled ES6 modules / TypeScript code because those may
// employ unusual patterns for accessing 'module.exports'. That should be
// okay because ES6 modules have a different approach to circular
// dependencies anyway.
!module.exports.__esModule) {
// This is later unset once the module is done loading.
Object.setPrototypeOf(module.exports, CircularRequirePrototypeWarningProxy);
}

return module.exports;
}

// Check the cache for the requested file.
// 1. If a module already exists in the cache: return its exports object.
// 2. If the module is native: call
Expand All @@ -776,6 +822,8 @@ Module._load = function(request, parent, isMain) {
const cachedModule = Module._cache[filename];
if (cachedModule !== undefined) {
updateChildren(parent, cachedModule, true);
if (!cachedModule.loaded)
return getExportsForCircularRequire(cachedModule);
return cachedModule.exports;
}
delete relativeResolveCache[relResolveCacheIdentifier];
Expand All @@ -787,6 +835,8 @@ Module._load = function(request, parent, isMain) {
const cachedModule = Module._cache[filename];
if (cachedModule !== undefined) {
updateChildren(parent, cachedModule, true);
if (!cachedModule.loaded)
return getExportsForCircularRequire(cachedModule);
return cachedModule.exports;
}

Expand Down Expand Up @@ -828,6 +878,10 @@ Module._load = function(request, parent, isMain) {
if (parent !== undefined) {
delete relativeResolveCache[relResolveCacheIdentifier];
}
} else if (module.exports &&
Object.getPrototypeOf(module.exports) ===
CircularRequirePrototypeWarningProxy) {
Object.setPrototypeOf(module.exports, PublicObjectPrototype);
}
}

Expand Down
1 change: 1 addition & 0 deletions test/fixtures/cycles/warning-a.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('./warning-b.js');
3 changes: 3 additions & 0 deletions test/fixtures/cycles/warning-b.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const a = require('./warning-a.js');
a.missingPropB;
a[Symbol('someSymbol')];
2 changes: 2 additions & 0 deletions test/fixtures/cycles/warning-esm-transpiled-a.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Object.defineProperty(exports, "__esModule", { value: true });
require('./warning-esm-transpiled-b.js');
1 change: 1 addition & 0 deletions test/fixtures/cycles/warning-esm-transpiled-b.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('./warning-esm-transpiled-a.js').missingPropESM;
2 changes: 2 additions & 0 deletions test/fixtures/cycles/warning-moduleexports-a.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
module.exports = {};
require('./warning-moduleexports-b.js');
1 change: 1 addition & 0 deletions test/fixtures/cycles/warning-moduleexports-b.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('./warning-moduleexports-b.js').missingPropModuleExportsB;
11 changes: 11 additions & 0 deletions test/fixtures/cycles/warning-moduleexports-class-a.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const assert = require('assert');

class Parent {}
class A extends Parent {}

module.exports = A;
require('./warning-moduleexports-class-b.js');
process.nextTick(() => {
assert.strictEqual(module.exports, A);
assert.strictEqual(Object.getPrototypeOf(module.exports), Parent);
});
1 change: 1 addition & 0 deletions test/fixtures/cycles/warning-moduleexports-class-b.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('./warning-moduleexports-class-a.js').missingPropModuleExportsClassB;
33 changes: 33 additions & 0 deletions test/parallel/test-module-circular-dependency-warning.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const fixtures = require('../common/fixtures');

common.expectWarning({
Warning: [
["Accessing non-existent property 'missingPropB' " +
'of module exports inside circular dependency'],
["Accessing non-existent property 'Symbol(someSymbol)' " +
'of module exports inside circular dependency'],
["Accessing non-existent property 'missingPropModuleExportsB' " +
'of module exports inside circular dependency']
]
});
const required = require(fixtures.path('cycles', 'warning-a.js'));
assert.strictEqual(Object.getPrototypeOf(required), Object.prototype);

const requiredWithModuleExportsOverridden =
require(fixtures.path('cycles', 'warning-moduleexports-a.js'));
assert.strictEqual(Object.getPrototypeOf(requiredWithModuleExportsOverridden),
Object.prototype);

// If module.exports is not a regular object, no warning should be emitted.
const classExport =
require(fixtures.path('cycles', 'warning-moduleexports-class-a.js'));
assert.strictEqual(Object.getPrototypeOf(classExport).name, 'Parent');

// If module.exports.__esModule is set, no warning should be emitted.
const esmTranspiledExport =
require(fixtures.path('cycles', 'warning-esm-transpiled-a.js'));
assert.strictEqual(esmTranspiledExport.__esModule, true);

0 comments on commit d7452b7

Please sign in to comment.