Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: remove reaches into _events internals #17440

Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,39 @@ to indicate an unlimited number of listeners.

Returns a reference to the `EventEmitter`, so that calls can be chained.

### emitter.rawListeners(eventName)
<!-- YAML
added: REPLACEME
-->
- `eventName` {any}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


Returns a copy of the array of listeners for the event named `eventName`,
including any wrappers (such as those created by `.once`).
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather this get documented fully.


```js
const emitter = new EventEmitter();
emitter.once('log', () => console.log('log once'));

// 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];
Copy link
Member

Choose a reason for hiding this comment

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

can you add an example also with on()?

Copy link
Member Author

Choose a reason for hiding this comment

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

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


// logs "log once" to the console and does not unbind the `once` event
logFnWrapper.listener();

// logs "log once" to the console and removes the listener
logFnWrapper();

emitter.on('log', () => console.log('log persistently'));
// will return a new Array with a single function bound by `on` above
const newListeners = emitter.rawListeners('log');

// logs "log persistently" twice
newListeners[0]();
emitter.emit('log');
```

[`--trace-warnings`]: cli.html#cli_trace_warnings
[`EventEmitter.defaultMaxListeners`]: #events_eventemitter_defaultmaxlisteners
[`domain`]: domain.html
Expand Down
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
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.

Copy link
Member Author

@apapirovski apapirovski Dec 3, 2017

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.

Copy link
Member Author

@apapirovski apapirovski Dec 3, 2017

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

Copy link
Member Author

@apapirovski apapirovski Dec 3, 2017

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
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
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
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
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
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
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
Member 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
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,22 @@ function listener2() {}
const s = new TestStream();
assert.deepStrictEqual(s.listeners('foo'), []);
}

{
const ee = new events.EventEmitter();
ee.on('foo', listener);
const wrappedListener = ee.rawListeners('foo');
assert.strictEqual(wrappedListener.length, 1);
assert.strictEqual(wrappedListener[0], listener);
assert.notStrictEqual(wrappedListener, ee.rawListeners('foo'));
ee.once('foo', listener);
const wrappedListeners = ee.rawListeners('foo');
assert.strictEqual(wrappedListeners.length, 2);
assert.strictEqual(wrappedListeners[0], listener);
assert.notStrictEqual(wrappedListeners[1], listener);
assert.strictEqual(wrappedListeners[1].listener, listener);
assert.notStrictEqual(wrappedListeners, ee.rawListeners('foo'));
ee.emit('foo');
assert.strictEqual(wrappedListeners.length, 2);
assert.strictEqual(wrappedListeners[1].listener, listener);
}