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.

Sign up for GitHub

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

Jump to bottom

[v13.x backport] stream: pipeline should only destroy un-finished streams. #32980

Closed
wants to merge 2 commits into from
Closed

[v13.x backport] stream: pipeline should only destroy un-finished streams. #32980

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 21, 2020 •
edited
Loading

PR-URL: #32968

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

@ronag ronag added stream Issues and PRs related to the stream subsystem. v13.x labels Apr 21, 2020
@ronag ronag added the wip Issues and PRs that are still a work in progress. label Apr 21, 2020
@ronag
Copy link
Member Author

ronag commented Apr 21, 2020

Since the main PR has not landed yet the commit does not contain correct meta.

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
stream: pipeline should only destroy un-finished streams
9e7f255 
This PR logically reverts nodejs#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 nodejs#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: nodejs#32954
Fixes: nodejs#32955

PR-URL: nodejs#32968
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mathias Buus <[email protected]>
Backport-PR-URL: nodejs#32980
@ronag ronag removed the wip Issues and PRs that are still a work in progress. label Apr 23, 2020
@ronag ronag requested a review from mcollina April 23, 2020 18:01
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag added the wip Issues and PRs that are still a work in progress. label Apr 23, 2020
@BridgeAR
Copy link
Member

@ronag the main PR landed, I guess this is therefore not WIP anymore?

@ronag
Copy link
Member Author

ronag commented Apr 28, 2020

@BridgeAR This doesn't pass CI yet. I will sort it out this week.

@ronag
Copy link
Member Author

ronag commented Apr 28, 2020

@targos @mcollina is this something I should continue working on and have ready by the end of the week or close it?

@mcollina
Copy link
Member

I would just close.

@ronag ronag closed this Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina Awaiting requested review from mcollina

Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ronag @nodejs-github-bot @BridgeAR @mcollina