Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: make all streams error in a pipeline #30869

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Dec 9, 2019

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
Copy link
Member Author

mcollina commented Dec 9, 2019

cc @ronag @marcosc90 please take a look.

@mcollina
Copy link
Member Author

mcollina commented Dec 9, 2019

cc @nodejs/streams

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member Author

mcollina commented Dec 9, 2019

@mcollina
Copy link
Member Author

mcollina commented Dec 9, 2019

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.

@mcollina mcollina requested a review from lpinca December 9, 2019 19:55
@@ -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) {

This comment was marked as resolved.

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

This comment was marked as resolved.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I like it

@marcosc90
Copy link
Contributor

LGTM, Thanks @mcollina

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

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@mafintosh
Copy link
Member

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

@mcollina
Copy link
Member Author

mcollina commented Dec 9, 2019

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

(There are some CITGM failures that seems unrelated)

@mafintosh
Copy link
Member

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

@mcollina mcollina mentioned this pull request Dec 9, 2019
4 tasks
@mcollina mcollina marked this pull request as ready for review December 10, 2019 09:44
@mcollina
Copy link
Member Author

The docs already mention calling .destroy(err).

@mcollina mcollina added lts-watch-v10.x stream Issues and PRs related to the stream subsystem. labels Dec 10, 2019
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
Copy link
Collaborator

@mcollina
Copy link
Member Author

@mcollina
Copy link
Member Author

1 similar comment
@mcollina
Copy link
Member Author

@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

mcollina added a commit that referenced this pull request Dec 14, 2019
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
Copy link
Member Author

Landed in 6480882

@mcollina mcollina closed this Dec 14, 2019
@mcollina mcollina deleted the fix-pipeline-async-iterator branch December 14, 2019 15:17
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
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>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
@bitinn bitinn mentioned this pull request Dec 28, 2019
35 tasks
@targos
Copy link
Member

targos commented Jan 14, 2020

I'm adding the backport-requested label because it seems like this shouldn't land without #31054

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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>
codebytere pushed a commit that referenced this pull request Jun 6, 2020
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>
@codebytere codebytere mentioned this pull request Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stream: Readable iterator unhandled error when piping
10 participants