From e53bd676a35a5d15fc24fd5948b3fe08c6f621dd Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Sun, 1 Feb 2015 19:57:31 -0800 Subject: [PATCH 1/2] events: remove indeterminancy from event ordering The order of the `newListener` and `removeListener` events with respect to the actual adding and removing from the underlying listeners array should be deterministic. There is no compelling reason for leaving it indeterminate. Changing the ordering is likely to result in breaking code that was unwittingly relying on the current behaviour, and the indeterminancy makes it impossible to use these events to determine when the first or last listener is added for an event. --- doc/api/events.markdown | 12 +++++--- .../test-event-emitter-add-listeners.js | 22 ++++++++++++++- ...test-event-emitter-remove-all-listeners.js | 12 ++++++++ .../test-event-emitter-remove-listeners.js | 28 ++++++++++++++++++- 4 files changed, 68 insertions(+), 6 deletions(-) diff --git a/doc/api/events.markdown b/doc/api/events.markdown index 3492619da77e95..ffee7d04f393bd 100644 --- a/doc/api/events.markdown +++ b/doc/api/events.markdown @@ -143,8 +143,11 @@ Return the number of listeners for a given event. * `event` {String} The event name * `listener` {Function} The event handler function -This event is emitted any time a listener is added. When this event is triggered, -the listener may not yet have been added to the array of listeners for the `event`. +This event is emitted *before* a listener is added. When this event is +triggered, the listener has not been added to the array of listeners for the +`event`. Any listeners added to the event `name` in the newListener event +callback will be added *before* the listener that is in the process of being +added. ### Event: 'removeListener' @@ -152,5 +155,6 @@ the listener may not yet have been added to the array of listeners for the `even * `event` {String} The event name * `listener` {Function} The event handler function -This event is emitted any time someone removes a listener. When this event is triggered, -the listener may not yet have been removed from the array of listeners for the `event`. +This event is emitted *after* a listener is removed. When this event is +triggered, the listener has been removed from the array of listeners for the +`event`. diff --git a/test/parallel/test-event-emitter-add-listeners.js b/test/parallel/test-event-emitter-add-listeners.js index 0a1c148b8f29f8..7cc302e0b81695 100644 --- a/test/parallel/test-event-emitter-add-listeners.js +++ b/test/parallel/test-event-emitter-add-listeners.js @@ -12,6 +12,8 @@ var times_hello_emited = 0; assert.equal(e.addListener, e.on); e.on('newListener', function(event, listener) { + if (event === 'newListener') + return; // Don't track our adding of newListener listeners. console.log('newListener: ' + event); events_new_listener_emited.push(event); listeners_new_listener_emited.push(listener); @@ -23,6 +25,11 @@ function hello(a, b) { assert.equal('a', a); assert.equal('b', b); } +e.once('newListener', function(name, listener) { + assert.equal(name, 'hello'); + assert.equal(listener, hello); + assert.deepEqual(this.listeners('hello'), []); +}); e.on('hello', hello); var foo = function() {}; @@ -44,4 +51,17 @@ process.on('exit', function() { assert.equal(1, times_hello_emited); }); - +var listen1 = function listen1() {}; +var listen2 = function listen2() {}; +var e1 = new events.EventEmitter; +e1.once('newListener', function() { + assert.deepEqual(e1.listeners('hello'), []); + e1.once('newListener', function() { + assert.deepEqual(e1.listeners('hello'), []); + }); + e1.on('hello', listen2); +}); +e1.on('hello', listen1); +// The order of listeners on an event is not always the order in which the +// listeners were added. +assert.deepEqual(e1.listeners('hello'), [listen2, listen1]); diff --git a/test/parallel/test-event-emitter-remove-all-listeners.js b/test/parallel/test-event-emitter-remove-all-listeners.js index 1c359ce5c262b4..8c6d68ec5fa2ee 100644 --- a/test/parallel/test-event-emitter-remove-all-listeners.js +++ b/test/parallel/test-event-emitter-remove-all-listeners.js @@ -57,3 +57,15 @@ e3.on('removeListener', listener); // there exists a removeListener listener, but there exists // no listeners for the provided event type assert.doesNotThrow(e3.removeAllListeners.bind(e3, 'foo')); + +var e4 = new events.EventEmitter(); +var expectLength = 2; +e4.on('removeListener', function(name, listener) { + assert.equal(expectLength--, this.listeners('baz').length); +}); +e4.on('baz', function(){}); +e4.on('baz', function(){}); +e4.on('baz', function(){}); +assert.equal(e4.listeners('baz').length, expectLength+1); +e4.removeAllListeners('baz'); +assert.equal(e4.listeners('baz').length, 0); diff --git a/test/parallel/test-event-emitter-remove-listeners.js b/test/parallel/test-event-emitter-remove-listeners.js index fd699662cae052..5d8acafc063d09 100644 --- a/test/parallel/test-event-emitter-remove-listeners.js +++ b/test/parallel/test-event-emitter-remove-listeners.js @@ -45,12 +45,20 @@ assert.deepEqual([listener1], e2.listeners('hello')); var e3 = new events.EventEmitter(); e3.on('hello', listener1); e3.on('hello', listener2); -e3.on('removeListener', common.mustCall(function(name, cb) { +e3.once('removeListener', common.mustCall(function(name, cb) { assert.equal(name, 'hello'); assert.equal(cb, listener1); + assert.deepEqual([listener2], e3.listeners('hello')); })); e3.removeListener('hello', listener1); assert.deepEqual([listener2], e3.listeners('hello')); +e3.once('removeListener', common.mustCall(function(name, cb) { + assert.equal(name, 'hello'); + assert.equal(cb, listener2); + assert.deepEqual([], e3.listeners('hello')); +})); +e3.removeListener('hello', listener2); +assert.deepEqual([], e3.listeners('hello')); var e4 = new events.EventEmitter(); e4.on('removeListener', common.mustCall(function(name, cb) { @@ -61,3 +69,21 @@ e4.on('removeListener', common.mustCall(function(name, cb) { e4.on('quux', remove1); e4.on('quux', remove2); e4.removeListener('quux', remove1); + +var e5 = new events.EventEmitter(); +e5.on('hello', listener1); +e5.on('hello', listener2); +e5.once('removeListener', common.mustCall(function(name, cb) { + assert.equal(name, 'hello'); + assert.equal(cb, listener1); + assert.deepEqual([listener2], e5.listeners('hello')); + e5.once('removeListener', common.mustCall(function(name, cb) { + assert.equal(name, 'hello'); + assert.equal(cb, listener2); + assert.deepEqual([], e5.listeners('hello')); + })); + e5.removeListener('hello', listener2); + assert.deepEqual([], e5.listeners('hello')); +})); +e5.removeListener('hello', listener1); +assert.deepEqual([], e5.listeners('hello')); From 09653fb7e793c54bc8a75958dc86809c52023923 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Mon, 29 Dec 2014 18:00:02 -0800 Subject: [PATCH 2/2] 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. --- src/node.js | 28 +++++-------- ...est-process-remove-all-signal-listeners.js | 40 +++++++++++++++++++ 2 files changed, 49 insertions(+), 19 deletions(-) create mode 100644 test/parallel/test-process-remove-all-signal-listeners.js diff --git a/src/node.js b/src/node.js index 3179378fbc5ae6..f0bee17ef26758 100644 --- a/src/node.js +++ b/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; - }; + }); }; diff --git a/test/parallel/test-process-remove-all-signal-listeners.js b/test/parallel/test-process-remove-all-signal-listeners.js new file mode 100644 index 00000000000000..e5dd5a13a16fca --- /dev/null +++ b/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');