From 7b8058a39e3485f7c6aeab87e9953f30c9974afd Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 27 Nov 2018 21:36:22 +0800 Subject: [PATCH] process: refactor the bootstrap mode branching for readability This patch refactors the branches for choosing the mode to run Node.js in `internal/bootstrap/node.js`. Instead of inlining the decision making all in `startup`, we create a `startExecution()` function which either detects and start the non-user-code mode, or prepares for user code execution (worker setup, preloading modules) and starts user code execution. We use early returns when we decide the mode to run Node.js in for fewer indentations and better readability. This patch also adds a few comments about the command-line switches and a few TODOs to remove underscore properties on `process` that are mainly used for bootstrap mode branching. It also includes a few other refactoring such as inlining functions/variables that are not reused and removing the default argument of `evalScript` for better clarity. PR-URL: https://github.com/nodejs/node/pull/24673 Reviewed-By: Gus Caplan Reviewed-By: Daniel Bevenius Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig --- lib/internal/bootstrap/node.js | 263 ++++++++++-------- test/message/assert_throws_stack.out | 1 + test/message/core_line_numbers.out | 2 +- test/message/error_exit.out | 3 +- test/message/eval_messages.out | 8 + .../events_unhandled_error_common_trace.out | 6 +- .../events_unhandled_error_nexttick.out | 5 +- .../events_unhandled_error_sameline.out | 5 +- test/message/nexttick_throw.out | 2 + test/message/stdin_messages.out | 8 +- .../unhandled_promise_trace_warnings.out | 6 + test/message/util_inspect_error.out | 5 + 12 files changed, 190 insertions(+), 124 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index a207dd648f7f7c..673a3f1b1d889e 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -104,18 +104,13 @@ } const { getOptionValue } = NativeModule.require('internal/options'); - const helpOption = getOptionValue('--help'); - const completionBashOption = getOptionValue('--completion-bash'); - const experimentalModulesOption = getOptionValue('--experimental-modules'); - const experimentalVMModulesOption = - getOptionValue('--experimental-vm-modules'); - const experimentalWorkerOption = getOptionValue('--experimental-worker'); - if (helpOption) { + + if (getOptionValue('--help')) { NativeModule.require('internal/print_help').print(process.stdout); return; } - if (completionBashOption) { + if (getOptionValue('--completion-bash')) { NativeModule.require('internal/bash_completion').print(process.stdout); return; } @@ -135,7 +130,7 @@ setupQueueMicrotask(); } - if (experimentalWorkerOption) { + if (getOptionValue('--experimental-worker')) { setupDOMException(); } @@ -167,8 +162,10 @@ 'DeprecationWarning', 'DEP0062', startup, true); } - if (experimentalModulesOption || experimentalVMModulesOption) { - if (experimentalModulesOption) { + const experimentalModules = getOptionValue('--experimental-modules'); + const experimentalVMModules = getOptionValue('--experimental-vm-modules'); + if (experimentalModules || experimentalVMModules) { + if (experimentalModules) { process.emitWarning( 'The ESM module loader is experimental.', 'ExperimentalWarning', undefined); @@ -228,22 +225,33 @@ setupAllowedFlags(); - // There are various modes that Node can run in. The most common two - // are running from a script and running the REPL - but there are a few - // others like the debugger or running --eval arguments. Here we decide - // which mode we run in. + startExecution(); + } + + // There are various modes that Node can run in. The most common two + // are running from a script and running the REPL - but there are a few + // others like the debugger or running --eval arguments. Here we decide + // which mode we run in. + function startExecution() { + // This means we are in a Worker context, and any script execution + // will be directed by the worker module. if (internalBinding('worker').getEnvMessagePort() !== undefined) { - // This means we are in a Worker context, and any script execution - // will be directed by the worker module. NativeModule.require('internal/worker').setupChild(evalScript); - } else if (NativeModule.exists('_third_party_main')) { - // To allow people to extend Node in different ways, this hook allows - // one to drop a file lib/_third_party_main.js into the build - // directory which will be executed instead of Node's normal loading. + return; + } + + // To allow people to extend Node in different ways, this hook allows + // one to drop a file lib/_third_party_main.js into the build + // directory which will be executed instead of Node's normal loading. + if (NativeModule.exists('_third_party_main')) { process.nextTick(() => { NativeModule.require('_third_party_main'); }); - } else if (process.argv[1] === 'inspect' || process.argv[1] === 'debug') { + return; + } + + // `node inspect ...` or `node debug ...` + if (process.argv[1] === 'inspect' || process.argv[1] === 'debug') { if (process.argv[1] === 'debug') { process.emitWarning( '`node debug` is deprecated. Please use `node inspect` instead.', @@ -254,95 +262,136 @@ process.nextTick(() => { NativeModule.require('internal/deps/node-inspect/lib/_inspect').start(); }); + return; + } - } else if (process.profProcess) { + // `node --prof-process` + // TODO(joyeecheung): use internal/options instead of process.profProcess + if (process.profProcess) { NativeModule.require('internal/v8_prof_processor'); - } else { - // There is user code to be run. - - // If this is a worker in cluster mode, start up the communication - // channel. This needs to be done before any user code gets executed - // (including preload modules). - if (process.argv[1] && process.env.NODE_UNIQUE_ID) { - const cluster = NativeModule.require('cluster'); - cluster._setupWorker(); - // Make sure it's not accidentally inherited by child processes. - delete process.env.NODE_UNIQUE_ID; + return; + } + + // There is user code to be run. + prepareUserCodeExecution(); + executeUserCode(); + } + + function prepareUserCodeExecution() { + // If this is a worker in cluster mode, start up the communication + // channel. This needs to be done before any user code gets executed + // (including preload modules). + if (process.argv[1] && process.env.NODE_UNIQUE_ID) { + const cluster = NativeModule.require('cluster'); + cluster._setupWorker(); + // Make sure it's not accidentally inherited by child processes. + delete process.env.NODE_UNIQUE_ID; + } + + // For user code, we preload modules if `-r` is passed + // TODO(joyeecheung): use internal/options instead of + // process._preload_modules + if (process._preload_modules) { + const { + _preloadModules + } = NativeModule.require('internal/modules/cjs/loader'); + _preloadModules(process._preload_modules); + } + } + + function executeUserCode() { + // User passed `-e` or `--eval` arguments to Node without `-i` or + // `--interactive`. + // Note that the name `forceRepl` is merely an alias of `interactive` + // in code. + // TODO(joyeecheung): use internal/options instead of + // process._eval/process._forceRepl + if (process._eval != null && !process._forceRepl) { + const { + addBuiltinLibsToObject + } = NativeModule.require('internal/modules/cjs/helpers'); + addBuiltinLibsToObject(global); + evalScript('[eval]', wrapForBreakOnFirstLine(process._eval)); + return; + } + + // If the first argument is a file name, run it as a main script + if (process.argv[1] && process.argv[1] !== '-') { + // Expand process.argv[1] into a full path. + const path = NativeModule.require('path'); + process.argv[1] = path.resolve(process.argv[1]); + + const CJSModule = NativeModule.require('internal/modules/cjs/loader'); + + // If user passed `-c` or `--check` arguments to Node, check its syntax + // instead of actually running the file. + // TODO(joyeecheung): use internal/options instead of + // process._syntax_check_only + if (process._syntax_check_only != null) { + const fs = NativeModule.require('fs'); + // Read the source. + const filename = CJSModule._resolveFilename(process.argv[1]); + const source = fs.readFileSync(filename, 'utf-8'); + checkScriptSyntax(source, filename); + process.exit(0); } - if (process._eval != null && !process._forceRepl) { - // User passed '-e' or '--eval' arguments to Node without '-i' or - // '--interactive'. - preloadModules(); - - const { - addBuiltinLibsToObject - } = NativeModule.require('internal/modules/cjs/helpers'); - addBuiltinLibsToObject(global); - evalScript('[eval]'); - } else if (process.argv[1] && process.argv[1] !== '-') { - // Make process.argv[1] into a full path. - const path = NativeModule.require('path'); - process.argv[1] = path.resolve(process.argv[1]); - - const CJSModule = NativeModule.require('internal/modules/cjs/loader'); - - preloadModules(); - // Check if user passed `-c` or `--check` arguments to Node. - if (process._syntax_check_only != null) { - const fs = NativeModule.require('fs'); - // Read the source. - const filename = CJSModule._resolveFilename(process.argv[1]); - const source = fs.readFileSync(filename, 'utf-8'); - checkScriptSyntax(source, filename); - process.exit(0); + // Note: this actually tries to run the module as a ESM first if + // --experimental-modules is on. + // TODO(joyeecheung): can we move that logic to here? Note that this + // is an undocumented method available via `require('module').runMain` + CJSModule.runMain(); + return; + } + + // Create the REPL if `-i` or `--interactive` is passed, or if + // stdin is a TTY. + // Note that the name `forceRepl` is merely an alias of `interactive` + // in code. + if (process._forceRepl || NativeModule.require('tty').isatty(0)) { + const cliRepl = NativeModule.require('internal/repl'); + cliRepl.createInternalRepl(process.env, (err, repl) => { + if (err) { + throw err; } - CJSModule.runMain(); - } else { - preloadModules(); - // If -i or --interactive were passed, or stdin is a TTY. - if (process._forceRepl || NativeModule.require('tty').isatty(0)) { - // REPL - const cliRepl = NativeModule.require('internal/repl'); - cliRepl.createInternalRepl(process.env, (err, repl) => { - if (err) { - throw err; - } - repl.on('exit', () => { - if (repl._flushing) { - repl.pause(); - return repl.once('flushHistory', () => { - process.exit(); - }); - } + repl.on('exit', () => { + if (repl._flushing) { + repl.pause(); + return repl.once('flushHistory', () => { process.exit(); }); - }); - - if (process._eval != null) { - // User passed '-e' or '--eval' - evalScript('[eval]'); } - } else { - // Read all of stdin - execute it. - process.stdin.setEncoding('utf8'); - - let code = ''; - process.stdin.on('data', (d) => { - code += d; - }); - - process.stdin.on('end', function() { - if (process._syntax_check_only != null) { - checkScriptSyntax(code, '[stdin]'); - } else { - process._eval = code; - evalScript('[stdin]'); - } - }); - } + process.exit(); + }); + }); + + // User passed '-e' or '--eval' along with `-i` or `--interactive` + if (process._eval != null) { + evalScript('[eval]', wrapForBreakOnFirstLine(process._eval)); } + return; } + + // Stdin is not a TTY, we will read it and execute it. + readAndExecuteStdin(); + } + + function readAndExecuteStdin() { + process.stdin.setEncoding('utf8'); + + let code = ''; + process.stdin.on('data', (d) => { + code += d; + }); + + process.stdin.on('end', () => { + if (process._syntax_check_only != null) { + checkScriptSyntax(code, '[stdin]'); + } else { + process._eval = code; + evalScript('[stdin]', wrapForBreakOnFirstLine(process._eval)); + } + }); } function setupTraceCategoryState() { @@ -651,7 +700,7 @@ return `process.binding('inspector').callAndPauseOnStart(${fn}, {})`; } - function evalScript(name, body = wrapForBreakOnFirstLine(process._eval)) { + function evalScript(name, body) { const CJSModule = NativeModule.require('internal/modules/cjs/loader'); const path = NativeModule.require('path'); const cwd = tryGetCwd(path); @@ -673,16 +722,6 @@ process._tickCallback(); } - // Load preload modules. - function preloadModules() { - if (process._preload_modules) { - const { - _preloadModules - } = NativeModule.require('internal/modules/cjs/loader'); - _preloadModules(process._preload_modules); - } - } - function checkScriptSyntax(source, filename) { const CJSModule = NativeModule.require('internal/modules/cjs/loader'); const vm = NativeModule.require('vm'); diff --git a/test/message/assert_throws_stack.out b/test/message/assert_throws_stack.out index cf96ee42c98cb7..371bd9ff917b86 100644 --- a/test/message/assert_throws_stack.out +++ b/test/message/assert_throws_stack.out @@ -18,3 +18,4 @@ AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal: at * at * at * + at * diff --git a/test/message/core_line_numbers.out b/test/message/core_line_numbers.out index fc647e41b92d6b..59953132fa0542 100644 --- a/test/message/core_line_numbers.out +++ b/test/message/core_line_numbers.out @@ -12,4 +12,4 @@ RangeError: Invalid input at tryModuleLoad (internal/modules/cjs/loader.js:*:*) at Function.Module._load (internal/modules/cjs/loader.js:*:*) at Function.Module.runMain (internal/modules/cjs/loader.js:*:*) - at startup (internal/bootstrap/node.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) diff --git a/test/message/error_exit.out b/test/message/error_exit.out index be46bbc1ae9b5a..39692a7b6e4482 100644 --- a/test/message/error_exit.out +++ b/test/message/error_exit.out @@ -14,5 +14,6 @@ AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: at tryModuleLoad (internal/modules/cjs/loader.js:*:*) at Function.Module._load (internal/modules/cjs/loader.js:*:*) at Function.Module.runMain (internal/modules/cjs/loader.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) at startup (internal/bootstrap/node.js:*:*) - at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) diff --git a/test/message/eval_messages.out b/test/message/eval_messages.out index d01dfe547cbd73..c26c63d2b0d016 100644 --- a/test/message/eval_messages.out +++ b/test/message/eval_messages.out @@ -9,6 +9,8 @@ SyntaxError: Strict mode code may not include a with statement at Object. ([eval]-wrapper:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) at evalScript (internal/bootstrap/node.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) at startup (internal/bootstrap/node.js:*:*) at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) 42 @@ -24,6 +26,8 @@ Error: hello at Object. ([eval]-wrapper:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) at evalScript (internal/bootstrap/node.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) at startup (internal/bootstrap/node.js:*:*) at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) @@ -38,6 +42,8 @@ Error: hello at Object. ([eval]-wrapper:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) at evalScript (internal/bootstrap/node.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) at startup (internal/bootstrap/node.js:*:*) at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) 100 @@ -52,6 +58,8 @@ ReferenceError: y is not defined at Object. ([eval]-wrapper:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) at evalScript (internal/bootstrap/node.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) at startup (internal/bootstrap/node.js:*:*) at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) diff --git a/test/message/events_unhandled_error_common_trace.out b/test/message/events_unhandled_error_common_trace.out index f6b74e9991306b..67be22554fb41e 100644 --- a/test/message/events_unhandled_error_common_trace.out +++ b/test/message/events_unhandled_error_common_trace.out @@ -12,11 +12,11 @@ Error: foo:bar at tryModuleLoad (internal/modules/cjs/loader.js:*:*) at Function.Module._load (internal/modules/cjs/loader.js:*:*) at Function.Module.runMain (internal/modules/cjs/loader.js:*:*) - at startup (internal/bootstrap/node.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) Emitted 'error' event at: at quux (*events_unhandled_error_common_trace.js:*:*) at Object. (*events_unhandled_error_common_trace.js:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) [... lines matching original stack trace ...] - at startup (internal/bootstrap/node.js:*:*) - at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) diff --git a/test/message/events_unhandled_error_nexttick.out b/test/message/events_unhandled_error_nexttick.out index 4d0eba3a8537a6..d043839fe2c40c 100644 --- a/test/message/events_unhandled_error_nexttick.out +++ b/test/message/events_unhandled_error_nexttick.out @@ -10,12 +10,15 @@ Error at tryModuleLoad (internal/modules/cjs/loader.js:*:*) at Function.Module._load (internal/modules/cjs/loader.js:*:*) at Function.Module.runMain (internal/modules/cjs/loader.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) at startup (internal/bootstrap/node.js:*:*) - at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) Emitted 'error' event at: at process.nextTick (*events_unhandled_error_nexttick.js:*:*) at internalTickCallback (internal/process/next_tick.js:*:*) at process._tickCallback (internal/process/next_tick.js:*:*) at Function.Module.runMain (internal/modules/cjs/loader.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) at startup (internal/bootstrap/node.js:*:*) at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) diff --git a/test/message/events_unhandled_error_sameline.out b/test/message/events_unhandled_error_sameline.out index 55841cdbc345d7..cd2b76120326ca 100644 --- a/test/message/events_unhandled_error_sameline.out +++ b/test/message/events_unhandled_error_sameline.out @@ -10,10 +10,11 @@ Error at tryModuleLoad (internal/modules/cjs/loader.js:*:*) at Function.Module._load (internal/modules/cjs/loader.js:*:*) at Function.Module.runMain (internal/modules/cjs/loader.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) at startup (internal/bootstrap/node.js:*:*) - at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) Emitted 'error' event at: at Object. (*events_unhandled_error_sameline.js:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) [... lines matching original stack trace ...] - at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) + at startup (internal/bootstrap/node.js:*:*) diff --git a/test/message/nexttick_throw.out b/test/message/nexttick_throw.out index 09b00de80fa080..5f3c85f874c599 100644 --- a/test/message/nexttick_throw.out +++ b/test/message/nexttick_throw.out @@ -7,5 +7,7 @@ ReferenceError: undefined_reference_error_maker is not defined at internalTickCallback (internal/process/next_tick.js:*:*) at process._tickCallback (internal/process/next_tick.js:*:*) at Function.Module.runMain (internal/modules/cjs/loader.js:*:*) + at executeUserCode (internal/bootstrap/node.js:*:*) + at startExecution (internal/bootstrap/node.js:*:*) at startup (internal/bootstrap/node.js:*:*) at bootstrapNodeJSCore (internal/bootstrap/node.js:*:*) diff --git a/test/message/stdin_messages.out b/test/message/stdin_messages.out index bbf996bb56a13c..6ec340c0c42cd3 100644 --- a/test/message/stdin_messages.out +++ b/test/message/stdin_messages.out @@ -9,7 +9,7 @@ SyntaxError: Strict mode code may not include a with statement at Object. ([stdin]-wrapper:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) at evalScript (internal/bootstrap/node.js:*:*) - at Socket. (internal/bootstrap/node.js:*:*) + at Socket.process.stdin.on (internal/bootstrap/node.js:*:*) at Socket.emit (events.js:*:*) at endReadableNT (_stream_readable.js:*:*) at process.internalTickCallback (internal/process/next_tick.js:*:*) @@ -26,7 +26,7 @@ Error: hello at Object. ([stdin]-wrapper:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) at evalScript (internal/bootstrap/node.js:*:*) - at Socket. (internal/bootstrap/node.js:*:*) + at Socket.process.stdin.on (internal/bootstrap/node.js:*:*) at Socket.emit (events.js:*:*) at endReadableNT (_stream_readable.js:*:*) at process.internalTickCallback (internal/process/next_tick.js:*:*) @@ -41,7 +41,7 @@ Error: hello at Object. ([stdin]-wrapper:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) at evalScript (internal/bootstrap/node.js:*:*) - at Socket. (internal/bootstrap/node.js:*:*) + at Socket.process.stdin.on (internal/bootstrap/node.js:*:*) at Socket.emit (events.js:*:*) at endReadableNT (_stream_readable.js:*:*) at process.internalTickCallback (internal/process/next_tick.js:*:*) @@ -57,7 +57,7 @@ ReferenceError: y is not defined at Object. ([stdin]-wrapper:*:*) at Module._compile (internal/modules/cjs/loader.js:*:*) at evalScript (internal/bootstrap/node.js:*:*) - at Socket. (internal/bootstrap/node.js:*:*) + at Socket.process.stdin.on (internal/bootstrap/node.js:*:*) at Socket.emit (events.js:*:*) at endReadableNT (_stream_readable.js:*:*) at process.internalTickCallback (internal/process/next_tick.js:*:*) diff --git a/test/message/unhandled_promise_trace_warnings.out b/test/message/unhandled_promise_trace_warnings.out index cf12c647ac67b1..2187ee1e85b8f8 100644 --- a/test/message/unhandled_promise_trace_warnings.out +++ b/test/message/unhandled_promise_trace_warnings.out @@ -15,6 +15,9 @@ at * at * at * + at * + at * + at * (node:*) Error: This was rejected at * (*test*message*unhandled_promise_trace_warnings.js:*) at * @@ -25,6 +28,7 @@ at * at * at * + at * (node:*) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code. at * at * @@ -34,6 +38,8 @@ at * at * at * + at * + at * (node:*) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1) at handledRejection (internal/process/promises.js:*) at handler (internal/process/promises.js:*) diff --git a/test/message/util_inspect_error.out b/test/message/util_inspect_error.out index 406d8112ce2599..408a3c8baf6d8f 100644 --- a/test/message/util_inspect_error.out +++ b/test/message/util_inspect_error.out @@ -10,6 +10,7 @@ at * at * at * + at * nested: { err: Error: foo @@ -22,6 +23,7 @@ at * at * at * + at * at * } } { err: Error: foo @@ -34,6 +36,7 @@ at * at * at * + at * at *, nested: { err: Error: foo @@ -47,6 +50,7 @@ at * at * at * + at * } } { Error: foo @@ -60,4 +64,5 @@ bar at * at * at * + at * foo: 'bar' }