Skip to content

quic: fix two small bugs in HTTP/3 stream internals - #65970

Open
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:http3-fixes
Open

quic: fix two small bugs in HTTP/3 stream internals#65970
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:http3-fixes

Conversation

@pimterry

Copy link
Copy Markdown
Member

This fixes two subtle bugs in HTTP/3 stream handling, originally implemented deep in #63995 as part of restructuring that touched this code. Much easier to review independently here, honestly I hadn't been through that as carefully as I have have with this PR now, breaking this up is definitely worthwhile.

HTTP/3 internal streams creation failure crashes node

When the application starts, it needs to create 3 unidirectional internal streams (1x control + 2x QPACK). It saved started_ = true before creating these streams. If that failed, this remained. When the session then shuts down it checks started_ and because it's set it tries to tear down those 3 streams, which don't actually exist => 💥

We now set _started to the actual result of startup.

Test here covers that directly, by connecting HTTP/3 with a peer who doesn't allow opening unidirectional streams at all. A remote QUIC peer like this crashes Node today - with this it just kills the session.

HTTP/3 stale stream data reuse

application().GetStreamData(&stream_data) is called in a loop from Session::SendPendingData to read the stream data out of the application. It reuses stream_data between iterations.

Previously the QUIC application reset this before using it (here) but the HTTP/3 application did not (here).

This doesn't actually break anything in practice I think, but mostly by luck. Right now, for example, data->count is set to kMaxVectorCount (16) at the start of the HTTP/3 GetStreamData but if there's no max data credit left then we just return and do nothing (so we've now updated the data length, but not the content or target stream id - which remain stale from the previous iteration). SendPendingData then continues on to call WriteVStream and then ngtcp2_conn_writev_stream with this stream data: length=16 plus the stream id & data from the previous iteration. The only reason it doesn't corrupt streams on the wire is that the broken case is only triggered when there's no data credit left, and so ngtcp2 ignores the write. We're currently doing this happy dance every time we hit the credit limit during this loop. AFAICT this means we basically always do an extra write where we unsuccessfully try to send duplicate stale data after we hit the data limit.

There's other potential stale data flows here as well, e.g. we bail early if nghttp3_conn_writev_stream fails.

This PR fixes the lot by resetting the stream data consistently in the loop itself instead, extracting that from the QUIC application so it applies to HTTP/3 too.

It also simplifies the related HTTP/3 GetStreamData logic: using the fixed value for the count directly instead of writing it to the output struct to pass it across (which seems very odd - looks like this is a complex artifact of historical changes).

Signed-off-by: Tim Perry <pimterry@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@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. labels Sep 10, 2026
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.17%. Comparing base (ffc4203) to head (563ddc9).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65970   +/-   ##
=======================================
  Coverage   90.17%   90.17%           
=======================================
  Files         771      771           
  Lines      265470   265470           
  Branches    50467    50450   -17     
=======================================
+ Hits       239382   239396   +14     
- Misses      17033    17044   +11     
+ Partials     9055     9030   -25     

see 21 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.

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++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants