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: support passing generator functions into pipeline() #31975

Closed
wants to merge 3 commits into from
Closed

[v13.x backport] stream: support passing generator functions into pipeline() #31975

wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Feb 26, 2020 •
edited
Loading

#31223

Also includes some required follow up fixes.

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

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. v13.x labels Feb 26, 2020
@ronag
Copy link
Member Author

ronag commented Feb 26, 2020

Since this backport contains multiple commits I'm unsure where to add the Backport-PR-URL: meta. Should it be on every commit?

@ronag ronag requested a review from codebytere February 26, 2020 22:54
@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag added stream Issues and PRs related to the stream subsystem. notable-change PRs with changes that should be highlighted in changelogs. and removed tools Issues and PRs related to the tools directory. labels Feb 26, 2020
@ronag ronag requested a review from mcollina February 26, 2020 23:17
@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed notable-change PRs with changes that should be highlighted in changelogs. labels Feb 26, 2020
codebytere
codebytere approved these changes Feb 27, 2020
View reviewed changes
Copy link
Member

@codebytere codebytere left a comment •
edited
Loading

Choose a reason for hiding this comment

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

🙇‍♀ ty - and yes @ronag every commit. I'll get this merged as soon as that's been updated as long as @mcollina has no concerns!

ronag added 3 commits February 28, 2020 00:24
@ronag
stream: support passing generator functions into pipeline()
8af1387 
Backport-PR-URL: nodejs#31975
PR-URL: nodejs#31223
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@ronag
stream: fix broken pipeline error propagation
21a0dd0 
If the destination was an async function any
error thrown from that function would be swallowed.

Backport-PR-URL: nodejs#31975
PR-URL: nodejs#31835
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@ronag
stream: ensure pipeline always destroys streams
23011b9 
There was an edge case where an incorrect assumption was made
in regardos whether eos/finished means that the stream is
actually destroyed or not.

Backport-PR-URL: nodejs#31975
PR-URL: nodejs#31940
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@ronag ronag force-pushed the backport-pipeline-generator branch from 995dcc6 to 23011b9 Compare February 27, 2020 23:25
@ronag
Copy link
Member Author

ronag commented Feb 27, 2020

Added Backport-PR-URL to each commit

@codebytere
Copy link
Member

codebytere commented Feb 29, 2020 •
edited
Loading

@ronag each commit also needs the normal PR-URL pointing to the initial PR

(update, nevermind - this looks like a bug in ncu landing behavior!)

@codebytere
Copy link
Member

@nodejs/releasers would someone else be willing to review this? with only 1 approval the required waiting period is 97 more hours and I'd like to finish preparing the release unless this can be left off it. It does, however, block clean landing of ~10 other streams backports.

Trott
Trott approved these changes Feb 29, 2020
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

A little bit rubber-stampy of me but this looks good to me.

@codebytere codebytere mentioned this pull request Feb 29, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 1, 2020
@ronag @codebytere
stream: support passing generator functions into pipeline()
8ad64b8 
Backport-PR-URL: #31975
PR-URL: #31223
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 1, 2020
@ronag @codebytere
stream: fix broken pipeline error propagation
313ecaa 
If the destination was an async function any
error thrown from that function would be swallowed.

Backport-PR-URL: #31975
PR-URL: #31835
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 1, 2020
@ronag @codebytere
stream: ensure pipeline always destroys streams
8a2b62e 
There was an edge case where an incorrect assumption was made
in regardos whether eos/finished means that the stream is
actually destroyed or not.

Backport-PR-URL: #31975
PR-URL: #31940
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@codebytere
Copy link
Member

codebytere commented Mar 1, 2020 •
edited by targos
Loading

Landed in 8ad64b8...8a2b62e

@codebytere codebytere closed this Mar 1, 2020
@ronag ronag mentioned this pull request Mar 9, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codebytere codebytere codebytere approved these changes

@Trott Trott Trott approved these changes

@mcollina mcollina Awaiting requested review from mcollina

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. 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.
4 participants
@ronag @nodejs-github-bot @codebytere @Trott