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

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

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

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

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.

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
}

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
}

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

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

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

How could it be on the stream?

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

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

Nope, just uncertainty 😅

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

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

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.

@Linkgoron can you add the experimental badge in there?

Added the experimental tag in the method docs

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

Landed in df85d37