Description

pandaGao

opened on Jul 3, 2018

Issue body actions

removeListener() in events module loop backwards to find listener from array. Consider the following code:

const EventEmitter = require('events')

function pong () {
  console.log('pong')
}

let a = new EventEmitter()

a.once('ping', pong)
a.on('ping', pong)
a.removeListener('ping', pong)
a.emit('ping')
a.emit('ping')

// output:
// pong

let b = new EventEmitter()

b.on('ping', pong)
b.once('ping', pong)
b.removeListener('ping', pong)
b.emit('ping')
b.emit('ping')

// output:
// pong
// pong

The output shows that for multiple same listeners, the last added one will be the first to remove (LIFO). I think maybe a "FIFO" logic is better for this case. And I find the commit on 5 Mar 2013 which implements "loop backwards" logic.
3e64b56#diff-71dcd327d0ca067b490b22d677f81966
The commit message shows this logic optimize removeAllListeners().But it did effect this specific case. Maybe removeAllListeners() should keep "LIFO" logic and the backward loop optimizing, and removeListener() could use a forward loop.

Reacted by Balint Soos

Activity

1. TimothyGu commented on Jul 3, 2018

   

   TimothyGu

   on Jul 3, 2018

   Member

   More actions

   I think maybe a "FIFO" logic is better for this case.

   Can you explain why you think so?
2. TimothyGu

   added

   eventsIssues and PRs related to EventEmitter and the events module.Issues and PRs related to EventEmitter and the events module.

   on Jul 3, 2018
3. pandaGao commented on Jul 4, 2018

   

   pandaGao

   on Jul 4, 2018

   Author

   More actions

   To remove a listener we need to "find" it. Like array.find() I consider this "find action" is from start to end at first. At least document doesn't indicate current "LIFO" logic in removeListener(), it could be confused when someone meet the same case like my example. Of course it only make a difference if you add multiple same listeners with on() and once() at the same time. Perhaps we could update the document to point it out.
4. TimothyGu commented on Jul 4, 2018

   

   TimothyGu

   on Jul 4, 2018

   Member

   More actions

   At this point, I'd say the easier thing to do is to update the documentation rather than making a semver-major change.

   Reacted by Ryan Gao
5. TimothyGu

   added

   docIssues and PRs related to Node.js documentation.Issues and PRs related to Node.js documentation.

   on Jul 4, 2018
6. jasnell

   closed this as completedin b049d55on Oct 23, 2018
7. targos

   added a commit that references this issue on Oct 24, 2018

   doc: add note about removeListener order

   84fdb1c
8. MylesBorins

   added 2 commits that reference this issue on Nov 26, 2018

   doc: add note about removeListener order

   c251d40

   doc: add note about removeListener order

   9982c09
9. rvagg

   added 2 commits that reference this issue on Nov 28, 2018

   doc: add note about removeListener order

   e5f75cf

   doc: add note about removeListener order

   8990e32
10. MylesBorins

    added a commit that references this issue on Nov 29, 2018

    doc: add note about removeListener order

    98fc848
11. sjarmak

    added a commit that references this issue on Jul 27, 2026

    doc: add note about removeListener order

    b049d55

Issue actions