Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

stream: prevent pipeline hang with generator functions #47712

Merged
merged 2 commits into from
Apr 27, 2023
Merged

stream: prevent pipeline hang with generator functions #47712

merged 2 commits into from
Apr 27, 2023

Conversation

debadree25
Copy link
Member

In the previous implementation
we had

node/lib/internal/streams/pipeline.js

Line 126 in 32e478d

const cleanup = eos(writable, { readable: false }, resume);

which would have called resume() only when the stream ended thus resolving the promise over here

node/lib/internal/streams/pipeline.js

Line 143 in 32e478d

await wait();

but for end: false eos would never call resume as the stream doesn't end, hence we wait() only if the stream is stated to end

Fixes: #47708

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@debadree25 debadree25 requested a review from ronag April 25, 2023 07:39
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 25, 2023
aduh95
aduh95 approved these changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

LGTM if CI agrees

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

# to view
@nodejs-github-bot

This comment was marked as outdated.

# to view
@debadree25
Copy link
Member Author

Seems CI agrees! 🎉

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2023
@debadree25 debadree25 added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

# to view
@nodejs-github-bot

This comment was marked as outdated.

# to view
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Apr 26, 2023
Open
36 tasks
@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 26, 2023
@github-actions github-actions bot mentioned this pull request Apr 27, 2023
Open
35 tasks
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2023
@nodejs-github-bot nodejs-github-bot merged commit f34a07d into nodejs:main Apr 27, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in f34a07d

@github-actions github-actions bot mentioned this pull request Apr 28, 2023
Open
29 tasks
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
@debadree25 @yjl9903
stream: prevent pipeline hang with generator functions
11abb06 
Fixes: nodejs#47708
PR-URL: nodejs#47712
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
@debadree25 @yjl9903
stream: prevent pipeline hang with generator functions
dd5048e 
Fixes: nodejs#47708
PR-URL: nodejs#47712
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
@github-actions github-actions bot mentioned this pull request Apr 29, 2023
Open
32 tasks
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 29, 2023
@debadree25 @yjl9903
stream: prevent pipeline hang with generator functions
f917263 
Fixes: nodejs#47708
PR-URL: nodejs#47712
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
This was referenced Apr 30, 2023
Open
Open
targos pushed a commit that referenced this pull request May 2, 2023
@debadree25 @targos
stream: prevent pipeline hang with generator functions
7048bfb 
Fixes: #47708
PR-URL: #47712
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
@targos targos mentioned this pull request May 2, 2023
Merged
targos pushed a commit that referenced this pull request May 3, 2023
@debadree25 @targos
stream: prevent pipeline hang with generator functions
f88132f 
Fixes: #47708
PR-URL: #47712
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@debadree25 @danielleadams
stream: prevent pipeline hang with generator functions
cd6a1a9 
Fixes: #47708
PR-URL: #47712
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@debadree25 @MoLow
stream: prevent pipeline hang with generator functions
d3449ca 
Fixes: nodejs#47708
PR-URL: nodejs#47712
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@climba03003 climba03003 climba03003 left review comments

@ronag ronag ronag approved these changes

@F3n67u F3n67u F3n67u approved these changes

@aduh95 aduh95 aduh95 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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

stream: pipeline with option end=false doesn't finish
6 participants
@debadree25 @nodejs-github-bot @ronag @F3n67u @aduh95 @climba03003