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

stream: async iterator improvements #31316

Closed
wants to merge 5 commits into from
Closed

stream: async iterator improvements #31316

wants to merge 5 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 11, 2020 •
edited
Loading

Includes a few different fixes separated into commits:

  • Adds support to create async iterators from v1 streams.
  • Normalize destroying non conforming streams.
  • Implement throw on the iterator.
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

@ronag
Copy link
Member Author

ronag commented Jan 11, 2020

ping @addaleax

@ronag ronag force-pushed the stream-async-iterator branch 2 times, most recently from f87b6e6 to 7558199 Compare January 11, 2020 16:27
@ronag ronag changed the title stream: add async iterator support for v1 streams stream: async iterator fixes Jan 11, 2020
@ronag ronag force-pushed the stream-async-iterator branch 6 times, most recently from a4f1b62 to 00d40b3 Compare January 11, 2020 17:30
@ronag ronag changed the title stream: async iterator fixes stream: async iterator improvements Jan 11, 2020
@ronag ronag force-pushed the stream-async-iterator branch 2 times, most recently from 1e6c55e to f1c4e5a Compare January 12, 2020 00:06
@ronag ronag force-pushed the stream-async-iterator branch 2 times, most recently from 5c034b4 to 54a560b Compare January 12, 2020 11:44
@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina approved these changes Jan 12, 2020
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

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem. labels Jan 12, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@ronag ronag closed this Jan 26, 2020
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@ronag @codebytere
stream: add async iterator support for v1 streams
20d0a0e 
PR-URL: #31316
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@ronag @codebytere
stream: normalize async iterator stream destroy
5a95fa4 
PR-URL: #31316
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@ronag @codebytere
stream: implement throw for async iterator
3046648 
PR-URL: #31316
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
@codebytere
Copy link
Member

@ronag the last two commits in this PR need manual backports

@MylesBorins MylesBorins mentioned this pull request Mar 9, 2020
Closed
7 tasks
@ronag ronag mentioned this pull request Mar 10, 2020
Closed
4 tasks
ronag added a commit to nxtedition/node that referenced this pull request Mar 10, 2020
@ronag
stream: simplify pipeline
0abd4f0 
PR-URL: nodejs#31316
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Backport-PR-URL: nodejs#32174
ronag added a commit to nxtedition/node that referenced this pull request Mar 10, 2020
@ronag
stream: re-use legacy destroyer
2578e5d 
PR-URL: nodejs#31316
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Backport-PR-URL: nodejs#32174
MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
@ronag @MylesBorins
stream: simplify pipeline
7ce1cc9 
Backport-PR-URL: #32174
PR-URL: #31316
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
@ronag @MylesBorins
stream: re-use legacy destroyer
4b04bf8 
Backport-PR-URL: #32174
PR-URL: #31316
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
Merged
@codebytere
Copy link
Member

@ronag should this go to v12.x, and if yes, can you open a manual backport? Given the recent issues on v13.x i'd like to play it safe so feel free to update the label to don't-land.

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
This was referenced Jun 16, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reklatsmasters reklatsmasters reklatsmasters left review comments

@JungMinu JungMinu JungMinu approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
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.