Skip to content

stream: simpler Readable async iterator#34035

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
Closed
ronag wants to merge 1 commit into from
Closed

stream: simpler Readable async iterator #34035

ronag wants to merge 1 commit into from

Conversation

@ronag
Copy link
Member

@ronagronag commented Jun 23, 2020•
edited
Loading

Simplifies async iteration for Readable using async generator.

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

@ronagronag added the stream Issues and PRs related to the stream subsystem. label Jun 23, 2020
@ronagronagforce-pushed the readable-async-iterator branch 2 times, most recently from a754d9b to bdded9eCompareJune 23, 2020 21:12
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 23, 2020

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

@ronag
Copy link
MemberAuthor

ronag commented Jun 23, 2020

Might be relevant for #30298

ronag
ronag commented Jun 23, 2020
View reviewed changes
ronag
ronag commented Jun 23, 2020
View reviewed changes
@ronag
Copy link
MemberAuthor

ronag commented Jun 23, 2020

Some really strange CI failures... will investigate

This was referenced Jun 24, 2020
Open
Open
benjamingr
benjamingr reviewed Jun 25, 2020
View reviewed changes
@ronagronagforce-pushed the readable-async-iterator branch 11 times, most recently from 647a0ab to 0bbbed1CompareJune 25, 2020 13:19
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@ronagronagforce-pushed the readable-async-iterator branch 5 times, most recently from a111d8d to 8944c59CompareJune 25, 2020 13:37
@ronagronag requested a review from mcollinaJune 25, 2020 13:42
@ronag
Copy link
MemberAuthor

ronag commented Jun 25, 2020•
edited
Loading

@mcollina PTAL when you have time. No hurry.

There are 3 tests that I have commented out which fail. These failures seem to be related to difference between how an async iterator from an async generator i.e. async function* works and how our custom async iterator works. I would guess that the behavior from async generator should be more spec compliant? Or maybe it's a timing issue? Either way probably a semver-major.

  1. the async iterator prototype test.
  2. next promises are not rejected, but instead resolved

ronag added a commit that referenced this pull request Jul 17, 2020
@ronag
stream: simpler and faster Readable async iterator
08e8997
Reimplement as an async generator instead of a custom iterator class. PR-URL: #34035 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
@ronag
Copy link
MemberAuthor

ronag commented Jul 17, 2020

Landed in 08e8997

@ronagronag closed this Jul 17, 2020
Trott
Trott reviewed Jul 17, 2020
View reviewed changes
@ronagronag mentioned this pull request Jul 17, 2020
Closed
2 tasks
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
@ronag@cjihrig
stream: simpler and faster Readable async iterator
f4a6e6f
Reimplement as an async generator instead of a custom iterator class. PR-URL: #34035 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
@ronag
Copy link
MemberAuthor

ronag commented Aug 1, 2020

@nodejs/tsc @mcollina@benjamingr I defensively marked this as semver-major. However, it would be nice to land this on v14 as it might become the basis for future semver-minor changes.

@mcollinamcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 2, 2020
@mcollina
Copy link
Member

mcollina commented Aug 2, 2020

I concur with @ronag. We might want to land this on v14 before it goes to LTS.

@ronag
Copy link
MemberAuthor

ronag commented Aug 8, 2020

@Trott This didn't make it to the tsc agenda this week? I guess it's because the PR is closed?

@mcollina
Copy link
Member

mcollina commented Aug 8, 2020

@ronag can you please open an issue or a PR so it gets there?

@ronagronag mentioned this pull request Aug 8, 2020
Closed
@ronag
Copy link
MemberAuthor

ronag commented Aug 8, 2020

Done

@ronagronag mentioned this pull request Aug 23, 2020
Closed
4 tasks
@BethGriggsBethGriggs mentioned this pull request Sep 1, 2020
Merged
richardlau pushed a commit that referenced this pull request Sep 7, 2020
@ronag@richardlau
stream: simpler and faster Readable async iterator
6a296a4
Reimplement as an async generator instead of a custom iterator class. Backport-PR-URL: #34887 PR-URL: #34035 Refs: #34680 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
@richardlaurichardlau mentioned this pull request Sep 7, 2020
Merged
4 tasks
richardlau pushed a commit that referenced this pull request Sep 7, 2020
@ronag@richardlau
stream: simpler and faster Readable async iterator
4bb4007
Reimplement as an async generator instead of a custom iterator class. Backport-PR-URL: #34887 PR-URL: #34035 Refs: #34680 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
richardlau added a commit that referenced this pull request Sep 7, 2020
@richardlau
2020-09-08, Version 14.10.0 (Current)
ce5f587
Notable changes: - buffer: also alias BigUInt methods (Anna Henningsen) #34960 - crypto: add randomInt function (Oli Lalonde) #34600 - perf_hooks: add idleTime and event loop util (Trevor Norris) #34938 - stream: simpler and faster Readable async iterator (Robert Nagy) #34035 - stream: save error in state (Robert Nagy) #34103 PR-URL: #35023
richardlau added a commit that referenced this pull request Sep 8, 2020
@richardlau
2020-09-08, Version 14.10.0 (Current)
b5a47ca
Notable changes: - buffer: also alias BigUInt methods (Anna Henningsen) #34960 - crypto: add randomInt function (Oli Lalonde) #34600 - perf_hooks: add idleTime and event loop util (Trevor Norris) #34938 - stream: simpler and faster Readable async iterator (Robert Nagy) #34035 - stream: save error in state (Robert Nagy) #34103 PR-URL: #35023 Conflicts: src/node_version.h
MylesBorins pushed a commit that referenced this pull request Oct 15, 2020
@ronag@MylesBorins
stream: multiple stream backports
573410f
includes: * stream: simpler and faster Readable async iterator * stream: don't destroy on async iterator success * stream: async iterator stop read if destroyed PR-URL: #34887 Refs: #34035 Refs: #35122 Refs: #35640 Refs: #34680 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@richardlau@joesepi
2020-09-08, Version 14.10.0 (Current)
8af5ac6
Notable changes: - buffer: also alias BigUInt methods (Anna Henningsen) nodejs#34960 - crypto: add randomInt function (Oli Lalonde) nodejs#34600 - perf_hooks: add idleTime and event loop util (Trevor Norris) nodejs#34938 - stream: simpler and faster Readable async iterator (Robert Nagy) nodejs#34035 - stream: save error in state (Robert Nagy) nodejs#34103 PR-URL: nodejs#35023 Conflicts: src/node_version.h
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@TrottTrottTrott left review comments

@jasnelljasnelljasnell approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

+2 more reviewers

@devsnekdevsnekdevsnek left review comments

@lundibundilundibundilundibundi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.semver-majorPRs that contain breaking changes and should be released in the next major version.streamIssues and PRs related to the stream subsystem.tsc-agendaIssues and PRs to discuss during the meetings of the TSC.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ronag@nodejs-github-bot@mcollina@benjamingr@jasnell@Trott@devsnek@lundibundi@targos