Skip to content

quic: fix stop sending behaviour & callback - #64710

Open
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:quic-stop-sending
Open

quic: fix stop sending behaviour & callback#64710
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:quic-stop-sending

Conversation

@pimterry

Copy link
Copy Markdown
Member

The current QUIC STOP_SENDING behaviour is broken because it uses the outbound "we are sending STOP_SENDING" callback and assumes it reports inbound STOP_SENDING frames.

That's not surprising because a callback for inbound STOP_SENDING didn't exist until recently (I've only just added it to ngtcp2: ngtcp2/ngtcp2#2210) meaning until now it was (approximately) impossible to handle this correctly.

End result in the current setup:

  • Remote STOP_SENDING weren't reported correctly (usually reported as a failed write on a closed stream later)
  • Calling stream.stopSending() locally sent the frame to stop inbound (readable) data from the remote peer, but then triggered the callback for a received STOP_SENDING frame.
  • Triggering this callback also sent RESET_STREAM (correct behaviour, if a STOP_SENDING had been received) which effectively stops outbound (writable) data to the remote peer.
  • Combined: calling stopSending() to close one side of the stream actually closed both, and blamed the other peer for it.

This fixes that: wiring up the new callback instead, and consolidating the existing tests to properly cover the correct behaviour.

@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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 24, 2026
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.17%. Comparing base (396aad0) to head (4984bb8).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64710      +/-   ##
==========================================
- Coverage   90.17%   90.17%   -0.01%     
==========================================
  Files         746      746              
  Lines      242776   242839      +63     
  Branches    45741    45770      +29     
==========================================
+ Hits       218929   218972      +43     
- Misses      15347    15351       +4     
- Partials     8500     8516      +16     
Files with missing lines Coverage Δ
lib/internal/quic/quic.js 100.00% <100.00%> (ø)
lib/internal/quic/state.js 100.00% <100.00%> (ø)
lib/internal/quic/symbols.js 100.00% <100.00%> (ø)

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

Signed-off-by: Tim Perry <pimterry@gmail.com>
@pimterry
pimterry force-pushed the quic-stop-sending branch from 23f4783 to 4984bb8 Compare July 31, 2026 09:34
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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants