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: add ability to prepend event listeners #6032

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@jasnell
Member

jasnell commented Apr 4, 2016

Pull Request check-list

  • 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][0]?
  • 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

A handful of modules (including readable-streams) make inappropriate use of the internal _events property. One such use is to prepend an event listener to the front of the array of listeners.

To address part of the issue, this adds a new optional bitwise flag to the addListener/on/once methods that, when set, causes the listener to be prepended.

Doc update and test case is included.

Fixes: #1817

/cc @ChALkeR

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 4, 2016

I'm not sure something like this should be documented since the whole point of Readable's use of _events is to make sure its error handler gets executed before any user error event handler. If end users now have a documented way or prepending event handlers, then Readable's error event handler could easily not be first anymore.

Yes, it's still possible for people to do this even if left undocumented, but it's less likely to happen and if it does, the end user should know they could end up shooting themselves in the foot.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

Another option: Readable can implement an override for on/once/addListener that emits a process warning if the flag is used to prepend error handlers.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

Or... a similar on/once/addListener override on listener could remove it's listener and prepend it again whenever the flag is used to prepend a new error listener.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Apr 4, 2016

@mscdex Could this be solved by throwing an exception if a listener with EventEmitter.F_PREPEND flag was added a second time to the same event name when there is already one present?

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 4, 2016

@ChALkeR That would mean storing more state somehow and AFAIK the way that would have to be implemented (storing flags for existing events in a separate object) would cause noticeable performance regressions.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

I'd considered something along those lines also but I'd prefer not to have
to track that additional state. Having an emitter subclass override on to
ensure that it's special error handlers are always first seems to be the
most elegant solution. Wouldn't be that difficult.
On Apr 4, 2016 3:04 AM, "Сковорода Никита Андреевич" <
notifications@github.com> wrote:

@mscdex https://github.com/mscdex Could this be solved by throwing an
exception if a listener with EventEmitter.F_PREPEND flag was added a
second time to the same event name when there is already one?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#6032 (comment)

@vkurchatkin

This comment has been minimized.

Member

vkurchatkin commented Apr 4, 2016

-1, it makes no sense to make it public.:

  • making it public doesn't make it less of a hack, the fact that order matter means that abstraction is bad; we can make at an internal helper, though;
  • bitwise flags seem to be antithetical to javascript;

A couple of ideas:

  • error event is special, maybe reserve a special system handler that is always called first, if exists;
  • or maybe error handlers in contrast to other event handlers should be called in reverse order.
@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Apr 4, 2016

Refs: #5833 (comment) -- I think it should be an options object over a flag.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

@vkurchatkin ... I'm fine if this doesn't make it in but the motivation for making this public is to discourage module developers from hacking against internal fields like _events. While this particular hack (prepending an event handler) appears to be relatively rare, it still happens and adding the ability to prepend turns out to be quite trivial. There's only one instance where we do this internally so an internal utility seems to be overkill.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

@Fishrock123 ... I'm good with the options object.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Apr 4, 2016

Note: if this ends up internal, we can just use a true/false parameter

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Apr 4, 2016

@vkurchatkin We have an external module inside the org atm that misuses the internal API: nodejs/readable-stream.

@vkurchatkin

This comment has been minimized.

Member

vkurchatkin commented Apr 4, 2016

@ChALkeR true, not sure what to do with this. Maybe we can just add listener normally? Is there an explanation, why it's really necessary to prepend the listener?

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Apr 4, 2016

@calvinmetcalf

This comment has been minimized.

Member

calvinmetcalf commented Apr 4, 2016

so what streams is really trying to do (and other modules might need to do as well) is set an error listener that doesn't prevent the default throwing behavior from also happening (in the case of streams it's to do resource cleanup), some other way we could handle it include

  • events accepting an onerror function in the options object that gets called whenever there is an error event without counting against error events
  • an _error event (or maybe a symbol or something) that gets called for error events as well without counting towards the limit
@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

Looking at the readable streams case in more detail, the error handler that is being prepended is getting set on the destination object that is being passed to the pipe method. In other words, Readable is modifying the internal private state of an object that it does not own. The only way to do this correctly would be via a public API.

I suppose that we could introduce a new pre-error event that is called immediately before the normal set of error listeners but that seems a bit hackish to me as well.

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 4, 2016

What about just monkey-patching dest.emit() (proxying other events) until eventName === 'error' at which point onerror() is called and the old dest.emit() is put back into place? I'm not sure what kind of performance impact that might have, but it seems like that would be the least intrusive and most backwards compatible?

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

@mscdex ... that seems a bit too hackish also, and given that we've made the decision not to support modules that monkey patch node internals we likely shouldn't be relying on that kind of behavior either.

Again, just continuing the brainstorming here: Another thought came to mind: an undocumented pre-{eventName} event. For instance,

const myEE = new EventEmitter();
myEE.on('pre-error', (er) => { /* ... */ });
myEE.on('pre-foo', (a,b,c) => { /* ... */ });

Essentially, prefix any event name with pre- and that event will be triggered immediately before the actual event handlers.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Apr 4, 2016

@jasnell Seems good to me, but why keep it undocumented?

If those events would be added, at some point someone would want to document them. Why not document those from the beginning?

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

Personally I'm good with documenting it. I suggested that given the feedback here questioning whether it should be documented.

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 4, 2016

For a solution like that I'd be very careful about reserving such a prefix since I could easily see someone already implementing similarly-named events.

@calvinmetcalf

This comment has been minimized.

Member

calvinmetcalf commented Apr 4, 2016

if we were to add a before method to the event object that would also help with versioning, if it was a pre event or an options object there would be no way to know if the event emitter supported that api.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Apr 4, 2016

@mscdex I couldn't find pre-error event anywhere, not sure about the other ones.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

@calvinmetcalf makes a very good point. Having something like a before('eventName', fn) would make it a bit more explicit in terms of whether this new functionality is supported or not.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 4, 2016

If we went with before(), would it work like on where I could attach multiple functions before or should it be limited to a single function?

e.g. what should happen in the following case:

const myEE = new EventEmitter();
myEE.before('foo', () => console.log('a'));
myEE.before('foo', () => console.log('b'));
myEE.on('foo', () => console.log('c'));
myEE.emit('foo');
@mafintosh

This comment has been minimized.

Member

mafintosh commented Apr 4, 2016

Should we name it something more explicit than before to avoid breaking userland apis that extend EventEmitter/Stream?

@calvinmetcalf

This comment has been minimized.

Member

calvinmetcalf commented Apr 4, 2016

the other option would be just have an addErrorListener which works exactly like addEventListener but doesn't prevent an error event from being thrown due to no listeners.

@mafintosh

This comment has been minimized.

Member

mafintosh commented Apr 4, 2016

@calvinmetcalf i like that. error listeners are already special anyway

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 19, 2016

Done! PTAL

@jasnell jasnell force-pushed the jasnell:events-prepend branch to 6cba1e6 Apr 19, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 19, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 19, 2016

CI is green

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Apr 19, 2016

Yes, those sound better to me, thanks!

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Apr 19, 2016

LGTM

* `listener` {Function} The callback function
Adds a **one time** `listener` function for the event named `eventName` to the
beginning of the listeners array. This listener is invoked only the next time

This comment has been minimized.

@mscdex

mscdex Apr 19, 2016

Contributor

Should "beginning" be emphasized too for consistency?

@@ -12,6 +12,25 @@ var StringDecoder;
util.inherits(Readable, Stream);
const has_firston = typeof EE.prototype.prependListener === 'function';

This comment has been minimized.

@mscdex

mscdex Apr 19, 2016

Contributor

Maybe rename the variable to make it clearer?

Also, camel casing?

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Apr 19, 2016

I'd like to still take a look at this if possible. May be a day or so.

myEE.emit('foo');
// Test fail-back if prependListener is undefined

This comment has been minimized.

@mscdex

mscdex Apr 19, 2016

Contributor

s/fail-back/fallback ?

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 19, 2016

@mscdex ... updated to address nits

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 19, 2016

LGTM. Hopefully once this change has been around for a long while I will finally be able to land my .once() optimizations ;-)

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 19, 2016

Yeah I was going to ping you about that. Would definitely like to get those in soon.
@Fishrock123 ... certainly possible! I'd definitely like to get this in for v6 tho

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 19, 2016

@jasnell The problem with modules using old readable-stream module versions still exists though, so AFAIK we'd have to wait for the changes in this PR to circulate in the ecosystem for awhile before landing the .once() optimizations.

@ChALkeR ChALkeR referenced this pull request Apr 19, 2016

Closed

EventEmitter API #1817

3 of 3 tasks complete
emitter._events[event].unshift(fn);
else
emitter._events[event] = [fn, emitter._events[event]];
}

This comment has been minimized.

@mcollina

mcollina Apr 19, 2016

Member

Thanks for including the monkeypatch here.

Maybe we can even avoid that, and just move it to readable-stream.
I don't see any case these lines will be executed in core.

cc @nodejs/streams

@mcollina

This comment has been minimized.

Member

mcollina commented Apr 19, 2016

Anyway, we should land this asap and include it in a release asap, so we can start integrating the change into readable-stream.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 20, 2016

@mcollina ... yep, @Fishrock123 asked for another day or two to review. My plan is to get this landed on Thursday if there are no objections between now and then

events: add prependListener() and prependOnceListener()
A handful of modules (including readable-streams) make
inappropriate use of the internal _events property. One
such use is to prepend an event listener to the front
of the array of listeners.

This adds EE.prototype.prependListener() and
EE.prototype.prependOnceListener() methods to add handlers
to the *front* of the listener array.

Doc update and test case is included.

Fixes: #1817

@jasnell jasnell force-pushed the jasnell:events-prepend branch from 26ef578 to d5c1072 Apr 21, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 21, 2016

Commits squashed and rebased. Planning to get this landed later on today.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 22, 2016

jasnell added a commit that referenced this pull request Apr 22, 2016

events: add prependListener() and prependOnceListener()
A handful of modules (including readable-streams) make
inappropriate use of the internal _events property. One
such use is to prepend an event listener to the front
of the array of listeners.

This adds EE.prototype.prependListener() and
EE.prototype.prependOnceListener() methods to add handlers
to the *front* of the listener array.

Doc update and test case is included.

Fixes: #1817
PR-URL: #6032
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell

This comment has been minimized.

Member

jasnell commented Apr 22, 2016

Landed in 0e7d57a

@jasnell jasnell closed this Apr 22, 2016

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

events: add prependListener() and prependOnceListener()
A handful of modules (including readable-streams) make
inappropriate use of the internal _events property. One
such use is to prepend an event listener to the front
of the array of listeners.

This adds EE.prototype.prependListener() and
EE.prototype.prependOnceListener() methods to add handlers
to the *front* of the listener array.

Doc update and test case is included.

Fixes: nodejs#1817
PR-URL: nodejs#6032
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell added a commit that referenced this pull request Apr 26, 2016

events: add prependListener() and prependOnceListener()
A handful of modules (including readable-streams) make
inappropriate use of the internal _events property. One
such use is to prepend an event listener to the front
of the array of listeners.

This adds EE.prototype.prependListener() and
EE.prototype.prependOnceListener() methods to add handlers
to the *front* of the listener array.

Doc update and test case is included.

Fixes: #1817
PR-URL: #6032
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment