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.

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 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.15%. Comparing base (fe5037b) to head (23f4783).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #64710   +/-   ##
=======================================
  Coverage   90.14%   90.15%           
=======================================
  Files         741      741           
  Lines      242246   242327   +81     
  Branches    45621    45624    +3     
=======================================
+ Hits       218381   218477   +96     
+ Misses      15370    15347   -23     
- Partials     8495     8503    +8     
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 37 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 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