doc: clarify async iterator leak

[ronag]

Clarifies that creating multiple async iterators from the same stream can lead to event listener leak.

Checklist

• documentation is changed or added
• commit message follows commit guidelines

[ronag]

@Trott flaky V8 compile, please restart?

[nodejs-github-bot]

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

[Trott]

Suggested change

	streams using async iterators any errors emitted after `'end'` or `'close'`
	streams using async iterators, any errors emitted after `'end'` or `'close'`

[Trott]

@nodejs/streams

going to defer to stream folks on this

[mcollina]

I don't think we should land this change.

When a stream is async iterated it will always exit the iteration in a destroyed state, i.e. break or throw will call destroy() on the stream. As a result, we cannot create multiple streams one after the other.
Moreover, creating multiple parallel AsyncIterators out of the same stream is problematic and should not be done: the behavior is going to be very unpredictable (which of the two iterators will get the data? only one will).

What should be documented is that, because it's left in a destroyed state, there will be a 'error' event handler attached to prevent further exceptions to crash the process.

[ronag]

Isn’t it possible to create multiple iterators? i.e when using the iterator API directly and not through a for loop?

Should we maybe throw if a second iterative is created?

[mcollina]

Isn’t it possible to create multiple iterators? i.e when using the iterator API directly and not through a for loop?

The semantics of that are currently not what somebody is going to expect. The two iterators are going to compete for the chunks (as they use 'readable' / read() internally). An iterator does not use .pipe(), so there is no multiple destination logic in there.

[ronag]

The semantics of that are currently not what somebody is going to expect. The two iterators are going to compete for the chunks (as they use 'readable' / read() internally). An iterator does not use .pipe(), so there is no multiple destination logic in there.

Hence, should we throw if a secondary iterator is created? Also possibly add a note in the docs?

[ronag]

Updated description in accordance with @mcollina's previous suggestion.

[jasnell]

Throwing if there's an attempt to create a second iterator would make sense to me. What do you think @mcollina?

[mcollina]

LGTM

[mcollina]

Throwing if there's an attempt to create a second iterator would make sense to me. What do you think @mcollina?

I'm definitely +1 on that. @ronag, would you like to send a PR for that?

[ronag]

@Trott: This fails because I didn't rebase master. However, since it's a simple doc change I don't think it's worth a rebase?

[Trott]

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3828/

[Trott]

Not sure that what I"m about to suggest is a good idea, so I'll defer to everyone else's judgment:

Is it worth re-wording to make it clear that this means the destroy() method?

Suggested change

	unhandled post-destroy errors.
	unhandled errors after `destroy()` executes.

I'm not sure that's an improvement to be honest, but maybe?

[Trott]

Landed in d7a4ace