Skip to content

http2: fix write deadlock exposed by larger window sizes - #65440

Open
pimterry wants to merge 2 commits into
nodejs:mainfrom
pimterry:fix-h2-write-deadlock
Open

http2: fix write deadlock exposed by larger window sizes#65440
pimterry wants to merge 2 commits into
nodejs:mainfrom
pimterry:fix-h2-write-deadlock

Conversation

@pimterry

Copy link
Copy Markdown
Member

This completes the fix from #65079. That PR resolved one test flake (test-worker-terminate-http2-respond-with-file), and reduced the second (test-stream-pipeline-http2) but left an underlying issue there which is still causing flakes.

This took more work, as it's a bit complicated. The issue is a real bug which can result in deadlocks between two Node HTTP/2 peers, not a test issue. This was preexisting though very hard to hit, but is exposed in some cases recently by the new window size update (#64623).

This PR fixes this by dropping a security guard (no reading while writing) completely. I've put it in two commits: the first does the tiny fix to drop the guard & tests it, the second removes various code which is now unreachable without the guard.

Dropping this guard needs careful review, but I think that dropping this guard is safe due to the various other mechanisms in place. As long as we're happy that this is safe, it has a lot of upsides: it fixes the flake, resolves a real deadlock, simply deletes some code, and provides some performance boosts.

An example deadlock flow looks roughly like this:

  • Both peers are reading & writing.
  • Peers write enough to fill the remote peer's TCP receive buffers (only now easily reachable, due to the larger window sizes).
  • While TCP receive buffers are full, both peers try to write at the same time.
  • The security guard here blocks reading while writing, so both peers stop reading.
  • The buffers are all full, so TCP backpressure stops the writes completing - they wait for space.
  • Both peers wait for their writes to complete, which requires a read to empty the buffers, but neither is reading => 🔒

This PR fixes that deadlock, by removing the "no reading while writing" guard completely, in its two forms.

These were added as part of a larger set of many HTTP/2 DoS mitigations in 2019 by @addaleax in #29122, so this needs careful review please!

I do think it's safe though: the key thing it's protecting against (one peer forcing the other to buffer large amounts of outgoing data through manipulation of flow control & window updates) is covered by other existing mechanisms here (most notably maxSessionMemory). I can't find any attacks that depend on this guard alone. I've added an additional test which covers CVE-2019-9517 directly, and confirms that maxSessionMemory blocks the key attack scenario regardless.

Removing this guard has notable performance benefits. @mcollina previously looked at this same issue briefly with #63009 testing small focused tweaks on the same guard. AFAICT tweaks alone had minimal benefit, but the guard is still a real perf problem, because it blocks all reads on the whole H2 session while writing any frames anywhere. That means during e.g. a large read (server large upload/client large download), every sent window update to read more data from any stream pauses reading on all streams until the write completes.

Many different dimensions where removing this entirely should improve things, but I've included one example in the benchmark here: this saturates TLS bidirectional streaming, and with this change you get roughly 25% throughput boost. This is using default settings except it shrinks the stream window size back to the standard H2 default - without that you can't benchmark against previous versions, because they deadlock.

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 <pimterry@gmail.com>
@nodejs-github-bot

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++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Aug 20, 2026
@pimterry
pimterry force-pushed the fix-h2-write-deadlock branch from e2733d2 to a46d64f Compare August 20, 2026 18:18
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.09%. Comparing base (fd5b135) to head (a46d64f).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65440      +/-   ##
==========================================
- Coverage   90.12%   90.09%   -0.03%     
==========================================
  Files         752      752              
  Lines      252325   252280      -45     
  Branches    47456    47435      -21     
==========================================
- Hits       227407   227301     -106     
- Misses      16217    16268      +51     
- Partials     8701     8711      +10     
Files with missing lines Coverage Δ
src/node_http2.cc 81.93% <100.00%> (+0.21%) ⬆️
src/node_http2.h 92.22% <ø> (-0.05%) ⬇️

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

@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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants