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 56668f54d1. 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: https://github.com/iojs/io.js/pull/687
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

src/node.js

@@ -630,16 +630,14 @@
     // Load events module in order to access prototype elements on process like
     // process.addListener.
     var signalWraps = {};
-    var addListener = process.addListener;
-    var removeListener = process.removeListener;
 
     function isSignal(event) {
       return event.slice(0, 3) === 'SIG' &&
              startup.lazyConstants().hasOwnProperty(event);
     }
 
-    // Wrap addListener for the special signal types
-    process.on = process.addListener = function(type, listener) {
+    // Detect presence of a listener for the special signal types
+    process.on('newListener', function(type, listener) {
       if (isSignal(type) &&
           !signalWraps.hasOwnProperty(type)) {
         var Signal = process.binding('signal_wrap').Signal;
@@ -659,23 +657,15 @@
 
         signalWraps[type] = wrap;
       }
+    });
 
-      return addListener.apply(this, arguments);
-    };
-
-    process.removeListener = function(type, listener) {
-      var ret = removeListener.apply(this, arguments);
-      if (isSignal(type)) {
-        assert(signalWraps.hasOwnProperty(type));
-
-        if (NativeModule.require('events').listenerCount(this, type) === 0) {
-          signalWraps[type].close();
-          delete signalWraps[type];
-        }
+    process.on('removeListener', function(type, listener) {
+      if (signalWraps.hasOwnProperty(type) &&
+          NativeModule.require('events').listenerCount(this, type) === 0) {
+        signalWraps[type].close();
+        delete signalWraps[type];
       }
-
-      return ret;
-    };
+    });
   };
 
 

test/parallel/test-process-remove-all-signal-listeners.js

@@ -0,0 +1,40 @@
+if (process.platform === 'win32') {
+  // Win32 doesn't have signals, just a kindof emulation, insufficient
+  // for this test to apply.
+  return;
+}
+
+var assert = require('assert');
+var spawn = require('child_process').spawn;
+var ok;
+
+if (process.argv[2] !== '--do-test') {
+  // We are the master, fork a child so we can verify it exits with correct
+  // status.
+  process.env.DOTEST = 'y';
+  var child = spawn(process.execPath, [__filename, '--do-test']);
+
+  child.once('exit', function(code, signal) {
+    assert.equal(signal, 'SIGINT');
+    ok = true;
+  });
+
+  process.on('exit', function() {
+    assert(ok);
+  });
+
+  return;
+}
+
+process.on('SIGINT', function() {
+  // Remove all handlers and kill ourselves. We should terminate by SIGINT
+  // now that we have no handlers.
+  process.removeAllListeners('SIGINT');
+  process.kill(process.pid, 'SIGINT');
+});
+
+// Signal handlers aren't sufficient to keep node alive, so resume stdin
+process.stdin.resume();
+
+// Demonstrate that signals are being handled
+process.kill(process.pid, 'SIGINT');