-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: fix deadlock when pipeing to full sink #48691
Conversation
Review requested:
|
07ade06
to
f11c61f
Compare
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
@ronag could you fix the commit message? |
This comment was marked as resolved.
This comment was marked as resolved.
When piping a paused Readable to a full Writable we didn't register a drain listener which cause the src to never resume. Refs: nodejs#48666 PR-URL: nodejs#48691
Commit Queue failed- Loading data for nodejs/node/pull/48691 ✔ Done loading data for nodejs/node/pull/48691 ----------------------------------- PR info ------------------------------------ Title stream: fix deadlock when pipeing to full sink (#48691) Author Robert Nagy (@ronag) Branch ronag:fix-pipe-deadlock -> nodejs:main Labels stream, needs-ci Commits 1 - stream: fix deadlock when pipeing to full sink Committers 1 - Robert Nagy PR-URL: https://github.com/nodejs/node/pull/48691 Refs: https://github.com/nodejs/node/issues/48666 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/48691 Refs: https://github.com/nodejs/node/issues/48666 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - stream: fix deadlock when pipeing to full sink ℹ This PR was created on Fri, 07 Jul 2023 12:08:43 GMT ✔ Approvals: 3 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/48691#pullrequestreview-1518738423 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/48691#pullrequestreview-1518877825 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/48691#pullrequestreview-1520765659 ✘ Last GitHub CI failed ℹ Last Full PR CI on 2023-07-10T08:08:41Z: https://ci.nodejs.org/job/node-test-pull-request/52681/ - Querying data for job/node-test-pull-request/52681/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/5512507137 |
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 b34c5a2 |
When piping a paused Readable to a full Writable we didn't register a drain listener which cause the src to never resume. Refs: #48666 PR-URL: #48691 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Any plans to backport? Technically, the exact same patch should apply cleanly. |
When piping a paused Readable to a full Writable we didn't register a drain listener which cause the src to never resume. Refs: nodejs#48666 PR-URL: nodejs#48691 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
When piping a paused Readable to a full Writable we didn't register a drain listener which cause the src to never resume. Refs: nodejs#48666 PR-URL: nodejs#48691 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Can we at least backport this to v18? |
When piping a paused Readable to a full Writable we didn't register a drain listener which cause the src to never resume. Refs: nodejs#48666 PR-URL: nodejs#48691 Backport-PR-URL: nodejs#49323 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
When piping a paused Readable to a full Writable we didn't register a drain listener which cause the src to never resume. Refs: #48666 PR-URL: #48691 Backport-PR-URL: #49323 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
When piping a paused Readable to a full Writable we didn't register a drain listener which cause the src to never resume. Refs: #48666 PR-URL: #48691 Backport-PR-URL: #49323 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
When piping a paused Readable to a full Writable we didn't register a drain listener which cause the src to never resume.
Refs: #48666