events: remove reaches into _events internals #17440

apapirovski · 2017-12-03T14:20:18Z

This strips out the harmless bits of #17324 into this standalone PR so they can actually land.

Refactoring of the lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra method to EventEmitter: rawListeners (this seems to be a common reason to reach into _events in user-code as well).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

events

lpinca · 2017-12-03T14:40:58Z

lib/events.js


- return unwrapListeners(evlistener);
+EventEmitter.prototype.rawListeners = function rawListeners(type) {


Is there a way to not make this public? I don't like to expose internal details of the EventEmitter.

I guess the question is whether it's beneficial to hide these internals or not? I've already seen lots of user-code reaching into _events for the wrappers. We do it ourselves even. That seems to indicate that we probably need a way to get those once wrappers.

Having this be behind a method at least gives us some control over the internals that users accessing _events[type] directly doesn't.

FWIW there's good discussion on this in #6881 where the change listeners was made. It seems like @addaleax, @ChALkeR & @Fishrock123 wanted to add an ability to return the wrapped listeners anyway but then that fell by the wayside.

I've already seen lots of user-code reaching into _events for the wrappers

Can you link some examples which is not readable-stream? We added eventNames() to prevent people from using _events and there weren't a lot of modules using _events at the time.

We do it ourselves even. That seems to indicate that we probably need a way to get those once wrappers.

Agreed but I would prefer to have it private.

I've seen it in private codebases for this particular use case, described by @ChALkeR in the PR linked above:

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?

As an example out in the wild, this module broke after the change to listeners and would need to use _events now. (Of course, as far as I can tell, prependListener just solves the same problem anyway but that's a different story.)

https://github.com/timoxley/overshadow-listeners/blob/master/index.js

Also, Ultron is subtly broken if one adds the same handler as a once using Ultron and then as a real event without Ultron. Ultron will then inadvertently remove the second version. (Although that's also partially a general issue with storing the ID directly on the function.)

https://github.com/unshiftio/ultron/blob/336c789616db9ca3f164f4e5f15ed78f49d528da/index.js#L105-L111

Yeah, I think this should be exposed for API completeness (for the reason in the @ChALkeR quote above); and that there is code in Node core that uses this should be a good indicator that there are valid use cases.

I don't see it coming, but changes to how once events are handled can potentially translate to breaking changes if this api is exposed.

@lpinca can you please articulate this more? What would change on how once events are handled?
I think we will have breaking changes anyway if people used _events, just outside of the public API.

This will probably never happen but imagine that we will switch to an object to handle events, for example:

{ once: false, listener }

rawListeners can no longer return a function.

I think we will have breaking changes anyway if people used _events, just outside of the public API.

Yes, the difference is that _events is "private" so you know beforehand that if you use it your code can break at any time.

However we can't currently touch _events because a good chunk of the ecosystem uses it.

apapirovski · 2017-12-05T20:25:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11897/

addaleax

Didn’t realize this was a separate PR when I commented

LGTM :)

TimothyGu · 2017-12-06T03:50:12Z

lib/events.js

@@ -36,6 +36,7 @@ EventEmitter.usingDomains = false;

 EventEmitter.prototype.domain = undefined;
 EventEmitter.prototype._events = undefined;
+EventEmitter.prototype._eventsCount = 0;


Why is this necessary? It seems to me that it used to be necessary for process in bootstrap_node.js since C++ pre-sets _events, which prevents the EventEmitter constructor from setting _eventsCount. But now that you've removed the C++ part of this, merely removing _eventsCount in bootstrap_node.js should suffice. I could well be mistaken though.

Can we use a private Symbol if it is necessary?

The issue is that this has been public for a while anyway. I'm just adding it onto the prototype since there are packages out there that use EventEmitter without using EventEmitter.prototype.init which means that they end up changing the hidden class when they first call emit or addListener.

The reason it wasn't there before — at least as far as I understand — is that there were issues with performance around it.

Oh ok then.

mcollina · 2017-12-06T10:49:02Z

lib/events.js

@@ -36,6 +36,7 @@ EventEmitter.usingDomains = false;

 EventEmitter.prototype.domain = undefined;
 EventEmitter.prototype._events = undefined;
+EventEmitter.prototype._eventsCount = 0;


Can we use a private Symbol if it is necessary?

mcollina · 2017-12-06T10:51:16Z

src/node.cc

- Local<Object> events_obj = Object::New(env->isolate());
- CHECK(events_obj->SetPrototype(env->context(),
- Null(env->isolate())).FromJust());
- process->Set(env->events_string(), events_obj);


does this cause a perf regression?

Not that I can tell, since we create it in the init call anyway.

mcollina · 2017-12-06T10:52:31Z

lib/vm.js

@@ -43,15 +43,15 @@ const realRunInThisContext = Script.prototype.runInThisContext;
 const realRunInContext = Script.prototype.runInContext;

 Script.prototype.runInThisContext = function(options) {
- if (options && options.breakOnSigint && process._events.SIGINT) {
+ if (options && options.breakOnSigint && process.listenerCount('SIGINT')) {


can you make listenerCount('SIGINT') > 0 explicit?

mcollina · 2017-12-06T10:52:45Z

lib/vm.js

 return sigintHandlersWrap(realRunInThisContext, this, [options]);
 } else {
 return realRunInThisContext.call(this, options);
 }
 };

 Script.prototype.runInContext = function(contextifiedSandbox, options) {
- if (options && options.breakOnSigint && process._events.SIGINT) {
+ if (options && options.breakOnSigint && process.listenerCount('SIGINT')) {


mcollina · 2017-12-06T10:55:21Z

lib/events.js


- return unwrapListeners(evlistener);
+EventEmitter.prototype.rawListeners = function rawListeners(type) {


@lpinca can you please articulate this more? What would change on how once events are handled?
I think we will have breaking changes anyway if people used _events, just outside of the public API.

mcollina · 2017-12-06T10:57:35Z

test/parallel/test-event-emitter-listeners.js

+ assert.strictEqual(wrappedListeners[0], listener);
+ assert.notStrictEqual(wrappedListeners[1], listener);
+ assert.strictEqual(wrappedListeners[1].listener, listener);
+}


Can you add some more tests?

check if it is a new copy every time

check that after firing an event the once listener is not removed from the wrappedListeners array (so it's not the internal copy)

mcollina · 2017-12-06T10:58:45Z

doc/api/events.md

+- `eventName` {any}
+
+Returns a copy of the array of listeners for the event named `eventName`,
+including any wrappers (such as those created by `.once`).


The doc does not explain what a wrapper is in this context, and how it is wrapped.

Any thoughts on how we should document this or whether we even should? I'm not certain we want to necessarily encourage the usage. If someone needs it they'll know and otherwise we might want it to be a bit obscure, haha? Not sure...

If anyone has any thoughts, please chime in.

If it's documented, it needs to be documented in full. Otherwise it doesn't. I'm fine either way.

Do we have precedent for including a publicly exposed function that's not documented? Just trying to figure out the direction we would best go here. This isn't something we want to encourage being used but it's a step better than the current situation of reaching into _events.

I'd rather this get documented fully.

addaleax · 2017-12-10T02:41:06Z

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

Are there any issues with landing this PR as it is?

apapirovski · 2017-12-10T13:15:42Z

I'm guessing that documenting wrappers might be a good idea (for the purposes of rawListeners), as per feedback above, but I don't really know a good format for it.

apapirovski · 2017-12-12T17:30:00Z

@mcollina @TimothyGu do you have any suggestions for a good way to document the once wrappers. This PR is held up on it and I don't have any good ideas re: format that doesn't make that information confusing... Also, do we document as part of rawListeners or once?

mcollina · 2017-12-12T18:25:49Z

I would just write there an example output of the rawListeners call. There is no need to document this in once.

apapirovski · 2017-12-12T18:36:23Z

@mcollina PTAL. Thank you!

mcollina · 2017-12-12T18:51:17Z

doc/api/events.md

+// Returns a new Array with a function `onceWrapper` which has a property
+// `listener` which contains the original listener bound above
+const listeners = emitter.rawListeners('log');
+const logFnWrapper = listeners[0];


can you add an example also with on()?

Updated. Let me know if this isn't what you had in mind.

mcollina

LGTM with my nit.

TimothyGu · 2017-12-13T08:03:01Z

doc/api/events.md

+<!-- YAML
+added: REPLACEME
+-->
+- `eventName` {any}


Hmm, shouldn't this be {string|symbol}

It matches the rest of the docs and it is at least partially correct. We don't validate the input so if they pass in a number, it'll get automatically converted to a string.

Either way, if we're changing this then it should be changed in all of the docs.

I've filed #17657 to track that. This again LGTM.

Refactor lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra method to EventEmitter: wrappedListeners.

Refactor lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra method to EventEmitter: rawListeners. PR-URL: #17440 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

apapirovski · 2017-12-14T13:44:58Z

Landed in decab71

Change type annotations for eventName in events API doc from {any} to {string|symbol} to be consistent with doc changes made in nodejs#17440. Fixes: nodejs#17657

Refactor lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra method to EventEmitter: rawListeners. PR-URL: #17440 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

apapirovski added events Issues and PRs related to the events subsystem / EventEmitter. semver-minor PRs that contain new features and should be released in the next minor version. labels Dec 3, 2017

apapirovski requested a review from addaleax December 3, 2017 14:20

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 3, 2017

lpinca reviewed Dec 3, 2017

View reviewed changes

lpinca mentioned this pull request Dec 4, 2017

Listeners incorrectly removed primus/ultron#23

Open

jasnell requested a review from mcollina December 5, 2017 17:08

apapirovski force-pushed the patch-ee-internals-usage branch from c71d935 to 0c953ec Compare December 5, 2017 20:23

addaleax approved these changes Dec 6, 2017

View reviewed changes

TimothyGu approved these changes Dec 6, 2017

View reviewed changes

mcollina reviewed Dec 6, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

BridgeAR approved these changes Dec 10, 2017

View reviewed changes

mcollina reviewed Dec 12, 2017

View reviewed changes

mcollina approved these changes Dec 12, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

TimothyGu reviewed Dec 13, 2017

View reviewed changes

TimothyGu mentioned this pull request Dec 13, 2017

Change type annotations for eventName in events API doc to {string|symbol} #17657

Closed

aprilwebster mentioned this pull request Dec 13, 2017

doc: change eventName type annotations #17666

Closed

2 tasks

apapirovski added 3 commits December 14, 2017 08:22

events: remove reaches into _events internals

707fca2

Refactor lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra method to EventEmitter: wrappedListeners.

fixup: address some of mcollina's feedback

a9b213c

fixup: example for rawListeners

db52004

fixup: add #on example

53aab29

apapirovski force-pushed the patch-ee-internals-usage branch from 1b0d9cd to 53aab29 Compare December 14, 2017 13:22

apapirovski closed this Dec 14, 2017

apapirovski deleted the patch-ee-internals-usage branch December 14, 2017 13:45

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added the dont-land-on-v8.x label Aug 2, 2018


		return unwrapListeners(evlistener);
		EventEmitter.prototype.rawListeners = function rawListeners(type) {

events: remove reaches into _events internals #17440

events: remove reaches into _events internals #17440

Conversation

apapirovski commented Dec 3, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

apapirovski Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Dec 5, 2017

addaleax left a comment

Choose a reason for hiding this comment

TimothyGu Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Dec 10, 2017

apapirovski commented Dec 10, 2017

apapirovski commented Dec 12, 2017

mcollina commented Dec 12, 2017

apapirovski commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Dec 14, 2017

apapirovski commented Dec 3, 2017 •

edited

Loading

apapirovski Dec 3, 2017 •

edited

Loading

apapirovski Dec 3, 2017 •

edited

Loading

apapirovski Dec 3, 2017 •

edited

Loading

TimothyGu Dec 6, 2017 •

edited

Loading