Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: Loop forwards to find listener in removeListener function #21635

Closed
pandaGao opened this issue Jul 3, 2018 · 3 comments
Closed

events: Loop forwards to find listener in removeListener function #21635

pandaGao opened this issue Jul 3, 2018 · 3 comments
Labels
doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter.

Comments

@pandaGao
Copy link

pandaGao commented Jul 3, 2018

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.

@TimothyGu
Copy link
Member

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

Can you explain why you think so?

@TimothyGu TimothyGu added the events Issues and PRs related to the events subsystem / EventEmitter. label Jul 3, 2018
@pandaGao
Copy link
Author

pandaGao commented Jul 4, 2018

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.

@TimothyGu
Copy link
Member

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

@TimothyGu TimothyGu added the doc Issues and PRs related to the documentations. label Jul 4, 2018
jasnell added a commit to jasnell/node that referenced this issue Oct 23, 2018
targos pushed a commit that referenced this issue Oct 24, 2018
Fixes: #21635

PR-URL: #23762
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 26, 2018
Fixes: #21635

PR-URL: #23762
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 26, 2018
Fixes: #21635

PR-URL: #23762
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
rvagg pushed a commit that referenced this issue Nov 28, 2018
Fixes: #21635

PR-URL: #23762
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
rvagg pushed a commit that referenced this issue Nov 28, 2018
Fixes: #21635

PR-URL: #23762
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
Fixes: #21635

PR-URL: #23762
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

No branches or pull requests

2 participants