Skip to content

stream: fix pipeline pump#39006

Closed
ronag wants to merge 13 commits intonodejs:masterfrom
nxtedition:fix-pipeline-pump
Closed

stream: fix pipeline pump#39006
ronag wants to merge 13 commits intonodejs:masterfrom
nxtedition:fix-pipeline-pump

Conversation

@ronag
Copy link
Member

@ronag ronag commented Jun 11, 2021

Refs: #39005

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Jun 11, 2021
@ronag ronag requested a review from mcollina June 11, 2021 09:16
@ronag
Copy link
Member Author

ronag commented Jun 11, 2021

Needs tests.

@github-actions github-actions bot added the needs-ci PRs that need a full CI run. label Jun 11, 2021
@ronag ronag force-pushed the fix-pipeline-pump branch from 1b18705 to 963f536 Compare June 11, 2021 09:58
@VoltrexKeyva

This comment has been minimized.

Sign in to view
@VoltrexKeyva

This comment has been minimized.

Sign in to view
@ronag
Copy link
Member Author

ronag commented Jun 11, 2021 •
edited
Loading

@VoltrexMaster Thanks for the help. Why not make github change suggestions? That way I can just commit your changes directly.

Screen Shot 2021-06-11 at 14 18 30

In general it's better to review the code in the code instead of in comments.

ronag
ronag commented Jun 11, 2021
View reviewed changes
@VoltrexKeyva
Copy link
Member

VoltrexKeyva commented Jun 11, 2021

@ronag yea sorry, I already know how those work; just placed those in a comment as github doesn't allow suggestions in unchanged lines which for example the const { line, wanted to be more specific if you know what I mean 😅

RaisinTen
RaisinTen reviewed Jun 11, 2021
View reviewed changes
@ronag @RaisinTen
Update lib/internal/streams/pipeline.js
1cd6a1c 
Co-authored-by: Darshan Sen <raisinten@gmail.com>
aduh95
aduh95 reviewed Jun 11, 2021
View reviewed changes
ronag and others added 2 commits June 11, 2021 15:24
@ronag @aduh95
Apply suggestions from code review
33c4e21 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ronag
fixup
30b7e8d
aduh95
aduh95 reviewed Jun 11, 2021
View reviewed changes
mcollina
mcollina requested changes Jun 11, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you please add a unit test?

ronag and others added 3 commits June 11, 2021 16:27
@ronag @aduh95
Apply suggestions from code review
e6a62f4 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ronag
fixup: test
2b453dc
@ronag
fixup: test
f12cc3d
mcollina
mcollina requested changes Jun 11, 2021
View reviewed changes
aduh95
aduh95 reviewed Jun 11, 2021
View reviewed changes
@ronag @aduh95
Update lib/internal/streams/pipeline.js
4e6be16 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
mcollina
mcollina approved these changes Jun 12, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 12, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 12, 2021 •
edited by ronag
Loading

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

@ronag
Copy link
Member Author

ronag commented Jun 12, 2021

@nodejs/streams

mcollina
mcollina approved these changes Jun 12, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@jasnell
Copy link
Member

jasnell commented Jun 14, 2021

Landed in bdcb738

@jasnell jasnell closed this Jun 14, 2021
jasnell pushed a commit that referenced this pull request Jun 14, 2021
@ronag @jasnell
stream: fix pipeline pump
bdcb738 
Refs: #39005

PR-URL: #39006
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 15, 2021
@ronag @danielleadams
stream: fix pipeline pump
c20e28e 
Refs: #39005

PR-URL: #39006
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Jun 15, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jun 17, 2021
@ronag @danielleadams
stream: fix pipeline pump
8dc8610 
Refs: #39005

PR-URL: #39006
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau
Copy link
Member

richardlau commented Jul 19, 2021

This doesn't land cleanly on v14.x-staging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments

@RaisinTen RaisinTen RaisinTen left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ronag @VoltrexKeyva @nodejs-github-bot @jasnell @richardlau @mcollina @aduh95 @RaisinTen