Skip to content

Commit e9eb2ec

Browse files
sam-githubbnoordhuis
authored andcommitted
process: fix regression in unlistening signals
When the last signal listener is removed, the signal wrap should be closed, restoring the default signal handling behaviour. This is done in a (patched) process.removeListener(). However, events.removeAllListeners has an optimization to avoid calling removeListener() if there are no listeners for the 'removeListener' event, introduced in 56668f5. That caused the following code to fail to terminate: process.stdin.resume(); function listener() {}; process.on('SIGINT', listener); process.removeAllListeners('SIGINT'); process.kill(process.pid, 'SIGINT') while the following will terminate: process.stdin.resume(); function listener() {}; process.on('SIGINT', listener); process.removeListener('SIGINT', listener); process.kill(process.pid, 'SIGINT') Replace the method patching with use of the 'newListener' and 'removeListener' events, which will fire no matter which methods are used to add or remove listeners. PR-URL: #687 Reviewed-By: Ben Noordhuis <[email protected]>
1 parent 233e333 commit e9eb2ec

File tree

2 files changed

+49
-19
lines changed

2 files changed

+49
-19
lines changed

src/node.js

+9-19
Original file line numberDiff line numberDiff line change
@@ -630,16 +630,14 @@
630630
// Load events module in order to access prototype elements on process like
631631
// process.addListener.
632632
var signalWraps = {};
633-
var addListener = process.addListener;
634-
var removeListener = process.removeListener;
635633

636634
function isSignal(event) {
637635
return event.slice(0, 3) === 'SIG' &&
638636
startup.lazyConstants().hasOwnProperty(event);
639637
}
640638

641-
// Wrap addListener for the special signal types
642-
process.on = process.addListener = function(type, listener) {
639+
// Detect presence of a listener for the special signal types
640+
process.on('newListener', function(type, listener) {
643641
if (isSignal(type) &&
644642
!signalWraps.hasOwnProperty(type)) {
645643
var Signal = process.binding('signal_wrap').Signal;
@@ -659,23 +657,15 @@
659657

660658
signalWraps[type] = wrap;
661659
}
660+
});
662661

663-
return addListener.apply(this, arguments);
664-
};
665-
666-
process.removeListener = function(type, listener) {
667-
var ret = removeListener.apply(this, arguments);
668-
if (isSignal(type)) {
669-
assert(signalWraps.hasOwnProperty(type));
670-
671-
if (NativeModule.require('events').listenerCount(this, type) === 0) {
672-
signalWraps[type].close();
673-
delete signalWraps[type];
674-
}
662+
process.on('removeListener', function(type, listener) {
663+
if (signalWraps.hasOwnProperty(type) &&
664+
NativeModule.require('events').listenerCount(this, type) === 0) {
665+
signalWraps[type].close();
666+
delete signalWraps[type];
675667
}
676-
677-
return ret;
678-
};
668+
});
679669
};
680670

681671

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
if (process.platform === 'win32') {
2+
// Win32 doesn't have signals, just a kindof emulation, insufficient
3+
// for this test to apply.
4+
return;
5+
}
6+
7+
var assert = require('assert');
8+
var spawn = require('child_process').spawn;
9+
var ok;
10+
11+
if (process.argv[2] !== '--do-test') {
12+
// We are the master, fork a child so we can verify it exits with correct
13+
// status.
14+
process.env.DOTEST = 'y';
15+
var child = spawn(process.execPath, [__filename, '--do-test']);
16+
17+
child.once('exit', function(code, signal) {
18+
assert.equal(signal, 'SIGINT');
19+
ok = true;
20+
});
21+
22+
process.on('exit', function() {
23+
assert(ok);
24+
});
25+
26+
return;
27+
}
28+
29+
process.on('SIGINT', function() {
30+
// Remove all handlers and kill ourselves. We should terminate by SIGINT
31+
// now that we have no handlers.
32+
process.removeAllListeners('SIGINT');
33+
process.kill(process.pid, 'SIGINT');
34+
});
35+
36+
// Signal handlers aren't sufficient to keep node alive, so resume stdin
37+
process.stdin.resume();
38+
39+
// Demonstrate that signals are being handled
40+
process.kill(process.pid, 'SIGINT');

0 commit comments

Comments
 (0)