This PR logically reverts https://github.com/nodejs/node/pull/31940
which has caused lots of unnecessary breakage in the ecosystem.
This PR also aligns better with the actual documented behavior:
`stream.pipeline()` will call `stream.destroy(err)` on all streams
except:
* `Readable` streams which have emitted `'end'` or `'close'`.
* `Writable` streams which have emitted `'finish'` or `'close'`.
The behavior introduced in https://github.com/nodejs/node/pull/31940
was much more aggressive in terms of destroying streams. This was
good for avoiding potential resources leaks however breaks some
common assumputions in legacy streams.
Furthermore, it makes the code simpler and removes some hacks.
Fixes: https://github.com/nodejs/node/issues/32954
Fixes: https://github.com/nodejs/node/issues/32955
PR-URL: https://github.com/nodejs/node/pull/32968
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>
pipeline did not support destination with generator
that does not return strings or buffers.
PR-URL: https://github.com/nodejs/node/pull/32414
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Adds a test to ensure that destroying the returned stream
of pipeline will cause a premature close error.
PR-URL: https://github.com/nodejs/node/pull/32425
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
User might still want to be able to use the writable side
of src. This is in the case where e.g. the Duplex input
is not directly connected to its output. Such a case could
happen when the Duplex is reading from a socket and then echos
the data back on the same socket.
PR-URL: https://github.com/nodejs/node/pull/32198
Refs: 4d93e105bf (commitcomment-37751035)
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
http1 objects are coupled with their corresponding
res/req and cannot be treated independently as
normal streams. Add a special exception for this
in the pipeline cleanup.
Fixes: https://github.com/nodejs/node/issues/32184
PR-URL: https://github.com/nodejs/node/pull/32197
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Before this patch, pipeline() could swallow errors by pre-emptively
producing a ERR_STREAM_PREMATURE_CLOSE that was not really helpful
to the user.
Co-Authored-By: Robert Nagy <ronagy@icloud.com>
PR-URL: https://github.com/nodejs/node/pull/32051
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Emitting 'close' before 'end' on a Readable should
result in a premature close error.
PR-URL: https://github.com/nodejs/node/pull/31545
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Previously destroy could be called multiple times causing inconsistent
and hard to predict behavior. Furthermore, since the stream _destroy
implementation can only be called once, the behavior of applying destroy
multiple times becomes unclear.
This changes so that only the first destroy() call is executed and any
subsequent calls are noops.
PR-URL: https://github.com/nodejs/node/pull/29197
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
There was an edge case where an incorrect assumption was made
in regardos whether eos/finished means that the stream is
actually destroyed or not.
PR-URL: https://github.com/nodejs/node/pull/31940
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
If the destination was an async function any
error thrown from that function would be swallowed.
PR-URL: https://github.com/nodejs/node/pull/31835
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
destroy(err) on http response will propagate the error to the
request causing 'error' to be unexpectedly emitted. Furthermore,
response.destroy() unlike request.abort() does not _dump buffered
data.
Fixes a breaking change introduced in 648088289d.
Prefer res.req.abort() over res.destroy() until this situation is
clarified.
Fixes: https://github.com/nodejs/node/issues/31029
Refs: 648088289d
PR-URL: https://github.com/nodejs/node/pull/31054
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This updates a lot of comments.
PR-URL: https://github.com/nodejs/node/pull/26223
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Splitting out the http2 portion of the test has a few benfits:
* We don't skip the rest of the tests if `node` is compiled without
crypto.
* We can find out if the http2 portion of the test is responsible for
the timeouts reported in issue 24456.
Refs: https://github.com/nodejs/node/issues/24456
PR-URL: https://github.com/nodejs/node/pull/24631
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This test is ported automatically in readable-stream, and it fails there
on Windows and older Node.js versions because of some bad interactions
between the code and the event loop on Windows.
See: https://github.com/nodejs/readable-stream/issues/353
PR-URL: https://github.com/nodejs/node/pull/22456
Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit removes `common.crashOnUnhandledRejection()` and adds
`common.disableCrashOnUnhandledRejection()`.
To reduce the risk of mistakes and make writing tests that involve
promises simpler, always install the unhandledRejection hook in tests
and provide a way to disable it for the rare cases where it's needed.
PR-URL: https://github.com/nodejs/node/pull/21849
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Right now when not adding a callback to the pipeline it could cause
an uncaught exception if there is an error. Instead, just make the
callback mandatory as mostly done in all other Node.js callback APIs
so users explicitly have to decide what to do in such situations.
PR-URL: https://github.com/nodejs/node/pull/21054
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
The bug referenced in this TODO was fixed and this test no longer
requires this code to pass.
PR-URL: https://github.com/nodejs/node/pull/21145
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes an issue where Stream.pipeline wouldn't re-throw errors
on a stream if no callback was specified, thus swallowing
said errors.
Fixes: https://github.com/nodejs/node/issues/20303
PR-URL: https://github.com/nodejs/node/pull/20437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
