Conversation
jesup
requested review from
KershawChang,
larseggert,
martinthomson and
mxinden
as code owners
September 14, 2026 02:23
This was referenced Sep 14, 2026
Member
Author
|
This PR is part of a stack of 10 bookmarks:
Created with jj-stack |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The implementation is sound, but its comments inaccurately claim transport never expires orphaned datagrams.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Drops and reports queued WebTransport datagrams when a session is removed.
Changes:
- Exposes session queue cleanup internally.
- Invokes cleanup during extended-CONNECT teardown.
- Adds peer-reset regression coverage.
File summaries
| File | Description |
|---|---|
connection.rs |
Cleans queued datagrams during teardown. |
session.rs |
Exposes the cleanup method crate-wide. |
datagrams.rs |
Tests cleanup after peer reset. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## users/jesup/report_datagrams #3992 +/- ##
================================================================
- Coverage 96.95% 96.93% -0.03%
================================================================
Files 119 119
Lines 41579 41579
Branches 41579 41579
================================================================
- Hits 40312 40303 -9
- Misses 1241 1252 +11
+ Partials 26 24 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Contributor
Merging this PR will degrade performance by 4.49%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decode 1000 STREAM frames |
179.6 µs | 226.5 µs | -20.68% |
| ❌ | Simulation | decode 100 STREAM frames |
19.3 µs | 24 µs | -19.9% |
| ❌ | Simulation | decode 10 STREAM frames |
3.4 µs | 4 µs | -14.57% |
| ❌ | Simulation | coalesce_acked_from_zero 1 ranges |
2.7 µs | 2.8 µs | -5.79% |
| ❌ | Simulation | coalesce_acked_from_zero 3 ranges |
3.6 µs | 3.7 µs | -4.6% |
| ❌ | Simulation | Builder encode+encrypt ACK packet |
21.9 µs | 22.7 µs | -3.26% |
| ❌ | Simulation | Builder encode+encrypt STREAM packet |
27.9 µs | 28.7 µs | -2.58% |
| ❌ | WallTime | neqo-quiche |
38.9 ms | 39.8 ms | -2.35% |
| ❌ | Simulation | coalesce_acked_from_zero 10 ranges |
9.7 µs | 9.9 µs | -2.35% |
| ❌ | WallTime | walltime/pacing-false/same-seed |
2 ms | 2 ms | -2.32% |
| ❌ | Simulation | simulated/pacing-false/varying-seeds |
72.7 ms | 74.4 ms | -2.23% |
| ⚡ | Simulation | mark_sent sequential |
234.1 µs | 210.7 µs | +11.14% |
| ⚡ | Simulation | mark_sent retransmit |
33.4 µs | 32.3 µs | +3.44% |
| ⚡ | WallTime | neqo-s2n |
50 ms | 48.9 ms | +2.13% |
| ⚡ | Simulation | simulated/10-streams/each-1048576-bytes |
516.5 ms | 506.1 ms | +2.06% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing users/jesup/drop_datagrams_on_teardown (dc976e1) with users/jesup/report_datagrams (266d6d3)1
Footnotes
jesup
force-pushed
the
users/jesup/report_datagrams
branch
from
September 14, 2026 05:31
51d14be to
fe2247d
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
from
September 14, 2026 05:31
0436765 to
6d838db
Compare
jesup
force-pushed
the
users/jesup/report_datagrams
branch
from
September 14, 2026 17:56
fe2247d to
c696957
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
from
September 14, 2026 17:56
6d838db to
e919805
Compare
mxinden
reviewed
Sep 15, 2026
mxinden
reviewed
Sep 15, 2026
jesup
force-pushed
the
users/jesup/report_datagrams
branch
from
September 16, 2026 03:45
c696957 to
d10a0cd
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
from
September 16, 2026 03:45
e919805 to
6c8995d
Compare
This was referenced Sep 20, 2026
jesup
force-pushed
the
users/jesup/report_datagrams
branch
from
September 23, 2026 06:27
dcba568 to
19fa640
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
from
September 23, 2026 06:27
c0eec0c to
609dbad
Compare
Contributor
Performance profiles for profiler.firefox.comBenchmarks (14)
|
jesup
force-pushed
the
users/jesup/report_datagrams
branch
from
September 23, 2026 23:55
19fa640 to
924e577
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
from
September 23, 2026 23:55
609dbad to
f3f6e7c
Compare
jesup
force-pushed
the
users/jesup/report_datagrams
branch
from
September 25, 2026 10:28
924e577 to
4c03816
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
from
September 25, 2026 10:28
f3f6e7c to
b3f7e86
Compare
jesup
force-pushed
the
users/jesup/report_datagrams
branch
from
September 25, 2026 12:35
4c03816 to
c49ebd3
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
from
September 25, 2026 12:35
b3f7e86 to
fad8dba
Compare
jesup
force-pushed
the
users/jesup/report_datagrams
branch
from
September 25, 2026 15:05
c49ebd3 to
540e2cf
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
2 times, most recently
from
September 30, 2026 03:30
37fdffd to
5f27707
Compare
jesup
force-pushed
the
users/jesup/report_datagrams
branch
2 times, most recently
from
September 30, 2026 15:32
266d6d3 to
df27015
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
from
September 30, 2026 15:32
5f27707 to
0415ed4
Compare
larseggert
approved these changes
Sep 30, 2026
mxinden
reviewed
Oct 1, 2026
mxinden
left a comment
mxinden
left a comment
Member
There was a problem hiding this comment.
Pull request name needs updating. Queued datagrams have been dropped before. This pull request just updates stats and emits events through wt.borrow_mut().drop_queued_datagrams.
Comment on lines
+303
to
+311
| fn next_dgram(server: &mut Http3Server, mut t: Instant) -> (Datagram, Instant) { | ||
| loop { | ||
| match server.process_output(t) { | ||
| Output::Datagram(d) => return (d, t), | ||
| Output::Callback(delay) => t += delay, | ||
| Output::None => panic!("the server had nothing to send"), | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Nit: Either don't take a mut t, or don't return an Instant. Preference for the former. Doing both makes the function signature unintuitive in my eyes.
A session reset by the peer leaves the stream maps via remove_extended_connect, which is the only teardown path that has both the session and the Connection the queue lives on. Drop the queue there so every path counts what was still queued as dropped and removes the per-session entry, including one re-created by a max-buffered or max-age change after the session had already closed.
jesup
force-pushed
the
users/jesup/report_datagrams
branch
from
October 2, 2026 21:48
df27015 to
dbd0f61
Compare
jesup
force-pushed
the
users/jesup/drop_datagrams_on_teardown
branch
from
October 2, 2026 21:48
0415ed4 to
dc976e1
Compare
Contributor
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
|
Contributor
Client/server transfer resultsPerformance differences relative to dbd0f61. 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 |
Contributor
Benchmark resultsNo significant performance differences relative to dbd0f61. All resultstransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1500 time: [50.107 ms 50.175 ms 50.285 ms]
thrpt: [1.9421 GiB/s 1.9463 GiB/s 1.9490 GiB/s]
Found 7 outliers among 100 measurements (7.00%)
6 (6.00%) high mild
1 (1.00%) high severetransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1500 time: [49.533 ms 49.581 ms 49.629 ms]
thrpt: [1.9677 GiB/s 1.9696 GiB/s 1.9716 GiB/s]transfer/1-conn/1-1b-resp (aka. HPS)/mtu-1500 time: [3.0056 ms 3.0067 ms 3.0078 ms]
thrpt: [332.47 B/s 332.59 B/s 332.71 B/s]
Found 6 outliers among 100 measurements (6.00%)
4 (4.00%) high mild
2 (2.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1500 time: [66.258 ms 66.520 ms 66.790 ms]
thrpt: [149.72 Kelem/s 150.33 Kelem/s 150.93 Kelem/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams-flow-controlled/walltime/1-streams/each-4194304-bytes time: [10.638 ms 10.640 ms 10.643 ms]
thrpt: [375.85 MiB/s 375.92 MiB/s 376.00 MiB/s]streams-flow-controlled/walltime/10-streams/each-1048576-bytes time: [27.788 ms 27.799 ms 27.810 ms]
thrpt: [359.58 MiB/s 359.73 MiB/s 359.87 MiB/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams/walltime/1-streams/each-1000-bytes time: [133.08 µs 133.63 µs 134.34 µs]
thrpt: [7.0988 MiB/s 7.1367 MiB/s 7.1664 MiB/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams/walltime/1000-streams/each-1-bytes time: [3.7764 ms 3.7893 ms 3.8017 ms]
thrpt: [256.88 KiB/s 257.72 KiB/s 258.60 KiB/s]streams/walltime/1000-streams/each-1000-bytes time: [11.061 ms 11.064 ms 11.068 ms]
thrpt: [86.165 MiB/s 86.193 MiB/s 86.220 MiB/s]
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) low mild
1 (1.00%) high mildtransfer/walltime/pacing-false/same-seed time: [2.0704 ms 2.0713 ms 2.0724 ms]
thrpt: [1.8849 GiB/s 1.8859 GiB/s 1.8867 GiB/s]transfer/walltime/pacing-false/varying-seeds time: [2.0070 ms 2.0079 ms 2.0091 ms]
thrpt: [1.9443 GiB/s 1.9454 GiB/s 1.9463 GiB/s]transfer/walltime/pacing-true/same-seed time: [2.0434 ms 2.0445 ms 2.0458 ms]
thrpt: [1.9094 GiB/s 1.9106 GiB/s 1.9116 GiB/s]transfer/walltime/pacing-true/varying-seeds time: [2.0817 ms 2.0828 ms 2.0842 ms]
thrpt: [1.8742 GiB/s 1.8755 GiB/s 1.8765 GiB/s]Instructions per cycleCriterion reported no significant timing changes. All benchmarks
Profiles for profiler.firefox.com (62)
Download data for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.