events: remove reaches into _events internals by apapirovski · Pull Request #17440 · nodejs/node · GitHub
Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions doc/api/events.md
18 changes: 14 additions & 4 deletions lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ module.exports = EventEmitter;
EventEmitter.EventEmitter = EventEmitter;

EventEmitter.prototype._events = undefined;
EventEmitter.prototype._eventsCount = 0;
EventEmitter.prototype._maxListeners = undefined;

// By default EventEmitters will print a warning if more than 10 listeners are
Expand Down Expand Up @@ -357,8 +358,8 @@ EventEmitter.prototype.removeAllListeners =
return this;
};

EventEmitter.prototype.listeners = function listeners(type) {
const events = this._events;
function _listeners(target, type, unwrap) {
const events = target._events;

if (events === undefined)
return [];
Expand All @@ -368,9 +369,18 @@ EventEmitter.prototype.listeners = function listeners(type) {
return [];

if (typeof evlistener === 'function')
return [evlistener.listener || evlistener];
return unwrap ? [evlistener.listener || evlistener] : [evlistener];

return unwrap ?
unwrapListeners(evlistener) : arrayClone(evlistener, evlistener.length);
}

EventEmitter.prototype.listeners = function listeners(type) {
return _listeners(this, type, true);
};

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

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.

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

@apapirovski apapirovski Dec 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@apapirovski apapirovski Dec 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

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.

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.

@apapirovski apapirovski Dec 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

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.

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.

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.

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.

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.

@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.

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.

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.

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.

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

return _listeners(this, type, false);
};

EventEmitter.listenerCount = function(emitter, type) {
Expand Down
1 change: 0 additions & 1 deletion lib/internal/bootstrap_node.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

function startup() {
const EventEmitter = NativeModule.require('events');
process._eventsCount = 0;

const origProcProto = Object.getPrototypeOf(process);
Object.setPrototypeOf(origProcProto, EventEmitter.prototype);
Expand Down
13 changes: 3 additions & 10 deletions lib/vm.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,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') > 0) {
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') > 0) {
return sigintHandlersWrap(realRunInContext, this,
[contextifiedSandbox, options]);
} else {
Expand Down Expand Up @@ -95,14 +95,7 @@ function createScript(code, options) {
// Remove all SIGINT listeners and re-attach them after the wrapped function
// has executed, so that caught SIGINT are handled by the listeners again.
function sigintHandlersWrap(fn, thisArg, argsArray) {
// Using the internal list here to make sure `.once()` wrappers are used,
// not the original ones.
let sigintListeners = process._events.SIGINT;

if (Array.isArray(sigintListeners))
sigintListeners = sigintListeners.slice();
else
sigintListeners = [sigintListeners];
const sigintListeners = process.rawListeners('SIGINT');

process.removeAllListeners('SIGINT');

Expand Down
1 change: 0 additions & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ class ModuleWrap;
V(env_pairs_string, "envPairs") \
V(errno_string, "errno") \
V(error_string, "error") \
V(events_string, "_events") \
V(exiting_string, "_exiting") \
V(exit_code_string, "exitCode") \
V(exit_string, "exit") \
Expand Down
6 changes: 0 additions & 6 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3354,12 +3354,6 @@ void SetupProcessObject(Environment* env,
env->SetMethod(process, "_setupNextTick", SetupNextTick);
env->SetMethod(process, "_setupPromises", SetupPromises);
env->SetMethod(process, "_setupDomainUse", SetupDomainUse);

// pre-set _events object for faster emit checks
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);

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.

does this cause a perf regression?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

}


Expand Down
19 changes: 19 additions & 0 deletions test/parallel/test-event-emitter-listeners.js