-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
stream: don't destroy on async iterator success #35122
Conversation
@richardlau can you give this a try? |
As you probably now, this needs a test. |
Yes, building... needs another 20 min or so 😄 |
|
@nodejs/streams |
@richardlau Sorry can you start another one. Turns out the first version of this broke a test. |
The job is still queued and hasn't started yet. I specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
in addition to the time outs there now appear to be two unhandled rejections that weren't seen in an equivalent run against |
I will dig some more tonight. |
Also if you want to run CITGM locally this is what I do:
|
@nodejs/streams this needs another review |
@nodejs/streams |
@nodejs/tsc Tomorrow is the cutoff for 15 and I believe it is important that this lands in 15. |
Destroying on async iterator completion ignores autoDestroy.
rebased to fix conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Landed in 2b9003b |
Destroying on async iterator completion ignores autoDestroy. PR-URL: #35122 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@ronag this doesn't land cleanly on v14.x, should we backport? |
@MylesBorins there is already a different WIP PR for that. |
includes: * stream: simpler and faster Readable async iterator * stream: don't destroy on async iterator success * stream: async iterator stop read if destroyed PR-URL: #34887 Refs: #34035 Refs: #35122 Refs: #35640 Refs: #34680 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Destroying on async iterator completion ignores autoDestroy. PR-URL: nodejs#35122 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Destroying on async iterator completion ignores autoDestroy.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes