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

omsmith · 2016-03-04T14:14:53Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

events

Description of change

The wrapped listeners from once() weren't handled consistently. This PR fixes up the two additional places that they are exposed publicly ('removeListener' and listeners()). There is a slowdown in doing so, but not too bad.

Unsure if a documentation update would be necessary (to say "it used to behave like so").

make bench-events:

make -C out BUILDTYPE=Release V=1 
make[1]: Entering directory '/home/omsmith/dev/node/out'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/home/omsmith/dev/node/out'
ln -fs out/Release/node node
events/ee-add-remove.js
events/ee-add-remove.js n=250000: 1603452.07582

events/ee-emit-multi-args.js
events/ee-emit-multi-args.js n=2000000: 8104495.63870

events/ee-emit.js
events/ee-emit.js n=2000000: 11585336.72431

events/ee-listener-count-on-prototype.js
events/ee-listener-count-on-prototype.js n=50000000: 627837401.26347

events/ee-listeners-many.js
events/ee-listeners-many.js n=5000000: 4315208.71881

events/ee-listeners.js
events/ee-listeners.js n=5000000: 26674496.30709

(master):

make -C out BUILDTYPE=Release V=1
make[1]: Entering directory '/home/omsmith/dev/node/out'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/home/omsmith/dev/node/out'
ln -fs out/Release/node node
events/ee-add-remove.js
events/ee-add-remove.js n=250000: 1721243.70130

events/ee-emit-multi-args.js
events/ee-emit-multi-args.js n=2000000: 8197063.10213

events/ee-emit.js
events/ee-emit.js n=2000000: 11848856.46094

events/ee-listener-count-on-prototype.js
events/ee-listener-count-on-prototype.js n=50000000: 617810355.54937

events/ee-listeners-many.js
events/ee-listeners-many.js n=5000000: 4734838.53093

events/ee-listeners.js
events/ee-listeners.js n=5000000: 28938418.31657

mscdex · 2016-03-04T14:57:28Z

I'm going to look to see if anything can be done about the perf hit.

omsmith · 2016-03-04T14:59:26Z

Not using unwrapListener within the removeListener if conditions might bump ee-add-remove back up a little bit

omsmith · 2016-03-04T18:53:46Z

@mscdex I'm thinking the changes to removeListener could be avoided, if instead we updated https://github.com/nodejs/node/pull/5564/files#diff-71dcd327d0ca067b490b22d677f81966R272 to do this.removeListener(type, listener);

That would revert any performance regression on that side of it.

Don't see how to avoid listeners() drop

omsmith · 2016-03-05T03:24:40Z

Updated as mentioned, so ee-add-remove is unaffected now.

Relevant numbers for listeners() (on different computer):

events/ee-listeners-many.js
events/ee-listeners-many.js n=5000000: 1720878.65037

events/ee-listeners.js
events/ee-listeners.js n=5000000: 12211093.94296

(master:)

events/ee-listeners-many.js
events/ee-listeners-many.js n=5000000: 1835640.69293

events/ee-listeners.js
events/ee-listeners.js n=5000000: 12632994.35337

simonkcleung · 2016-03-05T13:25:04Z

Change from this.removeListener(type, g);
to this.removeListener(type, listener);

You can add listener many times.
So you should remove the wrapper g, not listener.

mscdex · 2016-03-07T05:54:19Z

@omsmith Yeah I think that's probably going to be the best that can be done from what I can tell.

jasnell · 2016-03-07T18:43:03Z

LGTM

omsmith · 2016-03-07T18:50:05Z

As @simonkcleung points out, the current diff changes the following behaviour:

function listener() {}
new EventEmitter()
  .on('foo', listener)
  .once('foo', listener)
  .emit('foo');

With the change in place, the on listener will be removed, instead of the once listener. Now, I don't personally understand the use case of adding the same listener twice, ever. To avoid that change we need to go back to the original PR (maybe sans the helper function) and take a bit of a perf hit on ee-add-remove as well.

Don't have a strong opinion either way.

EDIT: Original first commit, for reference: omsmith@840a772

omsmith · 2016-03-11T02:50:44Z

@jasnell @mscdex I've reverted to the original change, minus the "helper function"

I've tried to run the benchmarking tools now that I know how to use them (thanks to @mscdex explaining it to someone else :)), but getting pretty inconsistent results on my machine. My best guess is that ee-listeners(-many) has about a 5% drop, and ee-add-remove is approximately unaffected.

jasnell · 2016-03-11T03:04:05Z

@nodejs/ctc ... thoughts?

indutny · 2016-03-15T20:08:51Z

lib/events.js

@@ -306,7 +306,8 @@ EventEmitter.prototype.removeListener =
        else {
          delete events[type];
          if (events.removeListener)
-            this.emit('removeListener', type, listener);
+            this.emit('removeListener', type,


I have a suggestion. Let's do let currentListener = list.listener ? list.listener : listener and check against it everywhere in this function. What do you think?

@omsmith ... ping

Jeeze, has it been 6 days? Apologies.

Had a few long days at work and needed the weekend to unwind. This is in my inbox to get to - probably tomorrow night.

:-) no worries! just going through backlog!

@indutny Thanks for having a look. That would probably do fine for the "there is a single listener right now" case, but I'm not sure it would work out for when list is actually a list, given:

passed in listener could be either the wrapped listener, or the unwrapped one (internal call vs user call)

the unwrapped listener could be used multiple times, either for once and on or multiple onces or whatever, and so we should prefer to match the wrapped fn

So we could remove repetition by doing const listenerToEmit = listener.listener ? listener.listener : listener; at the top, but you aren't necessarily (or as I understand it, likely) going to even need that (not the common case to have removeListener events).

When a user listens for an event using #once, their listener is wrapped in a function which will remove it after being called. While this was accounted for with the 'newListener' event, 'removeListener' would receive the wrapped function. Fixes nodejs#5551

omsmith · 2016-04-11T19:04:52Z

@indutny any followup to that?

simonkcleung · 2016-04-17T06:46:51Z

Line 401
ret = evlistener.map(unwrapListener); does the same thing as
ret = arrayMap(evlistener, unwrapListener); right?

jasnell · 2016-04-29T06:40:01Z

#6394 provides an alternative to this that is a bit simpler. Recommend closing this in favor of that one.

omsmith · 2016-04-29T11:49:51Z

@jasnell Hmm, apparently I pushed an older variation here by accident when
rebasing, must have been on the wrong computer. Oh well! The other one
looks good to me.

I can fix mine up to be just the fix for #listeners() (or you could suggest
including it in #6394, either way).
On 29 Apr 2016 2:41 a.m., "James M Snell" notifications@github.com wrote:

#6394 #6394 provides an alternative
to this that is a bit simpler. Recommend closing this in favor of that one.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5564 (comment)

jasnell · 2016-04-29T19:21:25Z

#6394 landed. Closing this! Thanks!

addaleax · 2016-05-18T21:41:04Z

@omsmith I think it might be a good idea if you PRed again with a fix for #listeners(), if you’re still interested… sorry that part of this PR got a bit lost.

mscdex added events Issues and PRs related to the events subsystem / EventEmitter. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 4, 2016

omsmith force-pushed the events-fix-once-unwrapping branch from 4ebeab8 to 9bbeca8 Compare March 5, 2016 03:22

omsmith force-pushed the events-fix-once-unwrapping branch from 9bbeca8 to ae7db76 Compare March 11, 2016 02:45

indutny reviewed Mar 15, 2016
View reviewed changes

omsmith added 3 commits April 11, 2016 14:30

events: refactor listener unwrapping into function

3307c30

events: unwrap #once listeners in #listeners

3465da2

omsmith force-pushed the events-fix-once-unwrapping branch from ae7db76 to 3465da2 Compare April 11, 2016 18:57

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

jasnell mentioned this pull request Apr 27, 2016

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

Closed

3 tasks

jasnell closed this Apr 29, 2016

addaleax mentioned this pull request May 18, 2016

EventEmitter.listener() returns different function then that one registered nodejs/help#171

Closed

Flarna mentioned this pull request May 19, 2016

EventEmitter: Inconsistent handling of once() handlers #6873

Closed

addaleax mentioned this pull request May 19, 2016

events: unwrap #once listeners in #listeners #6881

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

omsmith commented Mar 4, 2016

mscdex commented Mar 4, 2016

omsmith commented Mar 4, 2016

omsmith commented Mar 4, 2016

omsmith commented Mar 5, 2016

simonkcleung commented Mar 5, 2016

mscdex commented Mar 7, 2016

jasnell commented Mar 7, 2016

omsmith commented Mar 7, 2016

omsmith commented Mar 11, 2016

jasnell commented Mar 11, 2016

indutny Mar 15, 2016

jasnell Mar 21, 2016

omsmith Mar 22, 2016

jasnell Mar 22, 2016

omsmith Mar 23, 2016

omsmith commented Apr 11, 2016

simonkcleung commented Apr 17, 2016 •

edited

Loading

jasnell commented Apr 29, 2016

omsmith commented Apr 29, 2016

jasnell commented Apr 29, 2016

addaleax commented May 18, 2016

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

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

Conversation

omsmith commented Mar 4, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

mscdex commented Mar 4, 2016

omsmith commented Mar 4, 2016

omsmith commented Mar 4, 2016

omsmith commented Mar 5, 2016

simonkcleung commented Mar 5, 2016

mscdex commented Mar 7, 2016

jasnell commented Mar 7, 2016

omsmith commented Mar 7, 2016

omsmith commented Mar 11, 2016

jasnell commented Mar 11, 2016

indutny Mar 15, 2016

Choose a reason for hiding this comment

jasnell Mar 21, 2016

Choose a reason for hiding this comment

omsmith Mar 22, 2016

Choose a reason for hiding this comment

jasnell Mar 22, 2016

Choose a reason for hiding this comment

omsmith Mar 23, 2016

Choose a reason for hiding this comment

omsmith commented Apr 11, 2016

simonkcleung commented Apr 17, 2016 • edited Loading

jasnell commented Apr 29, 2016

omsmith commented Apr 29, 2016

jasnell commented Apr 29, 2016

addaleax commented May 18, 2016

simonkcleung commented Apr 17, 2016 •

edited

Loading