events: pass the original listener added by EventEmitter#once to the removeListener handler by DavidCai1111 · Pull Request #6394 · nodejs/node · GitHub

Checklist

tests and code linting passes
the commit message follows commit guidelines
a test and/or benchmark is included

Description of change

We use the _onceWrap function to wrap the listener added by EventEmitter#once, and use the flag fired inside to make sure the listener will only be called once, including the calling in the removeListener event:

'use strict'
const EventEmitter = require('events')

let ee = new EventEmitter()

ee.once('test', () => console.log('test'))

ee.on('removeListener', (eventName, listener) => {
  console.log(`eventName: ${eventName}`)
  listener.call(ee)
  listener.call(ee)
  listener.call(ee)
})

ee.emit('test')
// eventName: test
// test

But by explicitly invoking the listener in the removeListener handler the user expresses a pretty strong intent that the listener should be called. By now we can use a listener.listener trick to archive this:

'use strict'
const EventEmitter = require('events')

let ee = new EventEmitter()

ee.once('test', () => console.log('test'))

ee.on('removeListener', (eventName, listener) => {
  console.log(`eventName: ${eventName}`)
  listener.listener.call(ee)
  listener.listener.call(ee)
  listener.listener.call(ee)
})

ee.emit('test')
// eventName: test
// test
// test
// test
// test

But those who have not read the the code of lib/events.js should not know this trick at all, and using tricks is alway not a good way. so this PR is to pass the original listener added by EventEmitter#once to the removeListener handler:

'use strict'
const EventEmitter = require('events')

let ee = new EventEmitter()

ee.once('test', () => console.log('test'))

ee.on('removeListener', (eventName, listener) => {
  console.log(`eventName: ${eventName}`)
  listener()
  listener()
  listener()
})

ee.emit('test')
// eventName: test
// test
// test
// test
// test

Why would we go through the trouble of trying to prevent that? Using the delete operator like that isn't exactly free, either.

I’d say by explicitly invoking the listener in the removeListener handler the user expresses a pretty strong intent that the listener should be called.

@addaleax But as the output showed in the first code example, no matter invoking the listener in the removeListener handler how many times, it will only be called once eventually, unless we do the listener.listener trick.

I have to admit, I find the PR title here a bit confusing then.

I think a much cleaner solution (and more aligned with user expectations?) would be to pass the original listener which has been passed to .once() to the removeListener handler, though. That has the additional benifit of no extra work in the case that there is no removeListener handler.

@addaleax It's a better solution, i will change the code.

@addaleax Hey bro, I've changed the code and the content of this PR. Now the user will get the original listener which added by EventEmitter#once in removeListener handler 👍

I like the change, but would like to head other’s opinions on this. I think this could be considered a bugfix, because this is how I’d understand the documentation (if I didn’t know how .once() was implemented under the hood) and it aligns removeListener with newListener.

Hey bro

Appreciate you not wanting to be formal, but I’m definitely not a bro, for more than one reason. :)

I think so, too : = )

This makes sense, but could you add tests.

@cjihrig 👌

@cjihrig Already added : = )

I had no idea the original function wasn't passed, I'd expect the listener passed to once to be the listener passed in the removeListener event!

@sam-github It will be, if this PR merged.

Both assertions can be assert.strictEqual().

LGTM pending comments and CI.

Are we classifying this as semver major or patch? I'd lean toward patch, but this could potentially break things.

Yeah, I think this breaks only code that is already broken. So, leaning towards patch, too.

I lean towards patch, too.

Related (covers the same ground): #5564

Seems that the solution code in this PR is more lightweight and harmless ?

Look into #5564
Same problem appeared in EventEmitter.prototype.listeners

line 338: originalListener = list[i].listener;
line 360: this.emit('removeListener', type, originalListener || listener);

change to

originalListener = list[i].listener||listener;
this.emit('removeListener', type, originalListener);

seems more readable.

might be clearer to just inline listener6 with the removeListener line below instead of creating a separate decl for it

LGTM with a nit. Pending CI of course.

Looks good, landing

When removing a `once` listener, the listener being passed to the `removeListener` callback is the wrapper. This unwraps the listener so that `removeListener` is passed the actual listener. PR-URL: #6394 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 706778a

Shouldn't this be tagged as semver-major?

Yes. Done.

Wait, I take that back. We talked about this the other day. Most people (myself included) were leaning toward patch. I can see how it would be a major though.

That was brought up a few comments up: #6394 (comment)

@mscdex ... it could be see discussion here #6394 (comment) ... the existing behavior can be rightfully considered to be a bug and this is just a fix. I'm -1 on it being semver-major.

@mscdex ... to be on the safe side we can hold off on pulling this back to v5 or v4 for a while in case there are any regressions caused.

When removing a `once` listener, the listener being passed to the `removeListener` callback is the wrapper. This unwraps the listener so that `removeListener` is passed the actual listener. PR-URL: #6394 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

When removing a `once` listener, the listener being passed to the `removeListener` callback is the wrapper. This unwraps the listener so that `removeListener` is passed the actual listener. PR-URL: nodejs#6394 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

do you think it is about time ot backport @nodejs/lts?

@nodejs/lts @nodejs/ctc thoughts on this being backported?

I think its reasonable to backport

When removing a `once` listener, the listener being passed to the `removeListener` callback is the wrapper. This unwraps the listener so that `removeListener` is passed the actual listener. PR-URL: #6394 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added the events Issues and PRs related to the events subsystem / EventEmitter. label Apr 26, 2016

DavidCai1111 changed the title events: make sure the listener added by EventEmitter#once will only be called once events: pass the original listener added by EventEmitter#once to the removeListener handler Apr 26, 2016

DavidCai1111 force-pushed the optimize_oncewrap branch from cffb713 to b12c2ac Compare April 26, 2016 12:37

DavidCai1111 force-pushed the optimize_oncewrap branch from b12c2ac to fa40a5a Compare April 26, 2016 15:25

cjihrig reviewed Apr 26, 2016
View reviewed changes

DavidCai1111 force-pushed the optimize_oncewrap branch from fa40a5a to d26733d Compare April 26, 2016 16:25

jasnell reviewed Apr 28, 2016
View reviewed changes

jasnell mentioned this pull request Apr 29, 2016

events: fix unwrapping of ee.once() listeners #5564

Closed

4 tasks

jasnell closed this Apr 29, 2016

jasnell mentioned this pull request Apr 29, 2016

events: removeListener event should be passed the user's listener function when using once() #5551

Closed

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 29, 2016

cjihrig removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 29, 2016

This was referenced May 4, 2016

Propose v6.1.0 wrong base please ignore #6556

Closed

Propose v6.1.0 (Openssl update) #6557

Merged

Flarna mentioned this pull request May 19, 2016

EventEmitter: Inconsistent handling of once() handlers #6873

Closed

cjihrig mentioned this pull request May 19, 2016

events: unwrap #once listeners in #listeners #6881

Closed

4 tasks

MylesBorins added the lts-watch-v4.x label Jun 1, 2016

cjihrig mentioned this pull request Jun 2, 2016

test: refactor large event emitter tests #6446

Merged

3 tasks

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Oct 24, 2016

MylesBorins mentioned this pull request Nov 22, 2016

v4.7.0 proposal #9736

Merged

events: pass the original listener added by EventEmitter#once to the removeListener handler by DavidCai1111 · Pull Request #6394 · nodejs/node · GitHub

Uh oh!

Uh oh!

events: pass the original listener added by EventEmitter#once to the removeListener handler - #6394

events: pass the original listener added by EventEmitter#once to the removeListener handler#6394DavidCai1111 wants to merge 1 commit intonodejs:masternodejs/node:masterfrom DavidCai1111:optimize_oncewrapDavidCai1111/node:optimize_oncewrapCopy head branch name to clipboard

Conversation

DavidCai1111 commented Apr 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description of change

Uh oh!

bnoordhuis commented Apr 26, 2016

Uh oh!

addaleax commented Apr 26, 2016

Uh oh!

DavidCai1111 commented Apr 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Apr 26, 2016

Uh oh!

DavidCai1111 commented Apr 26, 2016

Uh oh!

DavidCai1111 commented Apr 26, 2016

Uh oh!

addaleax commented Apr 26, 2016

Uh oh!

DavidCai1111 commented Apr 26, 2016

Uh oh!

cjihrig commented Apr 26, 2016

Uh oh!

DavidCai1111 commented Apr 26, 2016

Uh oh!

DavidCai1111 commented Apr 26, 2016

Uh oh!

sam-github commented Apr 26, 2016

Uh oh!

DavidCai1111 commented Apr 26, 2016

Uh oh!

cjihrig Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Apr 26, 2016

Uh oh!

addaleax commented Apr 26, 2016

Uh oh!

sam-github commented Apr 26, 2016

Uh oh!

jasnell commented Apr 27, 2016

Uh oh!

DavidCai1111 commented Apr 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonkcleung commented Apr 28, 2016

Uh oh!

jasnell Apr 28, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 28, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

mscdex commented Apr 29, 2016

Uh oh!

cjihrig commented Apr 29, 2016

Uh oh!

cjihrig commented Apr 29, 2016

Uh oh!

addaleax commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

MylesBorins commented Aug 30, 2016

Uh oh!

MylesBorins commented Oct 10, 2016

Uh oh!

sam-github commented Oct 12, 2016

events: pass the original listener added by EventEmitter#once to the removeListener handler#6394
DavidCai1111 wants to merge 1 commit into
nodejs:masterfrom
DavidCai1111:optimize_oncewrap

DavidCai1111 commented Apr 26, 2016 •
edited
Loading

DavidCai1111 commented Apr 26, 2016 •
edited
Loading

DavidCai1111 commented Apr 27, 2016 •
edited
Loading