From 436800e1454276a0255dc649c6cca981e92d15a0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 10 Feb 2019 19:28:16 +0800 Subject: [PATCH 1/2] process: setup signal handler in prepareMainThreadExecution Because this is only necessary in the main thread. Also removes the rearming of signal events since no signal event handlers should be created during the execution of node.js now that we've made the creation of stdout and stderr streams lazy - this has been demonstrated in the test coverage. --- lib/internal/bootstrap/node.js | 21 -------------------- lib/internal/bootstrap/pre_execution.js | 25 ++++++++++++++++++++++++ lib/internal/process/main_thread_only.js | 1 - 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index aa0a6b282e138a..0dc34be96e1933 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -282,36 +282,15 @@ if (process.env.NODE_V8_COVERAGE) { }; } -// Worker threads don't receive signals. -if (isMainThread) { - const { - isSignal, - startListeningIfSignal, - stopListeningIfSignal - } = mainThreadSetup.createSignalHandlers(); - process.on('newListener', startListeningIfSignal); - process.on('removeListener', stopListeningIfSignal); - // re-arm pre-existing signal event registrations - // with this signal wrap capabilities. - const signalEvents = process.eventNames().filter(isSignal); - for (const ev of signalEvents) { - process.emit('newListener', ev); - } -} - if (getOptionValue('--experimental-report')) { const { config, - handleSignal, report, syncConfig } = NativeModule.require('internal/process/report'); process.report = report; // Download the CLI / ENV config into JS land. syncConfig(config, false); - if (config.events.includes('signal')) { - process.on(config.signal, handleSignal); - } } function setupProcessObject() { diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 556c6ba39e51f6..57f7420d6c2027 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -8,6 +8,9 @@ let traceEventsAsyncHook; function prepareMainThreadExecution() { setupTraceCategoryState(); + // Only main thread receives signals. + setupSignalHandlers(); + // 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: @@ -28,6 +31,28 @@ function prepareMainThreadExecution() { loadPreloadModules(); } +function setupSignalHandlers() { + const { + createSignalHandlers + } = require('internal/process/main_thread_only'); + const { + startListeningIfSignal, + stopListeningIfSignal + } = createSignalHandlers(); + process.on('newListener', startListeningIfSignal); + process.on('removeListener', stopListeningIfSignal); + + if (getOptionValue('--experimental-report')) { + const { + config, + handleSignal + } = require('internal/process/report'); + if (config.events.includes('signal')) { + process.on(config.signal, handleSignal); + } + } +} + function setupTraceCategoryState() { const { asyncHooksEnabledInitial, diff --git a/lib/internal/process/main_thread_only.js b/lib/internal/process/main_thread_only.js index 2572402f620c27..24a7b02520fa57 100644 --- a/lib/internal/process/main_thread_only.js +++ b/lib/internal/process/main_thread_only.js @@ -144,7 +144,6 @@ function createSignalHandlers() { } return { - isSignal, startListeningIfSignal, stopListeningIfSignal }; From 9bcfc2d0e99fa4161d92c8a74cf132f553d37020 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 21 Feb 2019 11:26:43 +0800 Subject: [PATCH 2/2] process: move initialization of node-report into pre_execution.js - Splits signal handler setup code into two functions: one sets up `process.on('SIGNAL_NAME')`, another takes care of the signal triggers of node-report. Both should only happen on the main thread. The latter needs to happen after the node-report configurations are read into the process. - Move the initialization of node-report into pre_execution.js because it depends on CLI/environment settings. --- lib/internal/bootstrap/node.js | 12 ------- lib/internal/bootstrap/pre_execution.js | 42 +++++++++++++++++++------ lib/internal/main/worker_thread.js | 2 ++ 3 files changed, 35 insertions(+), 21 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 0dc34be96e1933..8b40c2258b6e69 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -38,7 +38,6 @@ const { internalBinding, NativeModule } = loaderExports; const { Object, Symbol } = primordials; -const { getOptionValue } = NativeModule.require('internal/options'); const config = internalBinding('config'); const { deprecate } = NativeModule.require('internal/util'); @@ -282,17 +281,6 @@ if (process.env.NODE_V8_COVERAGE) { }; } -if (getOptionValue('--experimental-report')) { - const { - config, - report, - syncConfig - } = NativeModule.require('internal/process/report'); - process.report = report; - // Download the CLI / ENV config into JS land. - syncConfig(config, false); -} - function setupProcessObject() { const EventEmitter = NativeModule.require('events'); const origProcProto = Object.getPrototypeOf(process); diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 57f7420d6c2027..072112e68ba07f 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -11,6 +11,10 @@ function prepareMainThreadExecution() { // Only main thread receives signals. setupSignalHandlers(); + // Process initial configurations of node-report, if any. + initializeReport(); + initializeReportSignalHandlers(); // Main-thread-only. + // 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: @@ -31,6 +35,20 @@ function prepareMainThreadExecution() { loadPreloadModules(); } +function initializeReport() { + if (!getOptionValue('--experimental-report')) { + return; + } + const { + config, + report, + syncConfig + } = require('internal/process/report'); + process.report = report; + // Download the CLI / ENV config into JS land. + syncConfig(config, false); +} + function setupSignalHandlers() { const { createSignalHandlers @@ -41,15 +59,20 @@ function setupSignalHandlers() { } = createSignalHandlers(); process.on('newListener', startListeningIfSignal); process.on('removeListener', stopListeningIfSignal); +} - if (getOptionValue('--experimental-report')) { - const { - config, - handleSignal - } = require('internal/process/report'); - if (config.events.includes('signal')) { - process.on(config.signal, handleSignal); - } +// This has to be called after both initializeReport() and +// setupSignalHandlers() are called +function initializeReportSignalHandlers() { + if (!getOptionValue('--experimental-report')) { + return; + } + const { + config, + handleSignal + } = require('internal/process/report'); + if (config.events.includes('signal')) { + process.on(config.signal, handleSignal); } } @@ -204,5 +227,6 @@ module.exports = { initializeDeprecations, initializeESMLoader, loadPreloadModules, - setupTraceCategoryState + setupTraceCategoryState, + initializeReport }; diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js index 13c1b136edaf43..b6dbc47274a7da 100644 --- a/lib/internal/main/worker_thread.js +++ b/lib/internal/main/worker_thread.js @@ -6,6 +6,7 @@ const { initializeDeprecations, initializeESMLoader, + initializeReport, loadPreloadModules, setupTraceCategoryState } = require('internal/bootstrap/pre_execution'); @@ -75,6 +76,7 @@ port.on('message', (message) => { } = message; setupTraceCategoryState(); + initializeReport(); if (manifestSrc) { require('internal/process/policy').setup(manifestSrc, manifestURL); }