stream: prevent dead lock when Duplex generator is "thrown" by matthieusieben · Pull Request #56287 · nodejs/node · GitHub

Fixes regression introduced in #55096 and outlined in #56278.

When using a for await loop, v8 (?) might call the .throw function of the iterable instead of the stream handlers (write, final, destroy). This can cause a dead lock as this line waits for a promise that will never be resolved (since the promise is setteled from the stream handlers).

This PR adresses that issue by making sure the promise is resolved whenever the generator's return and throw function are called, preventing the dead lock.

cc @jakecastelli

Review requested:

@nodejs/streams

Codecov Report

❌ Patch coverage is 83.33333% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.54%. Comparing base (5ad2ca9) to head (361bc6c).
⚠️ Report is 4355 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/streams/duplexify.js	83.33%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56287      +/-   ##
==========================================
- Coverage   88.54%   88.54%   -0.01%     
==========================================
  Files         657      657              
  Lines      190285   190311      +26     
  Branches    36539    36540       +1     
==========================================
+ Hits       168490   168509      +19     
- Misses      14974    14984      +10     
+ Partials     6821     6818       -3

Files with missing lines	Coverage Δ
lib/internal/streams/duplexify.js	96.00% <83.33%> (-1.17%)	⬇️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

I believe #55096 could potentially cause a massive breakage in the eco-system - as I found out thru some of the user land applications / packages on nightly build. I'd suggest we revert 55096 and open another PR along with the fix in this PR and run a CITGM, so we can be more assured.

Update: actually Robert has added the don't land labels on the PR

cc. @nodejs/streams

@jakecastelli @matthieusieben is this PR solves the breakage introduced?

I agree with @jakecastelli, a quick revert is better.

Is this PR solves the breakage introduced?

At least it passes the test that I found thru the broken user land application. There are a few potential cases I will need to look into during holidays.

lgtm

Commit Queue failed

- Loading data for nodejs/node/pull/56287
✔  Done loading data for nodejs/node/pull/56287
----------------------------------- PR info ------------------------------------
Title      stream: prevent dead lock when Duplex generator is "thrown" (#56287)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     matthieusieben:fix-56278 -> nodejs:main
Labels     stream, needs-ci, needs-citgm
Commits    1
 - stream: prevent dead lock when Duplex generator is "thrown"
Committers 1
 - Matthieu Sieben <matthieu.sieben@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/56287
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56287
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 17 Dec 2024 12:57:50 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/56287#pullrequestreview-2521954662
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56287#pullrequestreview-2524628246
   ⚠  This PR has conflicts that must be resolved
   ✔  Last GitHub CI successful
   ✘  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/12527396373

Can you resolve the conflicts?

The resolution LGTM, but I am explicitly requesting changes as this PR has not yet run CI and CITGM. I will follow up once the merge conflict is resolved.

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

lgtm

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

Do you mind resolving the conflicts? so we can push this work forward

Hello,

I'm sorry but I don't have much bandwidth at the moment. I do still want this in so I'll try to find some time in the near future.

Hello, I'm doing some backlog hygiene on all PRs related to networking and streaming modules. There hasn't been any activity here for quite a while so I'm going to mark it stalled. It will auto-close in ~30 days. This is not a rejection; if you've like to pick it back up, rebase onto latest main and give me a ping.

Thank you!

Side note: this looked to be very close to making it over the finish line before. Just need to resolve the merge conflicts and I think this is a valuable contribution.

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Dec 17, 2024

matthieusieben mentioned this pull request Dec 17, 2024

Revert 5541300 and add coverage to pipeline #56278

Closed

stream: prevent dead lock when Duplex generator is "thrown"

361bc6c

matthieusieben force-pushed the fix-56278 branch from d8d8b42 to 361bc6c Compare December 17, 2024 13:17

jakecastelli added needs-citgm PRs that need a CITGM CI run. blocked PRs that are blocked by other issues or PRs. labels Dec 18, 2024

mcollina approved these changes Dec 24, 2024

View reviewed changes

jakecastelli removed the blocked PRs that are blocked by other issues or PRs. label Dec 24, 2024

jasnell approved these changes Dec 28, 2024

View reviewed changes

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 28, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 28, 2024

jakecastelli requested changes Dec 28, 2024

View reviewed changes

jakecastelli removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 28, 2024

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2024

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2024

mcollina approved these changes Dec 30, 2024

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2024

jakecastelli reviewed Jan 8, 2025

View reviewed changes

Ethan-Arrowood added the stalled Issues and PRs that are stalled. label Jul 15, 2026

github-actions Bot removed the stalled Issues and PRs that are stalled. label Jul 16, 2026

stream: prevent dead lock when Duplex generator is "thrown" by matthieusieben · Pull Request #56287 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: prevent dead lock when Duplex generator is "thrown" - #56287

stream: prevent dead lock when Duplex generator is "thrown"#56287matthieusieben wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom matthieusieben:fix-56278matthieusieben/node:fix-56278Copy head branch name to clipboard

Conversation

matthieusieben commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 17, 2024

Uh oh!

codecov Bot commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jakecastelli commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Dec 23, 2024

Uh oh!

jakecastelli commented Dec 23, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 28, 2024

Uh oh!

mcollina commented Dec 28, 2024

Uh oh!

jakecastelli left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 28, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 30, 2024

Uh oh!

jakecastelli left a comment

Choose a reason for hiding this comment

Uh oh!

matthieusieben commented Jan 14, 2025

Uh oh!

Ethan-Arrowood commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stream: prevent dead lock when Duplex generator is "thrown"#56287
matthieusieben wants to merge 1 commit into
nodejs:mainfrom
matthieusieben:fix-56278

matthieusieben commented Dec 17, 2024 •
edited
Loading

codecov Bot commented Dec 17, 2024 •
edited
Loading

jakecastelli commented Dec 17, 2024 •
edited
Loading

Ethan-Arrowood commented Jul 15, 2026 •
edited
Loading