Skip to content

stream: fix fromAsyncGen - #40499

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:fix-40497
Closed

stream: fix fromAsyncGen#40499
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:fix-40497

Conversation

@ronag

@ronagronag commented Oct 18, 2021

Copy link
Copy Markdown
Member

Fixes: #40497

The problem here is that the resolve does not get updated before yielding control back to the user, hence the user might call the wrong resolve and hang the pipeline.

@ronagronag added stream Issues and PRs related to the stream subsystem. v16.x labels Oct 18, 2021
@ronag
ronag requested a review from mcollinaOctober 18, 2021 10:09
@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 18, 2021
@ronag

Copy link
Copy Markdown
MemberAuthor

@nodejs/streams

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@ronagronag 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. and removed needs-ci PRs that need a full CI run. labels Oct 18, 2021
@ronag
ronag requested a review from lpincaOctober 18, 2021 10:19
@ronag

ronag commented Oct 18, 2021

Copy link
Copy Markdown
MemberAuthor

Credit to @kmamal for making me aware of this issue and writing a repro.

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2021
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

nodejs-github-bot commented Oct 18, 2021

Copy link
Copy Markdown
Collaborator

@ronagronag mentioned this pull request Oct 19, 2021
@ronag

Copy link
Copy Markdown
MemberAuthor

@benjamingr@lpinca@jasnell would appreciate another review here so we can land it in the next release.

@ronag

Copy link
Copy Markdown
MemberAuthor

Can we fast-track this so it can be included in 16.12 #40504 (comment)

@ronagronag added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 19, 2021
@github-actions

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @ronag. Please 👍 to approve.

@richardlaurichardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2021
@github-actionsgithub-actionsBot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2021
@github-actions

Copy link
Copy Markdown
Contributor

Landed in 2bed031...0f78d26

nodejs-github-bot pushed a commit that referenced this pull request Oct 19, 2021
Fixes: #40497
PR-URL: #40499
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
richardlau pushed a commit that referenced this pull request Oct 19, 2021
Fixes: #40497
PR-URL: #40499
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Oct 20, 2021
Fixes: #40497
PR-URL: #40499
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos added a commit that referenced this pull request Oct 20, 2021
Notable changes:
Fixed distribution for native addon builds
This release fixes an issue introduced in Node.js v17.0.0, where some V8
headers were missing from the distributed tarball, making it impossible
to build native addons. These headers are now included.
#40526
Fixed stream issues
* Fixed a regression in `stream.promises.pipeline`, which was introduced
in version 16.10.0, is fixed. It is now possible again to pass an
array of streams to the function.
#40193
* Fixed a bug in `stream.Duplex.from`, which didn't work properly when
an async generator function was passed to it.
#40499
PR-URL: #40535
@targostargos mentioned this pull request Oct 20, 2021
targos added a commit that referenced this pull request Oct 20, 2021
Notable changes:
Fixed distribution for native addon builds
This release fixes an issue introduced in Node.js v17.0.0, where some V8
headers were missing from the distributed tarball, making it impossible
to build native addons. These headers are now included.
#40526
Fixed stream issues
* Fixed a regression in `stream.promises.pipeline`, which was introduced
in version 16.10.0, is fixed. It is now possible again to pass an
array of streams to the function.
#40193
* Fixed a bug in `stream.Duplex.from`, which didn't work properly when
an async generator function was passed to it.
#40499
PR-URL: #40535
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fast-trackPRs that do not need to wait for 48 hours to land.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplex.from failure

5 participants

@ronag@nodejs-github-bot@mcollina@benjamingr@richardlau