stream: make all streams error in a pipeline#30869
stream: make all streams error in a pipeline#30869mcollina wants to merge 1 commit intonodejs:masterfrom
Conversation
|
cc @ronag @marcosc90 please take a look. |
|
cc @nodejs/streams |
|
This will require some thoughts on a semverness profile. On one hand, it's a change of behavior. On the other hand, it's a needed fix for a key feature in streams. Regardless, it's worth an update on docs, that's why it's a draft. |
lib/internal/streams/pipeline.js
Outdated
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lib/internal/streams/pipeline.js
Outdated
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
LGTM, Thanks @mcollina I think it would be better if we can drop the mandatory callback though. |
|
👍 the destroy with no error was originally added to support v old streams where the 1st arg was a function afair. |
|
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2120/ (There are some CITGM failures that seems unrelated) |
|
I’d say this is semver patch as the destroy(cb) is ancient. I originally wrote that +5 years ago. |
|
The docs already mention calling |
This changes makes all stream in a pipeline emit 'error' in case of an abnormal termination of the pipeline. If the last stream is currently being async iterated, this change will make the iteration reject accordingly. See: nodejs#30861 Fixes: nodejs#28194
888c798 to
a384141
Compare
|
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2125 |
1 similar comment
|
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2125 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This changes makes all stream in a pipeline emit 'error' in case of an abnormal termination of the pipeline. If the last stream is currently being async iterated, this change will make the iteration reject accordingly. See: #30861 Fixes: #28194 PR-URL: #30869 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
|
Landed in 6480882 |
This changes makes all stream in a pipeline emit 'error' in case of an abnormal termination of the pipeline. If the last stream is currently being async iterated, this change will make the iteration reject accordingly. See: #30861 Fixes: #28194 PR-URL: #30869 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
|
I'm adding the backport-requested label because it seems like this shouldn't land without #31054 |
This changes makes all stream in a pipeline emit 'error' in case of an abnormal termination of the pipeline. If the last stream is currently being async iterated, this change will make the iteration reject accordingly. See: nodejs#30861 Fixes: nodejs#28194 PR-URL: nodejs#30869 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This changes makes all stream in a pipeline emit 'error' in case of an abnormal termination of the pipeline. If the last stream is currently being async iterated, this change will make the iteration reject accordingly. See: #30861 Fixes: #28194 PR-URL: #30869 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This changes makes all stream in a pipeline emit 'error' in
case of an abnormal termination of the pipeline. If the last stream
is currently being async iterated, this change will make the iteration
reject accordingly.
See: #30861
Fixes: #28194
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes