Skip to content

stream: always invoke callback before emitting error

Rodrigo Muino Tomonari requested to merge github/fork/nxtedition/stream-cb-error-order into master

This is a very subtle but possibly serious bug.

The order of invoking the callback and emitting the error should be the same regardless if sync or async. We have code that depends on this order, e.g. https://github.com/nodejs/node/blob/d5c383706130638d9772c5fdc18545a90424bff3/lib/internal/console/constructor.js#L190

In particular the callback should give the user a chance to install an 'error' handler before the error is emitted. If the 'error' event sometimes (depending on sync/async write()) is emitted before the callback, this might not always be possible/consistent.

This also fixes so that error is emitted asynchronously instead of synchronously in some places, as you might notice in the tests.

Encountered this while working on https://github.com/nodejs/node/pull/29179.

EDIT: This is blocking work on a few other PR's.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

NOTE TO SELF: after merge looking into optimising away a redudant nextTick caused by destroy. Also destroyed could be set synchronously?

Merge request reports