stream: add a non-destroying iterator to Readable

[Linkgoron]

Add a non-destroying async iterator to Readable.

fixes: #38491

@nodejs/streams

A few things that I think might need attention:

1. The API itself. Is a new method needed and is the naming OK, or should options be added to Symbol.asyncIterator?
2. Should the new method receive different defaults than Symbol.asyncIterator?
3. 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.

[nodejs-github-bot]

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

[mcollina]

Good work! I've left a few notes.

[aduh95]

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

[mcollina]

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

[targos]

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

[aduh95]

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.

[jasnell]

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.

[Linkgoron]

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]

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

[mcollina]

lgtm

[nodejs-github-bot]

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

[benjamingr]

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]

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]

So here are the two use cases I have:

for await(const chunk of fs.createReadStream('./foo')) { // this should not leak

}
for await(const chunk of someMethodReturningAStream()) { // this should not leak

}

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

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

[Linkgoron]

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

const stream = fs.createReadStream('./someFile');
for await(const chunk of stream) {
  if (isSpecial(chunk)) break;
  processFirst(chunk); // e.g. read http headers
}
for await(const chunk of stream) { // continues from where the last for await ended
  if (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:

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

[mcollina]

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

[benjamingr]

@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]

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

[mcollina]

How could it be on the stream?

[nodejs-github-bot]

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

[Linkgoron]

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

[benjamingr]

Nope, just uncertainty 😅

[benjamingr]

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

[mcollina]

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

[benjamingr]

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]

@Linkgoron can you add the experimental badge in there?

[Linkgoron]

Added the experimental tag in the method docs

[nodejs-github-bot]

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

[jasnell]

Landed in df85d37