emit after 'error'

I believe the events allowed to be emitted after error should be rather limited.

Based on the test suite I've found the following exceptions:

exit, disconnect and close, should be ok.
unpipe, maybe ok?

I've fixed some:

Then there are a lot of possible cases that might need fixing:

https://gist.github.com/ronag/b5728ae5db305abaff9955da5b47a5c9

I've found these by updating EventEmitter.prototype.emit with:

  if (this._emittedError) {
    if (!['close', 'exit', 'disconnect'].includes(type)) {
      throw new Error("unexpected event: " + type);
    }
  } else if (type === 'error') {
    this._emittedError = true;
  }

Is this worth to further look into?

@mcollina

ping @mcollina

emit() is a leaky abstraction: there is nothing stopping anybody to call it and emit an event. The individual cases should be fixed if they do not cause too many regressions.

@ronag

@ronag I was wondering if it would be okay to allow 'error' events as well? Like the one here. Throwing before all subsequent errors are thrown would prevent those error logs from being shown.

@ronag

@ronag I was wondering if it would be okay to allow 'error' events as well? Like the one here.

In at least streams we don't want/allow multiple error events.

I see. So are we talking about different behaviour for different modules (subsequent errors not allowed in streams but allowed in crypto (or similar ones))?

I see. So are we talking about different behaviour for different modules (subsequent errors not allowed in streams but allowed in crypto (or similar ones))?

Yea, I guess so. I would rather it would be consistent everywhere but achieving that is a high goal. I think we should start focusing on making it sensible, e.g.

No more events after 'close'
No more "logic" events after 'error'

etc...

Here is an updated list based on master in case you want to have a go at it https://gist.github.com/ronag/3c4fdf8f73e2671efa8b9456d7bc4746

I think all of the most important issues with post 'error' events have been resolved or has PR's now. Except for:
'close', 'exit', 'disconnect', 'removeListener', 'unpipe', 'error' which I think is ok.

Next step (if any) would be to ensure 'error' is only emitted once and 'close' is always last.

There is work left to do on events after 'close' https://gist.github.com/ronag/c733ec3df54350964f2ead7dec6590e0

stream: don't emit 'finish' after 'error'

http: don't emit 'finish' after 'error'

stream: don't emit 'finish' after 'error'

http: don't emit 'finish' after 'error'

stream: don't emit 'finish' after 'error'

http: don't emit 'readable' after 'close'

http: don't emit 'finish' after 'error'

http: don't emit 'readable' after 'close'

Navigation Menu

Repository navigation

emit after 'error' #28710

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

emit after 'error' #28710

Description

Activity

rexagod commented on Mar 13, 2020

mcollina commented on Mar 14, 2020

rexagod commented on Mar 14, 2020

ronag commented on Mar 14, 2020

rexagod commented on Mar 14, 2020

ronag commented on Mar 14, 2020

ronag commented on Mar 14, 2020

ronag commented on Mar 14, 2020

ronag commented on Mar 14, 2020

ronag commented on Mar 15, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions