From 7b91ac043151c8c6c69262e6cb376c40a801a26e Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Fri, 12 Feb 2016 15:18:21 +0530 Subject: [PATCH 1/5] doc: update removeListener behaviour This commit updates events doc to describe removeListener behaviour when it is called within a listener. An example is added to make it more evident. A test is also incuded to make this behaviour consistent in future releases. fixes nodejs/node#4759 --- doc/api/events.markdown | 36 +++++++++++++++++++ .../test-event-emitter-remove-listeners.js | 26 ++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/doc/api/events.markdown b/doc/api/events.markdown index 0afd583ebc35d7..6edc223654c2fc 100644 --- a/doc/api/events.markdown +++ b/doc/api/events.markdown @@ -377,6 +377,42 @@ listener array. If any single listener has been added multiple times to the listener array for the specified `event`, then `removeListener` must be called multiple times to remove each instance. +Note that once an event has been emitted, all listeners attached to it at the +time of emitting will be called in order. This implies that any `removeListener` +call *after* emitting and *before* the last listener finishes execution will +not affect the current listener array. Subsequent events will behave as expected. + +```js +const myEmitter = new MyEmitter(); + +var callbackA = () => { + console.log('A'); + myEmitter.removeListener('event', callbackB); +}; + +var callbackB = () => { + console.log('B'); +}; + +myEmitter.on('event', callbackA); + +myEmitter.on('event', callbackB); + +// callbackA removes listener callbackB but it will still be called. +// Interal listener array at time of emit [callbackA, callbackB] +myEmitter.emit('event'); + // Prints: + // B + // A + +// callbackB is now removed. +// Interal listener array [callbackA] +myEmitter.emit('event'); + // Prints: + // A + +``` + Because listeners are managed using an internal array, calling this will change the position indices of any listener registered *after* the listener being removed. This will not impact the order in which listeners are called, diff --git a/test/parallel/test-event-emitter-remove-listeners.js b/test/parallel/test-event-emitter-remove-listeners.js index 396979a923d1ac..efd733e43edd5b 100644 --- a/test/parallel/test-event-emitter-remove-listeners.js +++ b/test/parallel/test-event-emitter-remove-listeners.js @@ -83,3 +83,29 @@ e5.once('removeListener', common.mustCall(function(name, cb) { })); e5.removeListener('hello', listener1); assert.deepEqual([], e5.listeners('hello')); + +var e6 = new events.EventEmitter(); +count = 0; + +function listener3() { + count++; + e6.removeListener('hello', listener4); +} + +function listener4() { + count++; +} + +e6.on('hello', listener3); +e6.on('hello', listener4); + +//listener4 will still be called although it is removed by listener 3. +e6.emit('hello'); +//This is so because the interal listener array at time of emit +//was [listener3,listener4] +assert.equal(count, 2); + +count = 0; +//Interal listener array [listener3] +e6.emit('hello'); +assert.equal(count, 1); From e7dffb01dc85ddda6ea9014317a2e501d4731689 Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Fri, 12 Feb 2016 20:55:05 +0530 Subject: [PATCH 2/5] doc: fix comment typo in events Add spaces before beginning of comments. --- test/parallel/test-event-emitter-remove-listeners.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-event-emitter-remove-listeners.js b/test/parallel/test-event-emitter-remove-listeners.js index efd733e43edd5b..0622803c14bf97 100644 --- a/test/parallel/test-event-emitter-remove-listeners.js +++ b/test/parallel/test-event-emitter-remove-listeners.js @@ -99,13 +99,13 @@ function listener4() { e6.on('hello', listener3); e6.on('hello', listener4); -//listener4 will still be called although it is removed by listener 3. +// listener4 will still be called although it is removed by listener 3. e6.emit('hello'); -//This is so because the interal listener array at time of emit -//was [listener3,listener4] +// This is so because the interal listener array at time of emit +// was [listener3,listener4] assert.equal(count, 2); count = 0; -//Interal listener array [listener3] +// Interal listener array [listener3] e6.emit('hello'); assert.equal(count, 1); From a21be7c56feb7568d5e443c2179f2462358ab0b9 Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Fri, 12 Feb 2016 21:56:06 +0530 Subject: [PATCH 3/5] test: use mustCall instead of count variable Improve code by replacing count variable to assert number of function calls with mustCall method from common module. Also use ES6 arrow function decalaration for anonymous functions. --- .../test-event-emitter-remove-listeners.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/test/parallel/test-event-emitter-remove-listeners.js b/test/parallel/test-event-emitter-remove-listeners.js index 0622803c14bf97..52220ca32c5667 100644 --- a/test/parallel/test-event-emitter-remove-listeners.js +++ b/test/parallel/test-event-emitter-remove-listeners.js @@ -84,17 +84,16 @@ e5.once('removeListener', common.mustCall(function(name, cb) { e5.removeListener('hello', listener1); assert.deepEqual([], e5.listeners('hello')); -var e6 = new events.EventEmitter(); -count = 0; +const e6 = new events.EventEmitter(); -function listener3() { - count++; +var listener3 = common.mustCall(() => { + console.log('listener3'); e6.removeListener('hello', listener4); -} +}, 2); -function listener4() { - count++; -} +var listener4 = common.mustCall(() => { + console.log('listener4'); +}, 1); e6.on('hello', listener3); e6.on('hello', listener4); @@ -103,9 +102,6 @@ e6.on('hello', listener4); e6.emit('hello'); // This is so because the interal listener array at time of emit // was [listener3,listener4] -assert.equal(count, 2); -count = 0; // Interal listener array [listener3] e6.emit('hello'); -assert.equal(count, 1); From b60d25f202b4787094bc74df70cc9cb33fd93aac Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Fri, 12 Feb 2016 22:26:35 +0530 Subject: [PATCH 4/5] doc: fix typo in events Correct print order for removeListener test. --- doc/api/events.markdown | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/events.markdown b/doc/api/events.markdown index 6edc223654c2fc..5cdd90e84d231b 100644 --- a/doc/api/events.markdown +++ b/doc/api/events.markdown @@ -380,7 +380,7 @@ multiple times to remove each instance. Note that once an event has been emitted, all listeners attached to it at the time of emitting will be called in order. This implies that any `removeListener` call *after* emitting and *before* the last listener finishes execution will -not affect the current listener array. Subsequent events will behave as expected. +not affect that `emit()`. Subsequent events will behave as expected. ```js const myEmitter = new MyEmitter(); @@ -402,8 +402,8 @@ myEmitter.on('event', callbackB); // Interal listener array at time of emit [callbackA, callbackB] myEmitter.emit('event'); // Prints: - // B // A + // B // callbackB is now removed. // Interal listener array [callbackA] From 9ae5a70467dc5fc35efbd6a02e6757e296806dec Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Fri, 12 Feb 2016 23:11:43 +0530 Subject: [PATCH 5/5] doc: Improve wording for removeListener text Previous commit was misleading by saying that internal array is not modified by removeListener or removeAllListeners call. Now worded appropriately. --- doc/api/events.markdown | 7 ++++--- test/parallel/test-event-emitter-remove-listeners.js | 7 ++----- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/doc/api/events.markdown b/doc/api/events.markdown index 5cdd90e84d231b..797b969ecc2824 100644 --- a/doc/api/events.markdown +++ b/doc/api/events.markdown @@ -378,9 +378,10 @@ listener array for the specified `event`, then `removeListener` must be called multiple times to remove each instance. Note that once an event has been emitted, all listeners attached to it at the -time of emitting will be called in order. This implies that any `removeListener` -call *after* emitting and *before* the last listener finishes execution will -not affect that `emit()`. Subsequent events will behave as expected. +time of emitting will be called in order. This implies that any `removeListener()` +or `removeAllListeners()` calls *after* emitting and *before* the last listener +finishes execution will not remove them from `emit()` in progress. Subsequent +events will behave as expected. ```js const myEmitter = new MyEmitter(); diff --git a/test/parallel/test-event-emitter-remove-listeners.js b/test/parallel/test-event-emitter-remove-listeners.js index 52220ca32c5667..cc6f9516b5e3f3 100644 --- a/test/parallel/test-event-emitter-remove-listeners.js +++ b/test/parallel/test-event-emitter-remove-listeners.js @@ -86,14 +86,11 @@ assert.deepEqual([], e5.listeners('hello')); const e6 = new events.EventEmitter(); -var listener3 = common.mustCall(() => { - console.log('listener3'); +const listener3 = common.mustCall(() => { e6.removeListener('hello', listener4); }, 2); -var listener4 = common.mustCall(() => { - console.log('listener4'); -}, 1); +const listener4 = common.mustCall(() => {}, 1); e6.on('hello', listener3); e6.on('hello', listener4);