emit after 'error'

[ronag]

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:

#28709
#28708
#28711

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?

[rexagod]

ping @mcollina

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

[rexagod]

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

[rexagod]

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))?

[ronag]

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

[ronag]

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

[ronag]

Here are some that I find notable:

=== release test-tls-set-secure-context ===
Path: parallel/test-tls-set-secure-context
events.js:268
      throw new Error("unexpected event: " + type);
      ^

Error: unexpected event: finish
    at ClientRequest.emit (events.js:268:13)
    at onFinish (_http_outgoing.js:748:10)
    at afterWrite (_stream_writable.js:469:5)
    at onwrite (_stream_writable.js:449:7)
    at WriteWrap.onWriteComplete [as oncomplete] (internal/stream_base_commons.js:87:12)
Command: out/Release/node /Users/ronagy/GitHub/nxtedition/node/test/parallel/test-tls-set-secure-context.js

[ronag]

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.

[ronag]

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