Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: clarify async iterator leak #28997

Closed
wants to merge 1 commit into from

Conversation

@ronag
Copy link
Contributor

commented Aug 6, 2019

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

Checklist

@ronag ronag force-pushed the nxtedition:doc-async-iterator branch 4 times, most recently from 8a6b51d to 5fb5109 Aug 6, 2019

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@Trott flaky V8 compile, please restart?

@jasnell
jasnell approved these changes Aug 7, 2019
@nodejs-github-bot

This comment has been minimized.

@@ -2499,6 +2499,14 @@ and async iterators are provided below.
})();
```

Async iterators register a permanent error handler. If the iterator is not
fully consumed, any unhandled stream error will be swallowed. When consuming
streams using async iterators any errors emitted after `'end'` or `'close'`

This comment has been minimized.

Copy link
@Trott

Trott Aug 7, 2019

Member
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

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

@Trott Trott dismissed their stale review Aug 7, 2019

going to defer to stream folks on this

@mcollina
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

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 ronag force-pushed the nxtedition:doc-async-iterator branch from 5fb5109 to 952d08a Aug 7, 2019

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

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

@jasnell

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

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

@mcollina
Copy link
Member

left a comment

LGTM

@mcollina

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

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 ronag referenced this pull request Aug 9, 2019
4 of 4 tasks complete

@ronag ronag force-pushed the nxtedition:doc-async-iterator branch from 952d08a to e4de3f0 Aug 9, 2019

@trivikr
trivikr approved these changes Aug 9, 2019
@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@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

This comment has been minimized.

@@ -2502,6 +2502,9 @@ and async iterators are provided below.
})();
```

Async iterators register a permanent error handler on the stream to prevent any
unhandled post-destroy errors.

This comment has been minimized.

Copy link
@Trott

Trott Aug 12, 2019

Member

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

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Landed in d7a4ace

@Trott Trott closed this Aug 12, 2019

Trott added a commit to Trott/io.js that referenced this pull request Aug 12, 2019
doc: clarify async iterator leak
Clarifies that creating multiple async iterators from the same stream
can lead to event listener leak.

PR-URL: nodejs#28997
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos added a commit that referenced this pull request Aug 19, 2019
doc: clarify async iterator leak
Clarifies that creating multiple async iterators from the same stream
can lead to event listener leak.

PR-URL: #28997
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos added a commit that referenced this pull request Aug 19, 2019
doc: clarify async iterator leak
Clarifies that creating multiple async iterators from the same stream
can lead to event listener leak.

PR-URL: #28997
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos referenced this pull request Aug 19, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
doc: clarify async iterator leak
Clarifies that creating multiple async iterators from the same stream
can lead to event listener leak.

PR-URL: nodejs#28997
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
doc: clarify async iterator leak
Clarifies that creating multiple async iterators from the same stream
can lead to event listener leak.

PR-URL: nodejs#28997
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.