module: warn on using unfinished circular dependency

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) [...]
nodejs · addaleax · Oct 11, 2019 · Oct 11, 2019 · Oct 11, 2019 · Oct 11, 2019
commit eaa5cc5c95192d9c1dba3fa98404bed905e3d2e6
diff --git a/benchmark/module/module-loader-circular.js b/benchmark/module/module-loader-circular.js
@@ -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();
+}
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -656,6 +656,47 @@ 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) {
+    // 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
@@ -676,6 +717,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];
@@ -687,6 +730,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;
   }
 
@@ -728,6 +773,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);
     }
   }
 

diff --git a/test/fixtures/cycles/warning-a.js b/test/fixtures/cycles/warning-a.js
@@ -0,0 +1 @@
+require('./warning-b.js');
diff --git a/test/fixtures/cycles/warning-b.js b/test/fixtures/cycles/warning-b.js
@@ -0,0 +1 @@
+require('./warning-a.js').missingPropB;
diff --git a/test/fixtures/cycles/warning-moduleexports-a.js b/test/fixtures/cycles/warning-moduleexports-a.js
@@ -0,0 +1,2 @@
+module.exports = {};
+require('./warning-moduleexports-b.js');
diff --git a/test/fixtures/cycles/warning-moduleexports-b.js b/test/fixtures/cycles/warning-moduleexports-b.js
@@ -0,0 +1 @@
+require('./warning-moduleexports-b.js').missingPropModuleExportsB;
diff --git a/test/fixtures/cycles/warning-moduleexports-class-a.js b/test/fixtures/cycles/warning-moduleexports-class-a.js
@@ -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);
+});
diff --git a/test/fixtures/cycles/warning-moduleexports-class-b.js b/test/fixtures/cycles/warning-moduleexports-class-b.js
@@ -0,0 +1 @@
+require('./warning-moduleexports-class-a.js').missingPropModuleExportsClassB;
diff --git a/test/parallel/test-module-circular-dependency-warning.js b/test/parallel/test-module-circular-dependency-warning.js
@@ -0,0 +1,26 @@
+'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 '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');
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		module.exports = {};
		require('./warning-moduleexports-b.js');
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		require('./warning-moduleexports-b.js').missingPropModuleExportsB;
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		require('./warning-moduleexports-class-a.js').missingPropModuleExportsClassB;