FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: clarify async iterator leak by ronag · Pull Request #28997 · nodejs/node · GitHub

/ node Public

doc: clarify async iterator leak - #28997

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:doc-async-iterator
ConversationCommits (1)ChecksFiles changed
Closed

doc: clarify async iterator leak#28997
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:doc-async-iterator

Conversation

ronag commented Aug 6, 2019

Copy link
Copy Markdown
Member

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

Checklist

nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Aug 6, 2019
ronag force-pushed the doc-async-iterator branch 4 times, most recently from 8a6b51d to 5fb5109 Compare August 6, 2019 09:47
This was referenced Aug 6, 2019
Closed
Closed

ronag commented Aug 6, 2019

Copy link
Copy Markdown
Member Author

@Trott flaky V8 compile, please restart?

nodejs-github-bot commented Aug 7, 2019

Copy link
Copy Markdown
Collaborator

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

Trott reviewed Aug 7, 2019
View reviewed changes
Comment thread doc/api/stream.md Outdated

Trott Aug 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
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 previously approved these changes Aug 7, 2019
View reviewed changes

Trott commented Aug 7, 2019

Copy link
Copy Markdown
Member

@nodejs/streams

Trott dismissed their stale review August 7, 2019 05:03

going to defer to stream folks on this

mcollina requested changes Aug 7, 2019
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Aug 7, 2019

Copy link
Copy Markdown
Member

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 commented Aug 7, 2019

Copy link
Copy Markdown
Member Author

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 force-pushed the doc-async-iterator branch from 5fb5109 to 952d08a Compare August 7, 2019 14:24

ronag commented Aug 7, 2019

Copy link
Copy Markdown
Member Author

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

jasnell commented Aug 7, 2019

Copy link
Copy Markdown
Member

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

mcollina approved these changes Aug 9, 2019
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mcollina commented Aug 9, 2019

Copy link
Copy Markdown
Member

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 mentioned this pull request Aug 9, 2019
Closed
4 tasks
ronag force-pushed the doc-async-iterator branch from 952d08a to e4de3f0 Compare August 9, 2019 16:03

ronag commented Aug 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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 commented Aug 12, 2019

Copy link
Copy Markdown
Member

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

Trott reviewed Aug 12, 2019
View reviewed changes
Comment thread doc/api/stream.md
```

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

Trott Aug 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 12, 2019

Copy link
Copy Markdown
Member

Landed in d7a4ace

Trott closed this Aug 12, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Aug 12, 2019
doc: clarify async iterator leak
d7a4ace 
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 pushed a commit that referenced this pull request Aug 19, 2019
doc: clarify async iterator leak
5ea9237 
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 mentioned this pull request Aug 19, 2019
Merged
This was referenced Aug 20, 2019
Closed
Closed
Closed
Closed
Closed
Closed
ronag mentioned this pull request Apr 22, 2020
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments
mcollina mcollina approved these changes
jasnell jasnell approved these changes
trivikr trivikr approved these changes

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL