stream: add a non-destroying iterator to Readable#38526

Linkgoron · 2021-05-04T00:01:40Z

Add a non-destroying async iterator to Readable.

fixes: #38491

@nodejs/streams

A few things that I think might need attention:

The API itself. Is a new method needed and is the naming OK, or should options be added to Symbol.asyncIterator?
Should the new method receive different defaults than Symbol.asyncIterator?
Maybe unrelated to this PR, should the createAsyncIterator method remove the listeners that it adds after the iteration ends? Currently it does not, but this was done when it essentially always destroyed the stream. Now it does not (although, it would be a bit problematic with Error, as it emits an error next-tick if an error was thrown). I'm not sure if it's that bad, as the listeners are mostly noops anyway.

add a non-destroying iterator to Readable fixes: nodejs#38491

nodejs-github-bot · 2021-05-04T06:53:45Z

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

mcollina

Good work! I've left a few notes.

lib/internal/streams/readable.js

aduh95 · 2021-05-04T08:00:07Z

doc/api/stream.md

 has less then 64KB of data because no `highWaterMark` option is provided to
 [`fs.createReadStream()`][].

+##### `readable.iterator([options])`


Just a thought: does this have to be in a new method rather than adding a parameter to the existing Symbol.asyncIterator one?

I'm -0 on naming. I prefer a new method as the Symbol.asyncIterator one has a predefined signature by the standard.

It doesn't feel right to me if the user has to call [Symbol.asyncIterator]() themselves.

the Symbol.asyncIterator one has a predefined signature by the standard.
Not really, all the standard says it this method is called with no argument (https://tc39.es/ecma262/#sec-getiterator). The standard gives a clear rule for the returned object (https://tc39.es/ecma262/#sec-asynciterable-interface), but not for the function signature. I personally don't feel strongly either way.

I'd prefer the separate method. The key issue with extending standard-defined APIs is that it makes reasoning about the portability of code far more complicated. A separate method makes it clear. That said, the behavior of the two can be identical such that [Symbol.asyncIterator]() could just defer to readable.iterator() with default arguments.

That said, the behavior of the two can be identical such that [Symbol.asyncIterator]() could just defer to readable.iterator() with default arguments.
Yeah, the reason that one doesn't call the other is because of legacy streams and this. I'd need to use ReflectApply to bind this, and I preferred to have a regular method and send this as the first parameter instead of primordials.

nodejs-github-bot · 2021-05-04T19:21:06Z

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

mcollina

lgtm

nodejs-github-bot · 2021-05-06T12:45:36Z

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

doc/api/stream.md

benjamingr · 2021-05-06T14:00:04Z

I'm a bit torn about this because I think there is a good chance we picked the wrong default for return on streams. I think pretty often people will need non-destructive iterators and for those cases having to write stream.iterator({destroyOnReturn: false, destoryOnError:false }) isn't super ergonomic

mcollina · 2021-05-06T14:03:38Z

I think the defaults are currently sound as the developer would need to do something to destroy the stream manually. The current defaults are safe.

benjamingr · 2021-05-06T14:22:36Z

So here are the two use cases I have:

forawait(constchunkoffs.createReadStream('./foo')){// this should not leak}forawait(constchunkofsomeMethodReturningAStream()){// this should not leak}

On the other hand, I want the following to also work:

conststream=fs.createReadSream('./someFile');forawait(constchunkofstream){if(isSpecial(chunk))break;processFirst(chunk);// e.g. read http headers}forawait(constchunkofstream){// continues from where the last for await endedif(isOtherSpecial(chunk))break;processSecond(chunk);// e.g. read http body}

Linkgoron · 2021-05-06T14:31:12Z

So here are the two use cases I have:
On the other hand, I want the following to also work:

conststream=fs.createReadStream('./someFile');forawait(constchunkofstream){if(isSpecial(chunk))break;processFirst(chunk);// e.g. read http headers}forawait(constchunkofstream){// continues from where the last for await endedif(isOtherSpecial(chunk))break;processSecond(chunk);// e.g. read http body}

One option would be to add a setIterationMode on Readable, where you could set the default type of iterator that Symbol.asyncIterator would return. Other options include stuff like having another alias for nonDestructive iterators (not a fan of having tons of aliases for the same method) or having iterator return with different defaults (which I don't think has much support here).

setIterationMode would look something like this:

conststream=fs.createReadStream('./someFile');stream.setIterationMode({destroyOnReturn: false,destoryOnError:false});forawait(constchunkofstream){if(isSpecial(chunk))break;processFirst(chunk);// e.g. read http headers}forawait(constchunkofstream){// continues from where the last for await endedif(isOtherSpecial(chunk))break;processSecond(chunk);// e.g. read http body}

mcollina · 2021-05-06T15:34:34Z

@benjamingr #38526 (comment) requirements are mutually exclusive.

benjamingr · 2021-05-07T12:56:59Z

@benjamingr #38526 (comment) requirements are mutually exclusive.

That's sort of my point - it's the most concise way I could describe the problem. I'm not sure .iterator is the best way to deal with it. I'm wondering if this should be on the stream rather than the iterator.

It's possible that this is the best we can do - I just think it's a difficult problem and I want to make sure we're not exploring too few options here.

nodejs-github-bot · 2021-05-07T13:46:46Z

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

mcollina · 2021-05-07T14:37:16Z

How could it be on the stream?

nodejs-github-bot · 2021-05-12T15:45:08Z

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

Linkgoron · 2021-05-12T15:46:20Z

@benjamingr Do you have any outstanding objections to this getting merged?

benjamingr · 2021-05-19T09:06:44Z

Nope, just uncertainty 😅

benjamingr · 2021-05-19T09:07:05Z

I'd be more comfortable if this was experimental but I'm fine with this landing as stable.

mcollina · 2021-05-19T10:48:47Z

I'd be happy to land it doc-experimental if you prefer @benjamingr? No warnings and we do not backport.

benjamingr · 2021-05-19T12:08:57Z

I'd be happy to land it doc-experimental if you prefer @benjamingr?

doc-experimental is good to me. If others feel strongly that this is the right API I'd also happily concede.

mcollina · 2021-05-19T12:25:41Z

@Linkgoron can you add the experimental badge in there?

…Readable

Linkgoron · 2021-05-22T21:20:18Z

Added the experimental tag in the method docs

nodejs-github-bot · 2021-05-24T22:29:58Z

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

jasnell · 2021-05-25T14:22:47Z

Landed in df85d37

add a non-destroying iterator to Readable fixes: #38491 PR-URL: #38526Fixes: #38491 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

stream: add a non-destroying iterator to Readable
e21ad8d
add a non-destroying iterator to Readable fixes: nodejs#38491

github-actionsbot added the needs-ci PRs that need a full CI run. label May 4, 2021

fixup! stream: add a non-destroying iterator to Readable
87f5316

Linkgoron added the stream Issues and PRs related to the stream subsystem. label May 4, 2021

mcollina reviewed May 4, 2021
View reviewed changes

lib/internal/streams/readable.js Outdated Show resolvedHide resolved
lib/internal/streams/readable.js Outdated Show resolvedHide resolved

aduh95 reviewed May 4, 2021
View reviewed changes

fixup! fixup! stream: add a non-destroying iterator to Readable
468f9ea

jasnell approved these changes May 4, 2021
View reviewed changes

mcollina approved these changes May 4, 2021
View reviewed changes

benjamingr reviewed May 6, 2021
View reviewed changes

doc/api/stream.md Outdated Show resolvedHide resolved

fixup! fixup! fixup! stream: add a non-destroying iterator to Readable
4bb108b

ronag approved these changes May 19, 2021
View reviewed changes

benjamingr approved these changes May 19, 2021
View reviewed changes

mcollina added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 19, 2021

fixup! fixup! fixup! fixup! stream: add a non-destroying iterator to …
f939af2
…Readable

This comment has been minimized.
Sign in to view

Linkgoron added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 25, 2021

jasnell closed this May 25, 2021

danielleadams mentioned this pull request May 31, 2021
v16.3.0 proposal #38874
Merged

targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022

Slayer95 mentioned this pull request Dec 20, 2022
fs: allow FileHandle to outlive ReadableStream #45917
Closed

Uh oh!

stream: add a non-destroying iterator to Readable#38526

stream: add a non-destroying iterator to Readable #38526

Uh oh!

Conversation

Linkgoron commented May 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented May 4, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aduh95May 4, 2021

Choose a reason for hiding this comment

Uh oh!

mcollinaMay 4, 2021

Choose a reason for hiding this comment

Uh oh!

targosMay 4, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95May 4, 2021

Choose a reason for hiding this comment

Uh oh!

jasnellMay 4, 2021

Choose a reason for hiding this comment

Uh oh!

LinkgoronMay 4, 2021

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 4, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 6, 2021

Uh oh!

Uh oh!

benjamingr commented May 6, 2021

Uh oh!

mcollina commented May 6, 2021

Uh oh!

benjamingr commented May 6, 2021

Uh oh!

Linkgoron commented May 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented May 6, 2021

Uh oh!

benjamingr commented May 7, 2021

Uh oh!

nodejs-github-bot commented May 7, 2021

Uh oh!

mcollina commented May 7, 2021

Uh oh!

nodejs-github-bot commented May 12, 2021

Uh oh!

Linkgoron commented May 12, 2021

Uh oh!

benjamingr commented May 19, 2021

Uh oh!

benjamingr commented May 19, 2021

Uh oh!

mcollina commented May 19, 2021

Uh oh!

benjamingr commented May 19, 2021

Uh oh!

mcollina commented May 19, 2021

Uh oh!

Linkgoron commented May 22, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 24, 2021• edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 25, 2021

Linkgoron commented May 4, 2021•
edited
Loading

Linkgoron commented May 6, 2021•
edited
Loading

nodejs-github-bot commented May 24, 2021•
edited by jasnell
Loading