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: prevent dead lock when Duplex generator is "thrown" #56287

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matthieusieben
Copy link
Contributor

@matthieusieben matthieusieben commented Dec 17, 2024

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot 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
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: 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 9 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

@jakecastelli
Copy link
Member

jakecastelli commented Dec 17, 2024

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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants