Conversation
|
This PR is part of a stack of 9 bookmarks:
Created with jj-stack |
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Wires outgoing WebTransport datagram expiry into HTTP/3 processing and records expiry statistics.
Changes:
- Expire datagram queues during client and server processing.
- Track expired outgoing datagrams in WebTransport stats.
- Add regression coverage and update documentation.
File summaries
| File | Description |
|---|---|
| neqo-http3/src/features/extended_connect/webtransport_session.rs | Updated as part of this pull request. |
| neqo-http3/src/features/extended_connect/tests/webtransport/datagrams.rs | Updated as part of this pull request. |
| neqo-http3/src/features/extended_connect/stats.rs | Updated as part of this pull request. |
| neqo-http3/src/connection_server.rs | Updated as part of this pull request. |
| neqo-http3/src/connection_client.rs | Updated as part of this pull request. |
Review details
Suppressed comments (4)
neqo-http3/src/connection_client.rs:750
- This sweep is inside the active-state arm, so it does not run once the client enters
Http3State::Closing. A peer-initiated close sets the HTTP/3 state toClosingwithout clearing the extended-CONNECT sessions, while the transport'sprocess_timeralso skips datagram expiry during closing; queued stale datagrams can therefore remain until the connection is dropped and are never counted. Run the sweep for every non-closed HTTP/3 tick, as the server path does.
self.base_handler
.expire_datagram_queues(&mut self.conn, now);
neqo-http3/src/connection_client.rs:750
- This activates a shared sweep whose accounting is not actually per session:
Session::expire_datagramscallsconn.expire_datagrams(now), which expires every queue, while this helper invokes it once for each active session. With the multiple-session case already supported bydatagrams_multiple_session, the first session visited consumes both sessions' stale datagrams and records both counts in its ownSessionStats; the other session records zero. Make the transport sweep session-scoped (or return per-session counts) before using this call.
self.base_handler
.expire_datagram_queues(&mut self.conn, now);
neqo-http3/src/connection_server.rs:198
- This sweep is not reached for every server connection at each
Http3Server::process_http3call: the outer server only selectsactive_connections()or handlers whoseshould_be_processed()is true, and that predicate does not include a pending QUIC datagram queue. If a send attempt leaves a datagram queued until its expiry timer, the transport'sConnection::process_timercan remove it before this handler is selected, so the WebTransport expiry statistic stays at zero. Select/sweep connections with a due datagram expiry before transport timer processing.
self.base_handler.expire_datagram_queues(conn, now);
neqo-http3/src/features/extended_connect/webtransport_session.rs:251
- This counter is intended to be per WebTransport session, but the sweep that calls this hook uses
Connection::expire_datagrams, which removes stale entries from every session queue on the connection. With two active WebTransport sessions, whichever session is visited first records all expired datagrams and the other records zero (or the first records datagrams belonging only to the other session). Scope the expiry/counting to the current session, or return per-session counts before updating this field.
fn record_expired_outgoing_datagrams(&mut self, count: u64) {
self.stats.datagrams_expired_outgoing += count;
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Merging this PR will degrade performance by 0.7%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
1f3c5c1 to
40b0996
Compare
d74cf65 to
b3a3b72
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## users/jesup/rename_max_buffered_datagrams #3986 +/- ##
=============================================================================
+ Coverage 96.98% 97.01% +0.03%
=============================================================================
Files 118 118
Lines 41441 41527 +86
Branches 41441 41527 +86
=============================================================================
+ Hits 40190 40287 +97
+ Misses 1227 1216 -11
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more.
|
40b0996 to
95b5b8d
Compare
b3a3b72 to
af3de7e
Compare
|
@jesup I don't think the PR title matches the code you are proposing here. This pull request only adds the reporting, no? |
You're right, and I had already updated this locally - the commit is retitled here to "WebTransport: count a session's expired outgoing datagrams" since the |
95b5b8d to
6a29098
Compare
af3de7e to
579205f
Compare
4ba7feb to
31e9004
Compare
d7b8b23 to
81b9830
Compare
31e9004 to
3a9684b
Compare
|
This pull request now includes #3988, right? If so, can you reply to my comment #3988 (review) ? |
3a9684b to
0fe78e4
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
0fe78e4 to
a0174a1
Compare
a0174a1 to
40e8b3a
Compare
40e8b3a to
8a6e7f8
Compare
8a6e7f8 to
5dfb5fe
Compare
There was a problem hiding this comment.
I found nothing new to report. The one defect I found is already raised in an existing unresolved review thread: a session closed by the peer, left in FinPending, keeps has_pending_datagram_counts() true, so should_be_processed() returns true on every server tick.
larseggert
left a comment
larseggert
left a comment
There was a problem hiding this comment.
Is the PR description accurate? There are a lot of changes here that appear unrelated to counting expired datagrams.
This is still pending. |
Yeah, it turns out some of this is no longer needed, post removal of IDs, etc. A server only exposes session stats through close_session's return value, and close_session already collects any pending expiries. The client sweeps on every process_output(). So the extra server wake-up just made stats read by tests fresher. I removed it here and from the later commits that relied on it. Less code FTW :-) |
the PR had absorbed #3988; that part is gone and the description now lists what's left. I'll add the description from the patch here as well (annoying that the tool doesn't add/sync them) |
the wake-up is gone, and the leftover queue is now dropped on peer close |
Implement Protocol::record_expired_outgoing_datagrams on the WebTransport session, so SessionStats::datagrams_expired_outgoing counts datagrams shed for exceeding outgoingMaxAge; the field existed but stayed zero. The HTTP/3 layer picks the counts up in its per-session sweep, and close_session collects any still pending before returning the final stats. Add Connection::next_datagram_expiry so the sweep can skip scanning the receive streams when nothing is due. A session the peer closes drops its queue as soon as it leaves the active state; a close capsule without a FIN would otherwise leave the queue in place until the FIN arrives. Datagrams still queued when a session closes are dropped, not counted as expired.
5dfb5fe to
1f13246
Compare
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Client/server transfer resultsPerformance differences relative to fdf2b5a. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Uh oh!
There was an error while loading. Please reload this page.