From 8d6105b3c812adbfd3cf8e05309bffef5dc956c9 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Feb 2019 04:03:25 +0800 Subject: [PATCH 1/3] process: group main thread execution preparation code This patch groups the preparation of main thread execution into `prepareMainThreadExecution()` and removes the cluster IPC setup in worker thread bootstrap since clusters do not use worker threads for its implementation and it's unlikely to change. --- lib/internal/bootstrap/node.js | 25 -------------- lib/internal/bootstrap/pre_execution.js | 42 +++++++++++++++++++++--- lib/internal/main/check_syntax.js | 12 ++----- lib/internal/main/eval_stdin.js | 12 ++----- lib/internal/main/eval_string.js | 12 ++----- lib/internal/main/repl.js | 12 ++----- lib/internal/main/run_main_module.js | 12 ++----- lib/internal/main/worker_thread.js | 22 +++++++++++-- lib/internal/process/main_thread_only.js | 14 -------- 9 files changed, 67 insertions(+), 96 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index b4f2d4e838cd5d..2585fff206d025 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -176,31 +176,6 @@ if (config.hasInspector) { internalBinding('inspector').registerAsyncHook(enable, disable); } -// If the process is spawned with env NODE_CHANNEL_FD, it's probably -// spawned by our child_process module, then initialize IPC. -// This attaches some internal event listeners and creates: -// process.send(), process.channel, process.connected, -// process.disconnect() -if (process.env.NODE_CHANNEL_FD) { - if (ownsProcessState) { - mainThreadSetup.setupChildProcessIpcChannel(); - } else { - Object.defineProperty(process, 'channel', { - enumerable: false, - get: workerThreadSetup.unavailable('process.channel') - }); - - Object.defineProperty(process, 'connected', { - enumerable: false, - get: workerThreadSetup.unavailable('process.connected') - }); - - process.send = workerThreadSetup.unavailable('process.send()'); - process.disconnect = - workerThreadSetup.unavailable('process.disconnect()'); - } -} - const browserGlobals = !process._noBrowserGlobals; if (browserGlobals) { setupGlobalTimeouts(); diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index d09fdb131a2ca9..ce4af115badaab 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -2,6 +2,27 @@ const { getOptionValue } = require('internal/options'); +function prepareMainThreadExecution() { + // If the process is spawned with env NODE_CHANNEL_FD, it's probably + // spawned by our child_process module, then initialize IPC. + // This attaches some internal event listeners and creates: + // process.send(), process.channel, process.connected, + // process.disconnect(). + setupChildProcessIpcChannel(); + + // Load policy from disk and parse it. + initializePolicy(); + + // 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). + initializeClusterIPC(); + + initializeDeprecations(); + initializeESMLoader(); + loadPreloadModules(); +} + // In general deprecations are intialized wherever the APIs are implemented, // this is used to deprecate APIs implemented in C++ where the deprecation // utitlities are not easily accessible. @@ -41,10 +62,22 @@ function initializeDeprecations() { } } +function setupChildProcessIpcChannel() { + if (process.env.NODE_CHANNEL_FD) { + const assert = require('internal/assert'); + + const fd = parseInt(process.env.NODE_CHANNEL_FD, 10); + assert(fd >= 0); + + // Make sure it's not accidentally inherited by child processes. + delete process.env.NODE_CHANNEL_FD; + + require('child_process')._forkChild(fd); + assert(process.send); + } +} + function initializeClusterIPC() { - // 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 = require('cluster'); cluster._setupWorker(); @@ -114,9 +147,8 @@ function loadPreloadModules() { } module.exports = { + prepareMainThreadExecution, initializeDeprecations, - initializeClusterIPC, - initializePolicy, initializeESMLoader, loadPreloadModules }; diff --git a/lib/internal/main/check_syntax.js b/lib/internal/main/check_syntax.js index 97584841b3a0c2..5d98701132f0e8 100644 --- a/lib/internal/main/check_syntax.js +++ b/lib/internal/main/check_syntax.js @@ -4,11 +4,7 @@ // instead of actually running the file. const { - initializeDeprecations, - initializeClusterIPC, - initializePolicy, - initializeESMLoader, - loadPreloadModules + prepareMainThreadExecution } = require('internal/bootstrap/pre_execution'); const { @@ -22,11 +18,7 @@ const { } = require('internal/modules/cjs/helpers'); // TODO(joyeecheung): not every one of these are necessary -initializeDeprecations(); -initializeClusterIPC(); -initializePolicy(); -initializeESMLoader(); -loadPreloadModules(); +prepareMainThreadExecution(); markBootstrapComplete(); if (process.argv[1] && process.argv[1] !== '-') { diff --git a/lib/internal/main/eval_stdin.js b/lib/internal/main/eval_stdin.js index f02d9ffa0cea03..2a2ef6d38a1fe8 100644 --- a/lib/internal/main/eval_stdin.js +++ b/lib/internal/main/eval_stdin.js @@ -3,11 +3,7 @@ // Stdin is not a TTY, we will read it and execute it. const { - initializeDeprecations, - initializeClusterIPC, - initializePolicy, - initializeESMLoader, - loadPreloadModules + prepareMainThreadExecution } = require('internal/bootstrap/pre_execution'); const { @@ -15,11 +11,7 @@ const { readStdin } = require('internal/process/execution'); -initializeDeprecations(); -initializeClusterIPC(); -initializePolicy(); -initializeESMLoader(); -loadPreloadModules(); +prepareMainThreadExecution(); markBootstrapComplete(); readStdin((code) => { diff --git a/lib/internal/main/eval_string.js b/lib/internal/main/eval_string.js index 7f746a6b11a951..953fab386d0671 100644 --- a/lib/internal/main/eval_string.js +++ b/lib/internal/main/eval_string.js @@ -4,21 +4,13 @@ // `--interactive`. const { - initializeDeprecations, - initializeClusterIPC, - initializePolicy, - initializeESMLoader, - loadPreloadModules + prepareMainThreadExecution } = require('internal/bootstrap/pre_execution'); const { evalScript } = require('internal/process/execution'); const { addBuiltinLibsToObject } = require('internal/modules/cjs/helpers'); const source = require('internal/options').getOptionValue('--eval'); -initializeDeprecations(); -initializeClusterIPC(); -initializePolicy(); -initializeESMLoader(); -loadPreloadModules(); +prepareMainThreadExecution(); addBuiltinLibsToObject(global); markBootstrapComplete(); evalScript('[eval]', source, process._breakFirstLine); diff --git a/lib/internal/main/repl.js b/lib/internal/main/repl.js index e931444ef32502..e6b98853511d11 100644 --- a/lib/internal/main/repl.js +++ b/lib/internal/main/repl.js @@ -4,22 +4,14 @@ // the main module is not specified and stdin is a TTY. const { - initializeDeprecations, - initializeClusterIPC, - initializePolicy, - initializeESMLoader, - loadPreloadModules + prepareMainThreadExecution } = require('internal/bootstrap/pre_execution'); const { evalScript } = require('internal/process/execution'); -initializeDeprecations(); -initializeClusterIPC(); -initializePolicy(); -initializeESMLoader(); -loadPreloadModules(); +prepareMainThreadExecution(); const cliRepl = require('internal/repl'); cliRepl.createInternalRepl(process.env, (err, repl) => { diff --git a/lib/internal/main/run_main_module.js b/lib/internal/main/run_main_module.js index abc41fc20220f0..97a4e33be2e629 100644 --- a/lib/internal/main/run_main_module.js +++ b/lib/internal/main/run_main_module.js @@ -1,18 +1,10 @@ 'use strict'; const { - initializeDeprecations, - initializeClusterIPC, - initializePolicy, - initializeESMLoader, - loadPreloadModules + prepareMainThreadExecution } = require('internal/bootstrap/pre_execution'); -initializeDeprecations(); -initializeClusterIPC(); -initializePolicy(); -initializeESMLoader(); -loadPreloadModules(); +prepareMainThreadExecution(); // Expand process.argv[1] into a full path. const path = require('path'); diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js index 7e4466c24d0c31..ac161b7588f04c 100644 --- a/lib/internal/main/worker_thread.js +++ b/lib/internal/main/worker_thread.js @@ -5,7 +5,6 @@ const { initializeDeprecations, - initializeClusterIPC, initializeESMLoader, loadPreloadModules } = require('internal/bootstrap/pre_execution'); @@ -42,6 +41,26 @@ debug(`[${threadId}] is setting up worker child environment`); // Set up the message port and start listening const port = getEnvMessagePort(); +// If the main thread is spawned with env NODE_CHANNEL_FD, it's probably +// spawned by our child_process module. In the work threads, mark the +// related IPC properties as unavailable. +if (process.env.NODE_CHANNEL_FD) { + const workerThreadSetup = require('internal/process/worker_thread_only'); + Object.defineProperty(process, 'channel', { + enumerable: false, + get: workerThreadSetup.unavailable('process.channel') + }); + + Object.defineProperty(process, 'connected', { + enumerable: false, + get: workerThreadSetup.unavailable('process.connected') + }); + + process.send = workerThreadSetup.unavailable('process.send()'); + process.disconnect = + workerThreadSetup.unavailable('process.disconnect()'); +} + port.on('message', (message) => { if (message.type === LOAD_SCRIPT) { const { @@ -57,7 +76,6 @@ port.on('message', (message) => { require('internal/process/policy').setup(manifestSrc, manifestURL); } initializeDeprecations(); - initializeClusterIPC(); initializeESMLoader(); loadPreloadModules(); publicWorker.parentPort = publicPort; diff --git a/lib/internal/process/main_thread_only.js b/lib/internal/process/main_thread_only.js index 0ce063f928974a..96c57fda35c755 100644 --- a/lib/internal/process/main_thread_only.js +++ b/lib/internal/process/main_thread_only.js @@ -152,22 +152,8 @@ function createSignalHandlers() { }; } -function setupChildProcessIpcChannel() { - const assert = require('internal/assert'); - - const fd = parseInt(process.env.NODE_CHANNEL_FD, 10); - assert(fd >= 0); - - // Make sure it's not accidentally inherited by child processes. - delete process.env.NODE_CHANNEL_FD; - - require('child_process')._forkChild(fd); - assert(process.send); -} - module.exports = { wrapProcessMethods, createSignalHandlers, - setupChildProcessIpcChannel, wrapPosixCredentialSetters }; From 5ea16b047fa940976856aa182f91280a2fea6e17 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 8 Feb 2019 11:22:37 +0800 Subject: [PATCH 2/3] process: normalize process.argv before user code execution And make sure that `process.argv` from the preloaded modules is the same as the one in the main module. Refs: https://github.com/nodejs/node/issues/25967 --- lib/internal/main/check_syntax.js | 11 ++++-- lib/internal/main/run_main_module.js | 4 +-- .../test-preload-print-process-argv.js | 34 +++++++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-preload-print-process-argv.js diff --git a/lib/internal/main/check_syntax.js b/lib/internal/main/check_syntax.js index 5d98701132f0e8..392fadb99ff668 100644 --- a/lib/internal/main/check_syntax.js +++ b/lib/internal/main/check_syntax.js @@ -17,9 +17,6 @@ const { stripShebang, stripBOM } = require('internal/modules/cjs/helpers'); -// TODO(joyeecheung): not every one of these are necessary -prepareMainThreadExecution(); -markBootstrapComplete(); if (process.argv[1] && process.argv[1] !== '-') { // Expand process.argv[1] into a full path. @@ -31,8 +28,16 @@ if (process.argv[1] && process.argv[1] !== '-') { const fs = require('fs'); const source = fs.readFileSync(filename, 'utf-8'); + // TODO(joyeecheung): not every one of these are necessary + prepareMainThreadExecution(); + markBootstrapComplete(); + checkScriptSyntax(source, filename); } else { + // TODO(joyeecheung): not every one of these are necessary + prepareMainThreadExecution(); + markBootstrapComplete(); + readStdin((code) => { checkScriptSyntax(code, '[stdin]'); }); diff --git a/lib/internal/main/run_main_module.js b/lib/internal/main/run_main_module.js index 97a4e33be2e629..634abe493ea60e 100644 --- a/lib/internal/main/run_main_module.js +++ b/lib/internal/main/run_main_module.js @@ -4,12 +4,12 @@ const { prepareMainThreadExecution } = require('internal/bootstrap/pre_execution'); -prepareMainThreadExecution(); - // Expand process.argv[1] into a full path. const path = require('path'); process.argv[1] = path.resolve(process.argv[1]); +prepareMainThreadExecution(); + const CJSModule = require('internal/modules/cjs/loader'); markBootstrapComplete(); diff --git a/test/parallel/test-preload-print-process-argv.js b/test/parallel/test-preload-print-process-argv.js new file mode 100644 index 00000000000000..7bd81ab6e53517 --- /dev/null +++ b/test/parallel/test-preload-print-process-argv.js @@ -0,0 +1,34 @@ +'use strict'; + +// This tests that process.argv is the same in the preloaded module +// and the user module. + +require('../common'); + +const tmpdir = require('../common/tmpdir'); +const assert = require('assert'); +const { spawnSync } = require('child_process'); +const fs = require('fs'); + +tmpdir.refresh(); + +process.chdir(tmpdir.path); +fs.writeFileSync( + 'preload.js', + 'console.log(JSON.stringify(process.argv));', + 'utf-8'); + +fs.writeFileSync( + 'main.js', + 'console.log(JSON.stringify(process.argv));', + 'utf-8'); + +const child = spawnSync(process.execPath, ['-r', './preload.js', 'main.js']); + +if (child.status !== 0) { + console.log(child.stderr.toString()); + assert.strictEqual(child.status, 0); +} + +const lines = child.stdout.toString().trim().split('\n'); +assert.deepStrictEqual(JSON.parse(lines[0]), JSON.parse(lines[1])); From 5453bf0197fd6fc4609a1e811a6a84906bd34698 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 10 Feb 2019 03:56:02 +0800 Subject: [PATCH 3/3] fixup! process: normalize process.argv before user code execution --- test/parallel/test-preload-print-process-argv.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-preload-print-process-argv.js b/test/parallel/test-preload-print-process-argv.js index 7bd81ab6e53517..ace20dfb3947c4 100644 --- a/test/parallel/test-preload-print-process-argv.js +++ b/test/parallel/test-preload-print-process-argv.js @@ -3,13 +3,17 @@ // This tests that process.argv is the same in the preloaded module // and the user module. -require('../common'); +const common = require('../common'); const tmpdir = require('../common/tmpdir'); const assert = require('assert'); const { spawnSync } = require('child_process'); const fs = require('fs'); +if (!common.isMainThread) { + common.skip('Cannot chdir to the tmp directory in workers'); +} + tmpdir.refresh(); process.chdir(tmpdir.path);