Skip to content

stream: simpler Readable async iterator - #34035

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:readable-async-iterator
Closed

stream: simpler Readable async iterator#34035
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:readable-async-iterator

Conversation

@ronag

@ronagronag commented Jun 23, 2020•
edited
Loading

Copy link
Copy Markdown
Member

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
@ronag
ronagforce-pushed the readable-async-iterator branch 2 times, most recently from a754d9b to bdded9eCompareJune 23, 2020 21:12
@nodejs-github-bot

nodejs-github-bot commented Jun 23, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag

ronag commented Jun 23, 2020

Copy link
Copy Markdown
MemberAuthor

Might be relevant for #30298

ronag
ronag commented Jun 23, 2020
View reviewed changes
Comment threadtest/parallel/test-stream-readable-async-iterators.js Outdated
ronag
ronag commented Jun 23, 2020
View reviewed changes
Comment threadlib/_stream_readable.js Outdated
@ronag

ronag commented Jun 23, 2020

Copy link
Copy Markdown
MemberAuthor

Some really strange CI failures... will investigate

This was referenced Jun 24, 2020
Open
Open
benjamingr
benjamingr reviewed Jun 25, 2020
View reviewed changes
Comment threadlib/_stream_readable.js Outdated
@ronag
ronagforce-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
@ronag
ronagforce-pushed the readable-async-iterator branch 5 times, most recently from a111d8d to 8944c59CompareJune 25, 2020 13:37
@ronag
ronag requested a review from mcollinaJune 25, 2020 13:42
@ronag

ronag commented Jun 25, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@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

@jasnell

jasnell commented Jul 16, 2020

Copy link
Copy Markdown
Member

I'm a bit unsure why this is semver-major. If it definitely needs to be a major, then a changelog note should be added to the documentation briefly describing the change in behavior.

@ronag

ronag commented Jul 17, 2020

Copy link
Copy Markdown
MemberAuthor

It’s defensively marked major. It changes behavior of iterators after errors very subtly and I believe it’s unlikely to affect anyone.

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 <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ronag

ronag commented Jul 17, 2020

Copy link
Copy Markdown
MemberAuthor

Landed in 08e8997

@ronagronag closed this Jul 17, 2020
Trott
Trott reviewed Jul 17, 2020
View reviewed changes
Comment threadbenchmark/streams/readable-async-iterator.js
@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 <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ronag

ronag commented Aug 1, 2020

Copy link
Copy Markdown
MemberAuthor

@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

mcollina commented Aug 2, 2020

Copy link
Copy Markdown
Member

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

@ronag

ronag commented Aug 8, 2020

Copy link
Copy Markdown
MemberAuthor

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

@mcollina

mcollina commented Aug 8, 2020

Copy link
Copy Markdown
Member

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

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

ronag commented Aug 8, 2020

Copy link
Copy Markdown
MemberAuthor

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 <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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 <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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