Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: move signal event handling into bootstrap/node.js #25859

Closed
wants to merge 1 commit into from
Closed

lib: move signal event handling into bootstrap/node.js #25859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,6 @@ process.config = JSON.parse(internalBinding('native_module').config);
const rawMethods = internalBinding('process_methods');
// Set up methods and events on the process object for the main thread
if (isMainThread) {
// This depends on process being an event emitter
mainThreadSetup.setupSignalHandlers(internalBinding);

process.abort = rawMethods.abort;
const wrapped = mainThreadSetup.wrapProcessMethods(rawMethods);
process.umask = wrapped.umask;
Expand Down Expand Up @@ -307,6 +304,23 @@ 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);
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any specific reason for placing this block of code at this location? Why I ask is:

  • the re-arming logic was introduced to support report - though it supports any code block that registers signals while in the bootstrap process.
  • right now the signal handler setup is established just before the report is established, making the re-arming un-necessary, at least for report.
  • if we place this block of code as early as the process object's creation, we can eliminate the re-arming
  • if we place this block as late as towards the exit of setup, the re-arming will be leveraged by any one in the middle.
  • right now it seems to be a mix of both

and hence my question on the placement - is there any other considerations for this being here?

Code changes looks good to me.

Copy link
Member Author

@joyeecheung joyeecheung Feb 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact I wonder if this is ever possible to trigger...as far as I can tell no signals events should be registered at this point (inside node.js) as now the stdio streams are lazily instantiated, and the branch has been yellow (never executed) in the coverage report

if (getOptionValue('--experimental-report')) {
NativeModule.require('internal/process/report').setup();
}
Expand Down
53 changes: 27 additions & 26 deletions lib/internal/process/main_thread_only.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ const {
validateString
} = require('internal/validators');

const { signals } = internalBinding('constants').os;

// The execution of this function itself should not cause any side effects.
function wrapProcessMethods(binding) {
function chdir(directory) {
Expand Down Expand Up @@ -104,19 +106,18 @@ function wrapPosixCredentialSetters(credentials) {
};
}

// Worker threads don't receive signals.
function setupSignalHandlers(internalBinding) {
const constants = internalBinding('constants').os.signals;
const signalWraps = Object.create(null);
let Signal;
let Signal;
function isSignal(event) {
return typeof event === 'string' && signals[event] !== undefined;
}

function isSignal(event) {
return typeof event === 'string' && constants[event] !== undefined;
}
// Worker threads don't receive signals.
function createSignalHandlers() {
const signalWraps = new Map();

// Detect presence of a listener for the special signal types
process.on('newListener', function(type) {
if (isSignal(type) && signalWraps[type] === undefined) {
function startListeningIfSignal(type) {
if (isSignal(type) && !signalWraps.has(type)) {
if (Signal === undefined)
Signal = internalBinding('signal_wrap').Signal;
const wrap = new Signal();
Expand All @@ -125,30 +126,30 @@ function setupSignalHandlers(internalBinding) {

wrap.onsignal = process.emit.bind(process, type, type);

const signum = constants[type];
const signum = signals[type];
const err = wrap.start(signum);
if (err) {
wrap.close();
throw errnoException(err, 'uv_signal_start');
}

signalWraps[type] = wrap;
signalWraps.set(type, wrap);
}
});
}

process.on('removeListener', function(type) {
if (signalWraps[type] !== undefined && this.listenerCount(type) === 0) {
signalWraps[type].close();
delete signalWraps[type];
function stopListeningIfSignal(type) {
const wrap = signalWraps.get(type);
if (wrap !== undefined && process.listenerCount(type) === 0) {
wrap.close();
signalWraps.delete(type);
}
});

// re-arm pre-existing signal event registrations
// with this signal wrap capabilities.
process.eventNames().forEach((ev) => {
if (isSignal(ev))
process.emit('newListener', ev);
});
}

return {
isSignal,
startListeningIfSignal,
stopListeningIfSignal
};
}

function setupChildProcessIpcChannel() {
Expand All @@ -166,7 +167,7 @@ function setupChildProcessIpcChannel() {

module.exports = {
wrapProcessMethods,
setupSignalHandlers,
createSignalHandlers,
setupChildProcessIpcChannel,
wrapPosixCredentialSetters
};