Open
Conversation
…grams per the WebTransport spec
… can't be retried
jesup
requested review from
KershawChang,
larseggert and
martinthomson
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements support for WebTransport send groups and related features to improve stream prioritization and session management. The changes enable inter-group bandwidth fairness while maintaining sendorder-based prioritization within groups.
Changes:
- Added send group infrastructure for organizing WebTransport streams with fair bandwidth allocation across groups
- Implemented TLS keying material export functionality for application-level security protocols
- Enhanced session management with draining state notification, protocol negotiation, and statistics tracking
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| neqo-transport/src/streams.rs | Core send group scheduling with round-robin fairness and new remote max streams setters |
| neqo-transport/src/send_stream.rs | Send group storage, accessors, and integration with existing stream ordering |
| neqo-transport/src/connection/tests/stream.rs | Test coverage for send group scheduling behavior |
| neqo-transport/src/connection/mod.rs | Connection-level send group API, keying material export, and PMTUD improvements |
| neqo-http3/src/server_events.rs | Return type fixes for datagram sending methods |
| neqo-http3/src/send_message.rs | Added as_any_mut for downcasting support |
| neqo-http3/src/lib.rs | Stream trait extensions for send groups and session protocol |
| neqo-http3/src/features/extended_connect/webtransport_streams.rs | Send group management for WebTransport streams |
| neqo-http3/src/features/extended_connect/webtransport_session.rs | Session-level send groups, stats tracking, and datagram queue |
| neqo-http3/src/features/extended_connect/tests/webtransport/streams.rs | Comprehensive send group API tests |
| neqo-http3/src/features/extended_connect/tests/webtransport/sessions.rs | Session protocol, stats, and send group validation tests |
| neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs | Test helper fix for datagram return value |
| neqo-http3/src/features/extended_connect/stats.rs | WebTransport session statistics structure |
| neqo-http3/src/features/extended_connect/session.rs | Protocol extraction, datagram queue processing, and stats |
| neqo-http3/src/features/extended_connect/send_group.rs | Send group ID and metadata structures |
| neqo-http3/src/features/extended_connect/mod.rs | Module exports for send groups and datagram outcomes |
| neqo-http3/src/features/extended_connect/datagram_queue.rs | Datagram queuing with age/watermark management |
| neqo-http3/src/connection_server.rs | Server-side datagram queue processing |
| neqo-http3/src/connection_client.rs | Client API for send groups, stats, keying material, and draining |
| neqo-http3/src/connection.rs | HTTP/3 connection send group plumbing and datagram queue integration |
| neqo-http3/src/client_events.rs | New events for draining, stream quota, and datagram outcomes |
| neqo-crypto/tests/agent.rs | Tests for keying material export functionality |
| neqo-crypto/src/err.rs | InvalidState error variant |
| neqo-crypto/src/agent.rs | TLS keying material export implementation |
| neqo-crypto/bindings/bindings.toml | NSS binding for SSL_ExportKeyingMaterial |
| WEBTRANSPORT_IMPLEMENTATION_PLAN.md | Implementation roadmap document |
|
|
||
| pub fn set_max_stream_data(&mut self, limit: u64) { | ||
| qdebug!("setting max_stream_data to {limit}"); | ||
| let previous_limit = self.avail(); |
There was a problem hiding this comment.
The previous_limit is now calculated using self.avail() before checking the stream state. If the stream is not in Ready or Send state, self.avail() will return 0, which may not represent the actual previous limit. This could cause incorrect writable event emission. The original code correctly got the limit from the flow controller when it existed.
Comment on lines
+902
to
+903
| Ok(()) | ||
| // Http3Connection::stream_set_sendorder(&mut self.conn, stream_id, sendorder) |
There was a problem hiding this comment.
The sendorder setting logic is commented out and replaced with an unconditional Ok(()), which breaks the intended functionality. This appears to be debugging code that should be removed or the original implementation should be restored.
Suggested change
| Ok(()) | |
| // Http3Connection::stream_set_sendorder(&mut self.conn, stream_id, sendorder) | |
| Http3Connection::stream_set_sendorder( | |
| &mut self.base_handler, | |
| &mut self.conn, | |
| stream_id, | |
| sendorder, | |
| ) |
Comment on lines
+290
to
+294
| let negotiated_protocol = headers | ||
| .iter() | ||
| .find(|h| h.name().eq_ignore_ascii_case("wt-protocol")) | ||
| .and_then(|h| from_utf8(h.value()).ok()) | ||
| .and_then(|s| { |
There was a problem hiding this comment.
The header name 'wt-protocol' in the implementation differs from 'sec-webtransport-http3-draft' mentioned in the implementation plan (line 356 of WEBTRANSPORT_IMPLEMENTATION_PLAN.md). This discrepancy should be clarified in comments or documentation.
| }; | ||
|
|
||
| use neqo_common::{qdebug, qtrace, Bytes}; | ||
|
|
There was a problem hiding this comment.
The hard limit of 1000 datagrams is not documented. Consider adding a comment explaining why this specific value was chosen and whether it should be configurable.
Suggested change
| // Default upper bound on the number of datagrams kept in the queue. | |
| // | |
| // This value is chosen as a conservative limit to prevent unbounded memory | |
| // growth in typical WebTransport usage while still allowing a reasonably | |
| // deep buffer for bursty traffic. It is only a default: the actual hard | |
| // limit used by a `WebTransportDatagramQueue` instance can be overridden | |
| // via its configuration/constructor when needed. |
Comment on lines
+1695
to
+1699
| #[allow( | ||
| clippy::allow_attributes, | ||
| clippy::missing_errors_doc, | ||
| reason = "OK here." | ||
| )] |
There was a problem hiding this comment.
The reason 'OK here.' is not descriptive. This should explain why missing errors documentation is acceptable for this particular function.
CodSpeed Performance ReportMerging this PR will degrade performance by 4.36%Comparing Summary
Performance Changes
Footnotes
|
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 c0027fa. 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 resultsSignificant performance differences relative to c0027fa. streams/walltime/1000-streams/each-1-bytes: 💔 Performance has regressed by +1.8355%. time: [12.585 ms 12.605 ms 12.625 ms]
change: [+1.6404% +1.8355% +2.0333] (p = 0.00 < 0.05)
Performance has regressed.streams/walltime/1000-streams/each-1000-bytes: 💔 Performance has regressed by +7.2506%. time: [48.466 ms 48.633 ms 48.898 ms]
change: [+6.7283% +7.2506% +7.9155] (p = 0.00 < 0.05)
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severeAll resultstransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1504: Change within noise threshold. time: [201.25 ms 201.81 ms 202.45 ms]
thrpt: [493.95 MiB/s 495.52 MiB/s 496.89 MiB/s]
change:
time: [+0.3135% +0.6753% +1.0399] (p = 0.00 < 0.05)
thrpt: [-1.0292% -0.6708% -0.3125]
Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1504: No change in performance detected. time: [281.06 ms 282.89 ms 284.76 ms]
thrpt: [35.117 Kelem/s 35.350 Kelem/s 35.580 Kelem/s]
change:
time: [-1.6036% -0.5538% +0.4339] (p = 0.30 > 0.05)
thrpt: [-0.4321% +0.5569% +1.6298]
No change in performance detected.transfer/1-conn/1-1b-resp (aka. HPS)/mtu-1504: No change in performance detected. time: [38.561 ms 38.738 ms 38.936 ms]
thrpt: [25.683 B/s 25.814 B/s 25.933 B/s]
change:
time: [-0.7472% -0.0494% +0.6893] (p = 0.90 > 0.05)
thrpt: [-0.6846% +0.0495% +0.7528]
No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
2 (2.00%) high mild
7 (7.00%) high severetransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1504: Change within noise threshold. time: [207.76 ms 208.20 ms 208.75 ms]
thrpt: [479.03 MiB/s 480.31 MiB/s 481.32 MiB/s]
change:
time: [+0.4844% +0.8128% +1.1165] (p = 0.00 < 0.05)
thrpt: [-1.1042% -0.8062% -0.4821]
Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
4 (4.00%) high mild
1 (1.00%) high severedecode 4096 bytes, mask ff: Change within noise threshold. time: [5.3147 µs 5.3507 µs 5.4049 µs]
change: [-1.0086% -0.5214% -0.0036] (p = 0.05 < 0.05)
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severedecode 1048576 bytes, mask ff: No change in performance detected. time: [1.3656 ms 1.3699 ms 1.3742 ms]
change: [-0.1787% +0.3289% +0.8102] (p = 0.20 > 0.05)
No change in performance detected.decode 4096 bytes, mask 7f: No change in performance detected. time: [7.2290 µs 7.2397 µs 7.2505 µs]
change: [-0.2968% +0.2100% +0.8284] (p = 0.54 > 0.05)
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severedecode 1048576 bytes, mask 7f: No change in performance detected. time: [1.8568 ms 1.8594 ms 1.8622 ms]
change: [-0.2217% -0.0255% +0.1793] (p = 0.81 > 0.05)
No change in performance detected.decode 4096 bytes, mask 3f: No change in performance detected. time: [6.9025 µs 6.9124 µs 6.9225 µs]
change: [-0.7198% -0.1616% +0.2994] (p = 0.57 > 0.05)
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severedecode 1048576 bytes, mask 3f: No change in performance detected. time: [1.7649 ms 1.7675 ms 1.7703 ms]
change: [-0.1483% +0.0541% +0.2469] (p = 0.60 > 0.05)
No change in performance detected.streams/simulated/1-streams/each-1000-bytes: No change in performance detected. time: [129.68 ms 129.68 ms 129.69 ms]
thrpt: [7.5302 KiB/s 7.5304 KiB/s 7.5306 KiB/s]
change:
time: [-0.0034% +0.0002% +0.0039] (p = 0.93 > 0.05)
thrpt: [-0.0039% -0.0002% +0.0034]
No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
2 (2.00%) high mildstreams/simulated/1000-streams/each-1-bytes: Change within noise threshold. time: [2.5359 s 2.5362 s 2.5365 s]
thrpt: [394.25 B/s 394.30 B/s 394.34 B/s]
change:
time: [-0.0378% -0.0208% -0.0044] (p = 0.02 < 0.05)
thrpt: [+0.0044% +0.0208% +0.0378]
Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mildstreams/simulated/1000-streams/each-1000-bytes: No change in performance detected. time: [6.5837 s 6.5978 s 6.6146 s]
thrpt: [147.64 KiB/s 148.01 KiB/s 148.33 KiB/s]
change:
time: [-0.2751% -0.0122% +0.2643] (p = 0.93 > 0.05)
thrpt: [-0.2636% +0.0122% +0.2759]
No change in performance detected.
Found 13 outliers among 100 measurements (13.00%)
8 (8.00%) high mild
5 (5.00%) high severestreams/walltime/1-streams/each-1000-bytes: Change within noise threshold. time: [589.69 µs 591.57 µs 593.85 µs]
change: [+0.6674% +1.0541% +1.4718] (p = 0.00 < 0.05)
Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
5 (5.00%) high severestreams/walltime/1000-streams/each-1-bytes: 💔 Performance has regressed by +1.8355%. time: [12.585 ms 12.605 ms 12.625 ms]
change: [+1.6404% +1.8355% +2.0333] (p = 0.00 < 0.05)
Performance has regressed.streams/walltime/1000-streams/each-1000-bytes: 💔 Performance has regressed by +7.2506%. time: [48.466 ms 48.633 ms 48.898 ms]
change: [+6.7283% +7.2506% +7.9155] (p = 0.00 < 0.05)
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severecoalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [92.183 ns 92.560 ns 92.936 ns]
change: [-0.8805% -0.3517% +0.1421] (p = 0.18 > 0.05)
No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
10 (10.00%) high mild
2 (2.00%) high severecoalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [109.71 ns 110.01 ns 110.33 ns]
change: [-0.3644% -0.0200% +0.2849] (p = 0.91 > 0.05)
No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
4 (4.00%) high mild
7 (7.00%) high severecoalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [109.10 ns 109.47 ns 109.92 ns]
change: [-0.6781% +0.8239% +3.2824] (p = 0.62 > 0.05)
No change in performance detected.
Found 13 outliers among 100 measurements (13.00%)
2 (2.00%) low severe
3 (3.00%) low mild
3 (3.00%) high mild
5 (5.00%) high severecoalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [94.976 ns 95.197 ns 95.507 ns]
change: [-0.6242% -0.0317% +0.5680] (p = 0.92 > 0.05)
No change in performance detected.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severeRxStreamOrderer::inbound_frame(): Change within noise threshold. time: [109.72 ms 109.81 ms 109.90 ms]
change: [+0.1794% +0.3875% +0.5494] (p = 0.00 < 0.05)
Change within noise threshold.
Found 15 outliers among 100 measurements (15.00%)
1 (1.00%) low severe
10 (10.00%) low mild
3 (3.00%) high mild
1 (1.00%) high severesent::Packets::take_ranges: No change in performance detected. time: [4.5025 µs 4.6283 µs 4.7557 µs]
change: [-2.6969% +0.7360% +4.5150] (p = 0.68 > 0.05)
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severetransfer/simulated/pacing-false/varying-seeds: No change in performance detected. time: [23.941 s 23.941 s 23.941 s]
thrpt: [171.09 KiB/s 171.09 KiB/s 171.09 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000]
No change in performance detected.transfer/simulated/pacing-true/varying-seeds: No change in performance detected. time: [23.676 s 23.676 s 23.676 s]
thrpt: [173.01 KiB/s 173.01 KiB/s 173.01 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000]
No change in performance detected.transfer/simulated/pacing-false/same-seed: No change in performance detected. time: [23.941 s 23.941 s 23.941 s]
thrpt: [171.09 KiB/s 171.09 KiB/s 171.09 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000]
No change in performance detected.transfer/simulated/pacing-true/same-seed: No change in performance detected. time: [23.676 s 23.676 s 23.676 s]
thrpt: [173.01 KiB/s 173.01 KiB/s 173.01 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000]
No change in performance detected.transfer/walltime/pacing-false/varying-seeds: Change within noise threshold. time: [23.458 ms 23.488 ms 23.532 ms]
change: [+1.6206% +1.8364% +2.0688] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severetransfer/walltime/pacing-true/varying-seeds: Change within noise threshold. time: [23.805 ms 23.837 ms 23.888 ms]
change: [+0.7782% +0.9483% +1.1716] (p = 0.00 < 0.05)
Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severetransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [23.579 ms 23.600 ms 23.622 ms]
change: [+0.8255% +0.9556% +1.0751] (p = 0.00 < 0.05)
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildtransfer/walltime/pacing-true/same-seed: Change within noise threshold. time: [23.795 ms 23.827 ms 23.875 ms]
change: [+0.5357% +0.7851% +1.0010] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severeDownload data for |
Member
|
Two things:
|
Collaborator
|
There is a ton of copy&paste repetition in the test code. Can we make this more DRY? |
Collaborator
|
@jesup is the key export stuff here addressing https://bugzilla.mozilla.org/show_bug.cgi?id=2007201? |
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.
patch for Issue 3381