events: unwrap #once listeners in #listeners by omsmith · Pull Request #6881 · nodejs/node

omsmith · 2016-05-19T15:56:47Z

Checklist

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

Affected core subsystem(s)

events

Description of change

Fixes: #6873

cjihrig · 2016-05-19T16:10:59Z

Could you just do evlistener.listener || evlistener?

Sure thing, pushing that up. This was just mirroring the check as written for 'newListener'.

cjihrig · 2016-05-19T16:12:32Z

LGTM with a couple comments.

addaleax · 2016-05-19T17:19:06Z

nit: const?

addaleax · 2016-05-19T17:21:48Z

LGTM with a nit… #5564 was labelled as semver-major. What do you think?

cjihrig · 2016-05-19T17:29:14Z

#6394 got through as a patch, so it could probably go either way. It just depends if we want to look at this as a bug or not.

jasnell · 2016-05-19T17:39:20Z

I call bug.

jasnell · 2016-05-19T17:39:58Z

LGTM if CI is green and @addaleax' nit is addressed.

Fixes: nodejs#6873

omsmith · 2016-05-19T17:49:46Z

Updated with const

addaleax · 2016-05-22T14:17:52Z

CI: https://ci.nodejs.org/job/node-test-commit/3436/

Going to land this if it’s green.

Fishrock123 · 2016-05-22T14:26:45Z

Should we run citgm against this? I don't understand whether or not there is risk here.

Edit: here's a run regardless: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/277/

jasnell · 2016-05-22T16:09:39Z

Couple of citgm failures on ubuntu ppc -> https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/277/nodes=ppcle-ubuntu1404/tapTestReport/

addaleax · 2016-05-22T16:32:02Z

Nothing that looks related… @Fishrock123 is it okay for you to consider this a semver-patch one?

Fishrock123 · 2016-05-22T17:58:58Z

I don't have a firm opinion. If you are confident about it, go ahead. :)

addaleax · 2016-05-22T19:02:32Z

The more I think about this the more I start to get cautious… e.g. I didn’t realize this so far, but I’d be creating a bug myself in #6635 if this one landed, too, because I was assuming that re-adding the elements from listeners() using .addListener() would restore the previous situation.

So, eh, thanks for making me think twice…

addaleax · 2016-05-22T19:37:42Z

i.e. there’s a valid use case for the old behaviour, namely temporarily disabling listeners by storing the result of listeners(), removing all of them and re-attaching them later.

ChALkeR · 2016-05-23T09:41:51Z

This was discussed before in #914, will break several modules, and is clearly a semver-major.
Also related: #5127.

See https://github.com/timoxley/overshadow-listeners/blob/master/index.js, for example.

addaleax · 2016-05-23T10:29:55Z

Could we add an option to .listeners() that defaults to the current behaviour of returning unwrapped listeners to un-semver-major this? Changing the that default could still be discussed as a separate semver-major change…

jasnell · 2016-05-23T13:30:36Z

I view this one as a bug fix and would rather the default behavior be to
return the unwrapped listeners. Mark it semver-major if necessary but the
current behavior is broken.
On May 23, 2016 3:30 AM, "Anna Henningsen" notifications@github.com wrote:

Could we add an option to .listeners() that defaults to the current
behaviour of returning unwrapped listeners to un-semver-major this?
Changing the that default could still be discussed as a separate
semver-major change…

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

ChALkeR · 2016-05-23T13:48:22Z

@jasnell It looks like there will be no way to re-attach the events then, which could be useful for cloning, intercepting, or temporary disabling events. Perhaps some API should be added that allows that?

Edit: obviously missed «no» here, updated.

addaleax · 2016-05-27T23:35:37Z

Concrete proposal for implementation as an optional feature: addaleax/node@cd0a542310be. What do you think?

Fishrock123 · 2016-05-28T02:29:49Z

Could someone clarify for myself and anyone else who may drop in here what we mean by "unwrapping"? Thanks!

addaleax · 2016-05-28T02:41:37Z

@Fishrock123 oh, sorry, wasn’t aware that this isn’t clear here (and that also means that we probably need a better name). So, I hope the following sums up everything so far:

Basically, .once() uses .on() internally, but wraps the user-supplied function using a closure that, when invoked, first removes itself from the event listener list and then calls the original user-supplied function. This closure has a .listener property that refers to that original function.

Unwrapping refers to returning the .listener property of an event handler, if present, when e.g. emitter.listeners() is called; it makes sense to do that from one perspective, because those are the values that the user actually supplied to .on()/.once(), and they can’t really use the wrapped listeners themselves.

For example, in

const emitter = new EventEmitter();
function listener() {}
emitter.once('foo', listener);
emitter.listeners('foo')[0] === listener // this should be true? no?
emitter.listeners('foo')[0].listener === listener // this is currently true, but not documented in any way

it would probably reflect user expectations to return the original listener function in emitter.listeners('foo')[0].

Fishrock123 · 2016-05-28T14:49:45Z

Basically, .once() uses .on() internally, but wraps the user-supplied function using a closure that, when invoked, first removes itself from the event listener list and then calls the original user-supplied function.

Ah right, I totally knew that but didn't link the two... >_<

jasnell · 2016-08-05T22:56:15Z

@nodejs/ctc ... looks like this one fell by the way side accidentally. Please take a moment to review (again if you already reviewed it once).

jasnell · 2016-08-05T22:56:37Z

LGTM if CI is green.
CI: https://ci.nodejs.org/job/node-test-pull-request/3547/

mscdex · 2016-08-05T23:03:55Z

citgm again: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/345/

omsmith · 2016-08-06T02:38:18Z

-      ret = [evlistener];
+      ret = [evlistener.listener || evlistener];
    else
-      ret = arrayClone(evlistener, evlistener.length);


No, the other usages of arrayClone expect the wrapped listeners.

I would expect declaring unwrapListeners within listeners (I believe that's what you're suggesting?) to result in a performance degradation.

jasnell · 2016-08-09T21:12:14Z

CI is green and CITGM is only showing known flaky failures. Will get this landed.
If there's anything additional that needs to be done here, we can do a follow on PR

Fixes: #6873 PR-URL: #6881 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2016-08-09T21:15:00Z

Landed in b7a8a69! (finally ;-) ...)

Matches the Node 8 API. v1 matched Node 4, and some new methods have since been introduced (notably `prepend[Once]Listener`). The major bump in `events` was because: - There is a breaking change in the `listeners()` method regarding `once()` listeners nodejs/node#6881 I think it's super unlikely to affect anyone, but there you go. - The gzipped size almost doubled from 1.1KB to 2.1KB. It'll shrink a bit because some patches have since landed in Node that remove some code, but it'll still be bigger than it was because of the new methods. Deets: https://github.com/Gozala/events/releases/tag/v2.0.0

nodejs-github-bot added the events Issues and PRs related to EventEmitter and the events module. label May 19, 2016

cjihrig reviewed May 19, 2016
View reviewed changes

omsmith force-pushed the events-listeners-fix-once-unwrapping branch from 49951dc to 3b2551a Compare May 19, 2016 16:48

addaleax reviewed May 19, 2016
View reviewed changes

Comment thread lib/events.js Outdated

addaleax May 19, 2016 •

edited

Loading

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const?

events: unwrap #once listeners in #listeners

d667704

Fixes: nodejs#6873

omsmith force-pushed the events-listeners-fix-once-unwrapping branch from 3b2551a to d667704 Compare May 19, 2016 17:49

ChALkeR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 23, 2016

addaleax mentioned this pull request Jun 6, 2016

events: allow unwraping #once handler in listeners #7172

Closed

4 tasks done

addaleax mentioned this pull request Jun 9, 2016

vm,repl: (add ability to) break on sigint/ctrl+c #6635

Merged

4 tasks done

yorkie reviewed Aug 6, 2016
View reviewed changes

jasnell pushed a commit that referenced this pull request Aug 9, 2016

events: unwrap #once listeners in #listeners

b7a8a69

Fixes: #6873 PR-URL: #6881 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell closed this Aug 9, 2016

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

2 of 3 tasks

loppear mentioned this pull request Nov 27, 2017

Fix issue with node event listeners change nxus/core#117

Merged

apapirovski mentioned this pull request Dec 3, 2017

events: remove reaches into _events internals #17440

Closed

4 tasks done

goto-bus-stop mentioned this pull request Feb 15, 2018

Update events to v3 webpack/node-libs-browser#74

Merged

Conversation

omsmith commented May 19, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

cjihrig May 19, 2016

Choose a reason for hiding this comment

Uh oh!

omsmith May 19, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 19, 2016

Uh oh!

addaleax May 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 19, 2016

Uh oh!

cjihrig commented May 19, 2016

Uh oh!

jasnell commented May 19, 2016

Uh oh!

jasnell commented May 19, 2016

Uh oh!

omsmith commented May 19, 2016

Uh oh!

addaleax commented May 22, 2016

Uh oh!

Fishrock123 commented May 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 22, 2016

Uh oh!

addaleax commented May 22, 2016

Uh oh!

Fishrock123 commented May 22, 2016

Uh oh!

addaleax commented May 22, 2016

Uh oh!

addaleax commented May 22, 2016

Uh oh!

ChALkeR commented May 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 23, 2016

Uh oh!

jasnell commented May 23, 2016

Uh oh!

ChALkeR commented May 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 27, 2016

Uh oh!

Fishrock123 commented May 28, 2016

Uh oh!

addaleax commented May 28, 2016

Uh oh!

Fishrock123 commented May 28, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

jasnell commented Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Aug 5, 2016

Uh oh!

omsmith Aug 6, 2016

Choose a reason for hiding this comment

Uh oh!

omsmith Aug 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 9, 2016

Uh oh!

jasnell commented Aug 9, 2016

Uh oh!

Reviewers

Assignees

addaleax May 19, 2016 •

edited

Loading

Fishrock123 commented May 22, 2016 •

edited

Loading

ChALkeR commented May 23, 2016 •

edited

Loading

ChALkeR commented May 23, 2016 •

edited

Loading

jasnell commented Aug 5, 2016 •

edited

Loading