stream: make all streams error in a pipeline #30869

mcollina · 2019-12-09T19:52:22Z

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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mcollina · 2019-12-09T19:52:45Z

cc @ronag @marcosc90 please take a look.

mcollina · 2019-12-09T19:53:14Z

cc @nodejs/streams

nodejs-github-bot · 2019-12-09T19:54:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27526/

mcollina · 2019-12-09T19:54:24Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2119/

mcollina · 2019-12-09T19:55:16Z

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

@@ -43,15 +43,21 @@ function destroyer(stream, reading, writing, callback) {

    // request.destroy just do .end - .abort is what we want
    if (isRequest(stream)) return stream.abort();
-    if (typeof stream.destroy === 'function') return stream.destroy();
+    if (typeof stream.destroy === 'function') {
+      if (stream.req && stream._writableState === undefined) {


lib/internal/streams/pipeline.js

-      destroys.forEach(call);
+      for (const destroy of destroys) {
+        destroy();
+      }


ronag

LGTM, I like it

marcosc90 · 2019-12-09T20:09:43Z

LGTM, Thanks @mcollina

I think it would be better if we can drop the mandatory callback though.

lpinca

SGTM

mafintosh · 2019-12-09T20:42:47Z

👍 the destroy with no error was originally added to support v old streams where the 1st arg was a function afair.

mcollina · 2019-12-09T21:15:57Z

CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2120/

(There are some CITGM failures that seems unrelated)

mafintosh · 2019-12-09T21:20:14Z

I’d say this is semver patch as the destroy(cb) is ancient. I originally wrote that +5 years ago.

mcollina · 2019-12-10T09:44:47Z

The docs already mention calling .destroy(err).

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

nodejs-github-bot · 2019-12-12T22:05:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27598/

mcollina · 2019-12-12T22:05:52Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2123/

mcollina · 2019-12-13T16:54:37Z

CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2125

mcollina · 2019-12-13T17:04:27Z

CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2125

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>

mcollina · 2019-12-14T15:17:18Z

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>

targos · 2020-01-14T13:31:48Z

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>

mcollina requested a review from mafintosh December 9, 2019 19:52

mcollina mentioned this pull request Dec 9, 2019

stream: propagate errors from src streams in async iterator #30861

Closed

3 tasks

mcollina requested review from addaleax and jasnell December 9, 2019 19:53

mcollina requested a review from lpinca December 9, 2019 19:55

ronag reviewed Dec 9, 2019

View reviewed changes

lib/internal/streams/pipeline.js

destroys.forEach(call);

for (const destroy of destroys) {

destroy();

}

This comment was marked as resolved.

Sign in to view

ronag approved these changes Dec 9, 2019

View reviewed changes

lpinca approved these changes Dec 9, 2019

View reviewed changes

mafintosh approved these changes Dec 9, 2019

View reviewed changes

mcollina mentioned this pull request Dec 9, 2019

[x] stream: eof & pipeline compat #29724

Closed

4 tasks

mcollina marked this pull request as ready for review December 10, 2019 09:44

mcollina added lts-watch-v10.x stream Issues and PRs related to the stream subsystem. labels Dec 10, 2019

marcosc90 approved these changes Dec 12, 2019

View reviewed changes

mcollina force-pushed the fix-pipeline-async-iterator branch from 888c798 to a384141 Compare December 12, 2019 22:05

This comment has been minimized.

Sign in to view

Trott approved these changes Dec 13, 2019

View reviewed changes

mcollina closed this Dec 14, 2019

mcollina deleted the fix-pipeline-async-iterator branch December 14, 2019 15:17

MylesBorins mentioned this pull request Dec 17, 2019

v13.5.0 proposal #31010

Merged

bitinn mentioned this pull request Dec 28, 2019

v3 Roadmap node-fetch/node-fetch#668

Closed

35 tasks

targos added the backport-requested-v12.x label Jan 14, 2020

targos mentioned this pull request Jan 14, 2020

stream: pipeline should use req.abort() to destroy response #31054

Closed

4 tasks

BethGriggs added the backport-requested-v10.x label Feb 27, 2020

szmarczak mentioned this pull request Mar 5, 2020

Pipeline aborts all HTTP requests #32105

Closed

targos added backport-open-v12.x and removed backport-requested-v12.x labels Apr 25, 2020

targos mentioned this pull request Apr 25, 2020

stream: support passing generator functions into pipeline #31223

Closed

4 tasks

codebytere mentioned this pull request Jun 9, 2020

v12.18.1 proposal #33811

Merged

addaleax added backported-to-v12.x and removed backport-open-v12.x labels Sep 23, 2020

watson mentioned this pull request Nov 9, 2020

Stream emitting same error twice under certain circomstances #36053

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: make all streams error in a pipeline #30869

stream: make all streams error in a pipeline #30869

mcollina commented Dec 9, 2019

mcollina commented Dec 9, 2019

mcollina commented Dec 9, 2019

nodejs-github-bot commented Dec 9, 2019

mcollina commented Dec 9, 2019

mcollina commented Dec 9, 2019 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

ronag left a comment

marcosc90 commented Dec 9, 2019

lpinca left a comment

mafintosh commented Dec 9, 2019

mcollina commented Dec 9, 2019

mafintosh commented Dec 9, 2019

mcollina commented Dec 10, 2019

nodejs-github-bot commented Dec 12, 2019

mcollina commented Dec 12, 2019

mcollina commented Dec 13, 2019

mcollina commented Dec 13, 2019

This comment has been minimized.

This comment has been minimized.

mcollina commented Dec 14, 2019

targos commented Jan 14, 2020

stream: make all streams error in a pipeline #30869

stream: make all streams error in a pipeline #30869

Conversation

mcollina commented Dec 9, 2019

Checklist

mcollina commented Dec 9, 2019

mcollina commented Dec 9, 2019

nodejs-github-bot commented Dec 9, 2019

mcollina commented Dec 9, 2019

mcollina commented Dec 9, 2019 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

ronag left a comment

Choose a reason for hiding this comment

marcosc90 commented Dec 9, 2019

lpinca left a comment

Choose a reason for hiding this comment

mafintosh commented Dec 9, 2019

mcollina commented Dec 9, 2019

mafintosh commented Dec 9, 2019

mcollina commented Dec 10, 2019

nodejs-github-bot commented Dec 12, 2019

mcollina commented Dec 12, 2019

mcollina commented Dec 13, 2019

mcollina commented Dec 13, 2019

This comment has been minimized.

This comment has been minimized.

mcollina commented Dec 14, 2019

targos commented Jan 14, 2020

mcollina commented Dec 9, 2019 •

edited

Loading