stream: always invoke callback before emitting error
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), orvcbuild 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?