Skip to content

[v26.x backport] http2: fix write deadlock exposed by larger window sizes - #65953

Closed
panva wants to merge 2 commits into
nodejs:v26.x-stagingfrom
panva:backport-65440-to-v26.x
Closed

[v26.x backport] http2: fix write deadlock exposed by larger window sizes#65953
panva wants to merge 2 commits into
nodejs:v26.x-stagingfrom
panva:backport-65440-to-v26.x

Conversation

@panva

@panva panva commented Sep 10, 2026

Copy link
Copy Markdown
Member

Backports #65440 with a prerequisite partial backport of #63249 to address the reset-cleanup hang seen in #65881.

The #63249 backport only adds peer-reset tracking and allows reset streams to be destroyed after readable 'end' without waiting for pending writes to finish. It preserves 26.x buffered reads, errors, and compat events, without taking the semver-major behavior changes. The #65440 patch is unchanged but present here so that both can be tested together.

@pimterry @panva
http2: error for incomplete reads on RST, auto-drain, deprecate aborted
f9ff20b 
Signed-off-by: Tim Perry <[email protected]>
PR-URL: nodejs#63249
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Signed-off-by: Filip Skokan <[email protected]>
Assisted-by: Codex
@pimterry @panva
http2: fix write deadlock exposed by larger window sizes
d41cbc0 
This removes a guard (no reads while write pending) that creates
this deadlock, which was added as a security mechanism. This guard is
redundant given then other existing mechanisms, and a test is added to
demonstrate that.

Signed-off-by: Tim Perry <[email protected]>
PR-URL: nodejs#65440
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Signed-off-by: Filip Skokan <[email protected]>
Assisted-by: Codex
@nodejs-github-bot

nodejs-github-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net
  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. v26.x Issues that can be reproduced on v26.x or PRs targeting the v26.x-staging branch. labels Sep 10, 2026
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
mcollina
mcollina approved these changes Sep 10, 2026
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.40%. Comparing base (ca26282) to head (d41cbc0).
⚠️ Report is 421 commits behind head on v26.x-staging.

Files with missing lines Patch % Lines
src/node_http2.cc 94.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@                Coverage Diff                @@
##           v26.x-staging   #65953      +/-   ##
=================================================
+ Coverage          90.24%   90.40%   +0.15%     
=================================================
  Files                729      744      +15     
  Lines             242760   250426    +7666     
  Branches           46044    47770    +1726     
=================================================
+ Hits              219073   226389    +7316     
- Misses             15133    15334     +201     
- Partials            8554     8703     +149
Files with missing lines Coverage Δ
lib/internal/http2/core.js 95.51% <100.00%> (-0.01%) ⬇️
src/node_http2.h 91.90% <100.00%> (+0.04%) ⬆️
src/node_http2.cc 82.10% <94.11%> (+0.23%) ⬆️

... and 162 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.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

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

pimterry
pimterry approved these changes Sep 11, 2026
View reviewed changes

@pimterry pimterry left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code LGTM 👍

Commit messages for the changelog will need fixing when we land, since they reference the breaking changes from #63249 that aren't included here.

@panva panva added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 11, 2026
mcollina
mcollina approved these changes Sep 11, 2026
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

aduh95 pushed a commit that referenced this pull request Sep 15, 2026
@pimterry @aduh95
http2: error for incomplete reads on RST, auto-drain, deprecate aborted
b7e97f6 
Signed-off-by: Tim Perry <[email protected]>
PR-URL: #63249
Backport-PR-URL: #65953
Signed-off-by: Filip Skokan <[email protected]>
Assisted-by: Codex
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tim Perry <[email protected]>
aduh95 pushed a commit that referenced this pull request Sep 15, 2026
@pimterry @aduh95
http2: fix write deadlock exposed by larger window sizes
b4a7cd3 
This removes a guard (no reads while write pending) that creates
this deadlock, which was added as a security mechanism. This guard is
redundant given then other existing mechanisms, and a test is added to
demonstrate that.

Signed-off-by: Tim Perry <[email protected]>
PR-URL: #65440
Backport-PR-URL: #65953
Signed-off-by: Filip Skokan <[email protected]>
Assisted-by: Codex
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tim Perry <[email protected]>
@github-actions github-actions Bot removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 17, 2026
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

This pull request has conflicts with its base branch, removing the author ready label.
Please rebase your branch onto the latest base branch, resolve the conflicts locally, and force-push.
Afterwards the pull request needs a fresh collaborator approval, and a collaborator will add the label back once it is author ready again.

@panva

panva commented Sep 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Landed in 2266c14...b4a7cd3

@panva panva closed this Sep 17, 2026
@panva
panva deleted the backport-65440-to-v26.x branch September 17, 2026 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@pimterry pimterry pimterry approved these changes
@richardlau richardlau richardlau approved these changes
@jasnell jasnell Awaiting requested review from jasnell
@aduh95 aduh95 Awaiting requested review from aduh95

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. v26.x Issues that can be reproduced on v26.x or PRs targeting the v26.x-staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@panva @nodejs-github-bot @mcollina @pimterry @richardlau