stream: prevent pipeline hang with generator functions #47712
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Collaborator
| Review requested:
|
ronag approved these changes Apr 25, 2023
aduh95 added author ready 
github-actions bot removed the request-ci This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Member Author
| Seems CI agrees! 🎉 |
aduh95 added the commit-queue 
climba03003 reviewed Apr 25, 2023
debadree25 added request-ci github-actions bot removed the request-ci This comment was marked as outdated.
This comment was marked as outdated.
F3n67u approved these changes Apr 25, 2023
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
36 tasks
debadree25 added the commit-queue 35 tasks
nodejs-github-bot removed the commit-queue Collaborator
| Landed in f34a07d |
29 tasks
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
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
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>
32 tasks
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 29, 2023
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
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
In the previous implementation
we had
node/lib/internal/streams/pipeline.js
Line 126 in 32e478d
which would have called
resume()only when the stream ended thus resolving the promise over herenode/lib/internal/streams/pipeline.js
Line 143 in 32e478d
but for
end: falseeos would never call resume as the stream doesn't end, hence wewait()only if the stream is stated to endFixes: #47708