events: add ability to prepend event listeners

[jasnell]

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]

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]

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]

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]

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

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

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, "Сковорода Никита Андреевич" <
[email protected]> wrote:

@mscdexhttps://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]

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

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

[jasnell]

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

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

[Fishrock123]

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

[ChALkeR]

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

[vkurchatkin]

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

/cc @nodejs/streams

[calvinmetcalf]

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]

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]

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]

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

constmyEE=newEventEmitter();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]

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

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

[mscdex]

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]

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]

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

[jasnell]

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

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:

constmyEE=newEventEmitter();myEE.before('foo',()=>console.log('a'));myEE.before('foo',()=>console.log('b'));myEE.on('foo',()=>console.log('c'));myEE.emit('foo');

[mafintosh]

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

[calvinmetcalf]

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]

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

[jasnell]

Done! PTAL

[jasnell]

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

[jasnell]

CI is green

[ChALkeR]

Yes, those sound better to me, thanks!

[ChALkeR]

LGTM

[mscdex]

Should "beginning" be emphasized too for consistency?

[Fishrock123]

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

[mscdex]

s/fail-back/fallback ?

[jasnell]

@mscdex ... updated to address nits

[mscdex]

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

[jasnell]

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]

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

[mcollina]

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]

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

[jasnell]

@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

[jasnell]

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

[jasnell]

One final CI run: https://ci.nodejs.org/job/node-test-pull-request/2367/

[jasnell]

Landed in 0e7d57a