module: reduce circular dependency of internal/modules/cjs/loader

Previously `internal/bootstrap/pre_execution.js` requires `internal/modules/cjs/loader.js` which in turn requires `internal/bootstrap/pre_execution.js`. This patch moves the entry point execution logic out of `pre_execution.js` and puts it into `internal/modules/run_main.js`. It also tests that `Module.runMain` can be monkey-patched before further deprecation/refactoring can be done. Also added an internal assertion `hasLoadedAnyUserCJSModule` for documentation purposes. PR-URL: #30349 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
nodejs · Jan 14, 2020 · 4de6097 · 4de6097
1 parent 0dd8605
commit 4de6097
Show file tree

Hide file tree

Showing 25 changed files with 158 additions and 109 deletions.
diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js
@@ -9,7 +9,7 @@ const {
 const { getOptionValue } = require('internal/options');
 const { Buffer } = require('buffer');
 const { ERR_MANIFEST_ASSERT_INTEGRITY } = require('internal/errors').codes;
-const path = require('path');
+const assert = require('internal/assert');
 
 function prepareMainThreadExecution(expandArgv1 = false) {
   // Patch the process object with legacy properties and normalizations
@@ -64,6 +64,9 @@ function prepareMainThreadExecution(expandArgv1 = false) {
   initializeDeprecations();
   initializeCJSLoader();
   initializeESMLoader();
+
+  const CJSLoader = require('internal/modules/cjs/loader');
+  assert(!CJSLoader.hasLoadedAnyUserCJSModule);
   loadPreloadModules();
   initializeFrozenIntrinsics();
 }
@@ -398,7 +401,11 @@ function initializePolicy() {
 }
 
 function initializeCJSLoader() {
-  require('internal/modules/cjs/loader').Module._initPaths();
+  const CJSLoader = require('internal/modules/cjs/loader');
+  CJSLoader.Module._initPaths();
+  // TODO(joyeecheung): deprecate this in favor of a proper hook?
+  CJSLoader.Module.runMain =
+    require('internal/modules/run_main').executeUserEntryPoint;
 }
 
 function initializeESMLoader() {
@@ -446,74 +453,11 @@ function loadPreloadModules() {
   }
 }
 
-function resolveMainPath(main) {
-  const { toRealPath, Module: CJSModule } =
-      require('internal/modules/cjs/loader');
-
-  // Note extension resolution for the main entry point can be deprecated in a
-  // future major.
-  let mainPath = CJSModule._findPath(path.resolve(main), null, true);
-  if (!mainPath)
-    return;
-
-  const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
-  if (!preserveSymlinksMain)
-    mainPath = toRealPath(mainPath);
-
-  return mainPath;
-}
-
-function shouldUseESMLoader(mainPath) {
-  const experimentalModules = getOptionValue('--experimental-modules');
-  if (!experimentalModules)
-    return false;
-  const userLoader = getOptionValue('--experimental-loader');
-  if (userLoader)
-    return true;
-  const experimentalSpecifierResolution =
-    getOptionValue('--experimental-specifier-resolution');
-  if (experimentalSpecifierResolution === 'node')
-    return true;
-  // Determine the module format of the main
-  if (mainPath && mainPath.endsWith('.mjs'))
-    return true;
-  if (!mainPath || mainPath.endsWith('.cjs'))
-    return false;
-  const { readPackageScope } = require('internal/modules/cjs/loader');
-  const pkg = readPackageScope(mainPath);
-  return pkg && pkg.data.type === 'module';
-}
-
-function runMainESM(mainPath) {
-  const esmLoader = require('internal/process/esm_loader');
-  const { pathToFileURL } = require('internal/url');
-  const { hasUncaughtExceptionCaptureCallback } =
-      require('internal/process/execution');
-  return esmLoader.initializeLoader().then(() => {
-    const main = path.isAbsolute(mainPath) ?
-      pathToFileURL(mainPath).href : mainPath;
-    return esmLoader.ESMLoader.import(main);
-  }).catch((e) => {
-    if (hasUncaughtExceptionCaptureCallback()) {
-      process._fatalException(e);
-      return;
-    }
-    internalBinding('errors').triggerUncaughtException(
-      e,
-      true /* fromPromise */
-    );
-  });
-}
-
-
 module.exports = {
   patchProcessObject,
-  resolveMainPath,
-  runMainESM,
   setupCoverageHooks,
   setupWarningHandler,
   setupDebugEnv,
-  shouldUseESMLoader,
   prepareMainThreadExecution,
   initializeDeprecations,
   initializeESMLoader,

diff --git a/lib/internal/main/run_main_module.js b/lib/internal/main/run_main_module.js
@@ -6,11 +6,13 @@ const {
 
 prepareMainThreadExecution(true);
 
-const CJSModule = require('internal/modules/cjs/loader').Module;
-
 markBootstrapComplete();
 
 // Note: this loads the module through the ESM loader if
 // --experimental-loader is provided or --experimental-modules is on
-// and the module is determined to be an ES module
-CJSModule.runMain(process.argv[1]);
+// and the module is determined to be an ES module. This hangs from the CJS
+// module loader because we currently allow monkey-patching of the module
+// loaders in the preloaded scripts through require('module').
+// runMain here might be monkey-patched by users in --require.
+// XXX: the monkey-patchability here should probably be deprecated.
+require('internal/modules/cjs/loader').Module.runMain(process.argv[1]);
diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js
@@ -110,6 +110,9 @@ port.on('message', (message) => {
     initializeDeprecations();
     initializeCJSLoader();
     initializeESMLoader();
+
+    const CJSLoader = require('internal/modules/cjs/loader');
+    assert(!CJSLoader.hasLoadedAnyUserCJSModule);
     loadPreloadModules();
     initializeFrozenIntrinsics();
     publicWorker.parentPort = publicPort;
@@ -138,8 +141,9 @@ port.on('message', (message) => {
       evalScript('[worker eval]', filename);
     } else {
       // script filename
-      const CJSModule = require('internal/modules/cjs/loader').Module;
-      CJSModule.runMain(process.argv[1] = filename);
+      // runMain here might be monkey-patched by users in --require.
+      // XXX: the monkey-patchability here should probably be deprecated.
+      CJSLoader.Module.runMain(process.argv[1] = filename);
     }
   } else if (message.type === STDIO_PAYLOAD) {
     const { stream, chunk, encoding } = message;

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -74,21 +74,23 @@ const manifest = getOptionValue('--experimental-policy') ?
   null;
 const { compileFunction } = internalBinding('contextify');
 
+// Whether any user-provided CJS modules had been loaded (executed).
+// Used for internal assertions.
+let hasLoadedAnyUserCJSModule = false;
+
 const {
   ERR_INVALID_ARG_VALUE,
   ERR_INVALID_OPT_VALUE,
   ERR_INVALID_PACKAGE_CONFIG,
   ERR_REQUIRE_ESM
 } = require('internal/errors').codes;
 const { validateString } = require('internal/validators');
-const {
-  resolveMainPath,
-  shouldUseESMLoader,
-  runMainESM
-} = require('internal/bootstrap/pre_execution');
 const pendingDeprecation = getOptionValue('--pending-deprecation');
 
-module.exports = { wrapSafe, Module, toRealPath, readPackageScope };
+module.exports = {
+  wrapSafe, Module, toRealPath, readPackageScope,
+  get hasLoadedAnyUserCJSModule() { return hasLoadedAnyUserCJSModule; }
+};
 
 let asyncESM, ModuleJob, ModuleWrap, kInstantiated;
 
@@ -1148,6 +1150,7 @@ Module.prototype._compile = function(content, filename) {
     result = compiledWrapper.call(thisValue, exports, require, module,
                                   filename, dirname);
   }
+  hasLoadedAnyUserCJSModule = true;
   if (requireDepth === 0) statCache = null;
   return result;
 };
@@ -1197,18 +1200,6 @@ Module._extensions['.node'] = function(module, filename) {
   return process.dlopen(module, path.toNamespacedPath(filename));
 };
 
-// Bootstrap main module.
-Module.runMain = function(main = process.argv[1]) {
-  const resolvedMain = resolveMainPath(main);
-  const useESMLoader = shouldUseESMLoader(resolvedMain);
-  module.exports.asyncRunMain = useESMLoader;
-  if (useESMLoader) {
-    runMainESM(resolvedMain || main);
-  } else {
-    Module._load(main, null, true);
-  }
-};
-
 function createRequireFromPath(filename) {
   // Allow a directory to be passed as the filename
   const trailingSlash =

diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js
@@ -0,0 +1,80 @@
+'use strict';
+
+const CJSLoader = require('internal/modules/cjs/loader');
+const { Module, toRealPath, readPackageScope } = CJSLoader;
+const { getOptionValue } = require('internal/options');
+const path = require('path');
+
+function resolveMainPath(main) {
+  // Note extension resolution for the main entry point can be deprecated in a
+  // future major.
+  // Module._findPath is monkey-patchable here.
+  let mainPath = Module._findPath(path.resolve(main), null, true);
+  if (!mainPath)
+    return;
+
+  const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
+  if (!preserveSymlinksMain)
+    mainPath = toRealPath(mainPath);
+
+  return mainPath;
+}
+
+function shouldUseESMLoader(mainPath) {
+  const experimentalModules = getOptionValue('--experimental-modules');
+  if (!experimentalModules)
+    return false;
+  const userLoader = getOptionValue('--experimental-loader');
+  if (userLoader)
+    return true;
+  const experimentalSpecifierResolution =
+    getOptionValue('--experimental-specifier-resolution');
+  if (experimentalSpecifierResolution === 'node')
+    return true;
+  // Determine the module format of the main
+  if (mainPath && mainPath.endsWith('.mjs'))
+    return true;
+  if (!mainPath || mainPath.endsWith('.cjs'))
+    return false;
+  const pkg = readPackageScope(mainPath);
+  return pkg && pkg.data.type === 'module';
+}
+
+function runMainESM(mainPath) {
+  const esmLoader = require('internal/process/esm_loader');
+  const { pathToFileURL } = require('internal/url');
+  const { hasUncaughtExceptionCaptureCallback } =
+      require('internal/process/execution');
+  return esmLoader.initializeLoader().then(() => {
+    const main = path.isAbsolute(mainPath) ?
+      pathToFileURL(mainPath).href : mainPath;
+    return esmLoader.ESMLoader.import(main);
+  }).catch((e) => {
+    if (hasUncaughtExceptionCaptureCallback()) {
+      process._fatalException(e);
+      return;
+    }
+    internalBinding('errors').triggerUncaughtException(
+      e,
+      true /* fromPromise */
+    );
+  });
+}
+
+// For backwards compatibility, we have to run a bunch of
+// monkey-patchable code that belongs to the CJS loader (exposed by
+// `require('module')`) even when the entry point is ESM.
+function executeUserEntryPoint(main = process.argv[1]) {
+  const resolvedMain = resolveMainPath(main);
+  const useESMLoader = shouldUseESMLoader(resolvedMain);
+  if (useESMLoader) {
+    runMainESM(resolvedMain || main);
+  } else {
+    // Module._load is the monkey-patchable CJS module loader.
+    Module._load(main, null, true);
+  }
+}
+
+module.exports = {
+  executeUserEntryPoint
+};
diff --git a/node.gyp b/node.gyp
@@ -145,6 +145,7 @@
       'lib/internal/main/run_main_module.js',
       'lib/internal/main/run_third_party_main.js',
       'lib/internal/main/worker_thread.js',
+      'lib/internal/modules/run_main.js',
       'lib/internal/modules/cjs/helpers.js',
       'lib/internal/modules/cjs/loader.js',
       'lib/internal/modules/esm/loader.js',

diff --git a/test/fixtures/monkey-patch-run-main.js b/test/fixtures/monkey-patch-run-main.js
@@ -0,0 +1,8 @@
+'use strict';
+
+const oldRunMain = require('module').runMain;
+
+require('module').runMain = function(...args) {
+  console.log('runMain is monkey patched!');
+  oldRunMain(...args);
+};
diff --git a/test/message/core_line_numbers.out b/test/message/core_line_numbers.out
@@ -10,5 +10,5 @@ RangeError: Invalid input
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*:*)
     at Module.load (internal/modules/cjs/loader.js:*:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*:*)
-    at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*)
     at internal/main/run_main_module.js:*:*
diff --git a/test/message/error_exit.out b/test/message/error_exit.out
@@ -12,7 +12,7 @@ AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*:*)
     at Module.load (internal/modules/cjs/loader.js:*:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*:*)
-    at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*)
     at internal/main/run_main_module.js:*:* {
   generatedMessage: true,
   code: 'ERR_ASSERTION',

diff --git a/test/message/esm_loader_not_found.out b/test/message/esm_loader_not_found.out
@@ -11,8 +11,8 @@ Error: Cannot find package 'i-dont-exist' imported from *
     at Loader.import (internal/modules/esm/loader.js:*:*)
     at internal/process/esm_loader.js:*:*
     at Object.initializeLoader (internal/process/esm_loader.js:*:*)
-    at runMainESM (internal/bootstrap/pre_execution.js:*:*)
-    at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at runMainESM (internal/modules/run_main.js:*:*)
+    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*)
     at internal/main/run_main_module.js:*:* {
   code: 'ERR_MODULE_NOT_FOUND'
 }
diff --git a/test/message/events_unhandled_error_common_trace.out b/test/message/events_unhandled_error_common_trace.out
@@ -10,7 +10,7 @@ Error: foo:bar
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*:*)
     at Module.load (internal/modules/cjs/loader.js:*:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*:*)
-    at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*)
     at internal/main/run_main_module.js:*:*
 Emitted 'error' event at:
     at quux (*events_unhandled_error_common_trace.js:*:*)

diff --git a/test/message/events_unhandled_error_nexttick.out b/test/message/events_unhandled_error_nexttick.out
@@ -8,7 +8,7 @@ Error
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*:*)
     at Module.load (internal/modules/cjs/loader.js:*:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*:*)
-    at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*)
     at internal/main/run_main_module.js:*:*
 Emitted 'error' event at:
     at *events_unhandled_error_nexttick.js:*:*

diff --git a/test/message/events_unhandled_error_sameline.out b/test/message/events_unhandled_error_sameline.out
@@ -8,7 +8,7 @@ Error
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*:*)
     at Module.load (internal/modules/cjs/loader.js:*:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*:*)
-    at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*)
     at internal/main/run_main_module.js:*:*
 Emitted 'error' event at:
     at Object.<anonymous> (*events_unhandled_error_sameline.js:*:*)

diff --git a/test/message/events_unhandled_error_subclass.out b/test/message/events_unhandled_error_subclass.out
@@ -8,7 +8,7 @@ Error
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*:*)
     at Module.load (internal/modules/cjs/loader.js:*:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*:*)
-    at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*)
     at internal/main/run_main_module.js:*:*
 Emitted 'error' event on Foo instance at:
     at Object.<anonymous> (*events_unhandled_error_subclass.js:*:*)

diff --git a/test/message/if-error-has-good-stack.out b/test/message/if-error-has-good-stack.out
@@ -15,7 +15,7 @@ AssertionError [ERR_ASSERTION]: ifError got unwanted exception: test error
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*:*)
     at Module.load (internal/modules/cjs/loader.js:*:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*:*)
-    at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*)
     at internal/main/run_main_module.js:*:* {
   generatedMessage: false,
   code: 'ERR_ASSERTION',
@@ -28,7 +28,7 @@ AssertionError [ERR_ASSERTION]: ifError got unwanted exception: test error
       at Object.Module._extensions..js (internal/modules/cjs/loader.js:*:*)
       at Module.load (internal/modules/cjs/loader.js:*:*)
       at Function.Module._load (internal/modules/cjs/loader.js:*:*)
-      at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
+      at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*)
       at internal/main/run_main_module.js:*:*
   expected: null,
   operator: 'ifError'

diff --git a/test/message/throw_error_with_getter_throw_traced.out b/test/message/throw_error_with_getter_throw_traced.out
@@ -9,4 +9,4 @@ Thrown at:
     at Module._extensions..js (internal/modules/cjs/loader.js:*:*)
     at Module.load (internal/modules/cjs/loader.js:*:*)
     at Module._load (internal/modules/cjs/loader.js:*:*)
-    at Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at executeUserEntryPoint (internal/modules/run_main.js:*:*)
diff --git a/test/message/throw_null_traced.out b/test/message/throw_null_traced.out
@@ -9,4 +9,4 @@ Thrown at:
     at Module._extensions..js (internal/modules/cjs/loader.js:*:*)
     at Module.load (internal/modules/cjs/loader.js:*:*)
     at Module._load (internal/modules/cjs/loader.js:*:*)
-    at Module.runMain (internal/modules/cjs/loader.js:*:*)
+    at executeUserEntryPoint (internal/modules/run_main.js:*:*)