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

stream: fix deadlock when pipeing to full sink #48691

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 7, 2023

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

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Jul 7, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 7, 2023
@ronag ronag requested review from mcollina and lpinca July 7, 2023 12:09
@ronag ronag force-pushed the fix-pipe-deadlock branch 2 times, most recently from 07ade06 to f11c61f Compare July 7, 2023 12:12
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

mcollina commented Jul 9, 2023

@ronag could you fix the commit message?

@ronag

This comment was marked as resolved.

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2023
@nodejs-github-bot
Copy link
Collaborator

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
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2023
@nodejs-github-bot
Copy link
Collaborator

@ronag ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 10, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 10, 2023
@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 10, 2023
@nodejs-github-bot
Copy link
Collaborator

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/.ncu
https://github.com/nodejs/node/actions/runs/5512507137

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jul 12, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 12, 2023
@nodejs-github-bot nodejs-github-bot merged commit b34c5a2 into nodejs:main Jul 12, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in b34c5a2

juanarbol pushed a commit that referenced this pull request Jul 13, 2023
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>
@juanarbol juanarbol mentioned this pull request Jul 13, 2023
@kanongil
Copy link
Contributor

kanongil commented Jul 31, 2023

Any plans to backport? Technically, the exact same patch should apply cleanly.

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
@kanongil
Copy link
Contributor

Can we at least backport this to v18?

kanongil pushed a commit to kanongil/node-1 that referenced this pull request Aug 25, 2023
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>
ruyadorno pushed a commit that referenced this pull request Sep 8, 2023
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>
@ruyadorno ruyadorno mentioned this pull request Sep 8, 2023
@ruyadorno ruyadorno added the backported-to-v18.x PRs backported to the v18.x-staging branch. label Sep 9, 2023
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants