feat: Define a path migration event#3598
Conversation
martinthomson
requested review from
KershawChang,
larseggert and
mxinden
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped addition. The new PathMigrated event correctly covers all three migration paths: client-initiated forced migration (Connection::migrate with force=true), server-side peer migration (handle_migration), and client deferred migration upon PATH_RESPONSE validation. The was_primary guard in handle_migration correctly avoids spurious events when the path was already primary (since Paths::handle_migration is a no-op in that case aside from updating the timestamp). Deduplication via insert/contains with the derived PartialEq works as expected.
One minor import ordering nit noted inline.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Martin Thomson <mt@lowentropy.net>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3598 +/- ##
==========================================
- Coverage 95.11% 94.99% -0.13%
==========================================
Files 111 116 +5
Lines 37931 38293 +362
Branches 37931 38293 +362
==========================================
+ Hits 36078 36375 +297
- Misses 1158 1211 +53
- Partials 695 707 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Clean, well-scoped addition. The three emission sites correctly cover the distinct migration triggers:
- Client forced migration (
Connection::migratewithforce=true/ path already valid) — immediate event. - Server peer migration (
handle_migration) — event gated bywas_primaryto avoid spurious fires when the path is already primary. - Client deferred migration (
PATH_RESPONSEhandler) — event after path validation completes.
Preferred address migration is covered transitively through Connection::migrate(force=false) → deferred → site 3. Deduplication via insert()/PartialEq on the derived ConnectionEvent is sound. The HTTP/3 layers correctly acknowledge and ignore the new variant.
The previous review's import ordering nit has been addressed in the second commit.
Two optional suggestions left inline: a small DRY helper for the repeated borrow-extract-emit pattern, and an event assertion in the preferred_address test to close the last coverage gap.
Which should save one borrow each time.
There was a problem hiding this comment.
Clean, well-scoped addition. The path_migrated helper on Connection consolidates the borrow-extract-emit pattern across all three emission sites, addressing the DRY suggestion from the prior review. The import ordering nit is also fixed.
Correctness: The three call sites cover the distinct migration triggers correctly:
- Forced/immediate migration in
Connection::migrate(client-initiated,force=true). - Peer migration in
handle_migration(server-side), gated by!was_primaryto avoid spurious events. - Deferred migration upon
PATH_RESPONSEvalidation.
Preferred-address migration flows through site 3 transitively (via migrate(force=false) → deferred → PATH_RESPONSE), so it's covered. The preferred_address() test helper doesn't assert the event though — adding an assertion after client.process_input(resp, now()) (line 810) would close the last coverage gap.
Public API: ConnectionEvent::PathMigrated is a new variant on a public enum. Downstream consumers with exhaustive matches will need updating — the three in-tree consumers (http09, connection_client, connection_server) are handled.
There was a problem hiding this comment.
Clean, well-scoped addition. The new ConnectionEvent::PathMigrated variant and the three emission sites correctly cover all migration triggers:
- Forced/immediate migration (
Connection::migratewithforce=trueor already-valid path) — event emitted synchronously before returning to caller. - Server-side peer migration (
handle_migration) — gated by!was_primary, which is correct:Paths::handle_migrationearly-returns when the path is already primary (only updating the timestamp viaupdate(now)), so no migration actually occurs in that case. - Deferred migration (PATH_RESPONSE validation) — emitted after
Paths::path_responsereturnstrue, which internally callsselect_primaryto promote the migration target. Theif let Some(primary)guard is sound sinceprimary()is guaranteed to returnSomeafterselect_primarycompletes.
Preferred-address migration flows through site 1 (if path is already valid) or transitively through site 3 (via migrate(force=false) → deferred → PATH_RESPONSE), so it is covered without a dedicated call site.
The path_migrated helper on Connection consolidates the borrow-extract-emit pattern, keeping the three call sites DRY. Event deduplication via insert()/derived PartialEq works as expected for PathMigrated.
Public API: ConnectionEvent is pub and not #[non_exhaustive], so this new variant is a semver-breaking change for downstream consumers with exhaustive matches. The three in-tree consumers (http09, connection_client, connection_server) are handled. The semver CI check should surface any concern here.
Prior review suggestions (import ordering, DRY helper extraction) have been addressed in subsequent commits.
larseggert
left a comment
larseggert
left a comment
There was a problem hiding this comment.
LGTM; minor clippy nit.
Benchmark resultsSignificant performance differences relative to 6d76b52. transfer/1-conn/1-100mb-resp (aka. Download): 💔 Performance has regressed by +3.9086%. time: [197.15 ms 197.54 ms 198.01 ms]
thrpt: [505.02 MiB/s 506.23 MiB/s 507.23 MiB/s]
change:
time: [+3.5454% +3.9086% +4.2733] (p = 0.00 < 0.05)
thrpt: [-4.0982% -3.7616% -3.4240]
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/1-conn/1-100mb-req (aka. Upload): 💔 Performance has regressed by +3.3303%. time: [198.55 ms 199.09 ms 199.83 ms]
thrpt: [500.42 MiB/s 502.29 MiB/s 503.64 MiB/s]
change:
time: [+2.9276% +3.3303% +3.7739] (p = 0.00 < 0.05)
thrpt: [-3.6366% -3.2230% -2.8443]
Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severeAll resultstransfer/1-conn/1-100mb-resp (aka. Download): 💔 Performance has regressed by +3.9086%. time: [197.15 ms 197.54 ms 198.01 ms]
thrpt: [505.02 MiB/s 506.23 MiB/s 507.23 MiB/s]
change:
time: [+3.5454% +3.9086% +4.2733] (p = 0.00 < 0.05)
thrpt: [-4.0982% -3.7616% -3.4240]
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS): No change in performance detected. time: [282.23 ms 284.12 ms 286.04 ms]
thrpt: [34.960 Kelem/s 35.197 Kelem/s 35.433 Kelem/s]
change:
time: [-1.5122% -0.5883% +0.3225] (p = 0.22 > 0.05)
thrpt: [-0.3215% +0.5918% +1.5354]
No change in performance detected.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/1-conn/1-1b-resp (aka. HPS): No change in performance detected. time: [38.537 ms 38.714 ms 38.919 ms]
thrpt: [25.694 B/s 25.830 B/s 25.949 B/s]
change:
time: [-0.3424% +0.2832% +0.9193] (p = 0.40 > 0.05)
thrpt: [-0.9109% -0.2824% +0.3436]
No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
6 (6.00%) high severetransfer/1-conn/1-100mb-req (aka. Upload): 💔 Performance has regressed by +3.3303%. time: [198.55 ms 199.09 ms 199.83 ms]
thrpt: [500.42 MiB/s 502.29 MiB/s 503.64 MiB/s]
change:
time: [+2.9276% +3.3303% +3.7739] (p = 0.00 < 0.05)
thrpt: [-3.6366% -3.2230% -2.8443]
Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severestreams/walltime/1-streams/each-1000-bytes: No change in performance detected. time: [587.63 µs 589.36 µs 591.44 µs]
change: [-0.4199% -0.0218% +0.4140] (p = 0.92 > 0.05)
No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
2 (2.00%) high mild
7 (7.00%) high severestreams/walltime/1000-streams/each-1-bytes: Change within noise threshold. time: [12.090 ms 12.108 ms 12.127 ms]
change: [-0.6241% -0.4036% -0.1832] (p = 0.00 < 0.05)
Change within noise threshold.streams/walltime/1000-streams/each-1000-bytes: No change in performance detected. time: [43.276 ms 43.328 ms 43.381 ms]
change: [-0.3716% -0.1375% +0.0793] (p = 0.24 > 0.05)
No change in performance detected.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildstreams-flow-controlled/walltime/1-streams/each-4194304-bytes: Change within noise threshold. time: [33.742 ms 33.792 ms 33.842 ms]
change: [+0.2775% +0.4732% +0.6940] (p = 0.00 < 0.05)
Change within noise threshold.streams-flow-controlled/walltime/10-streams/each-1048576-bytes: No change in performance detected. time: [95.687 ms 96.979 ms 98.321 ms]
change: [-1.8088% +0.1042% +2.1068] (p = 0.91 > 0.05)
No change in performance detected.transfer/walltime/pacing-false/varying-seeds: Change within noise threshold. time: [22.840 ms 22.871 ms 22.919 ms]
change: [-2.4338% -2.2011% -1.9653] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severetransfer/walltime/pacing-true/varying-seeds: Change within noise threshold. time: [23.335 ms 23.362 ms 23.396 ms]
change: [-1.0514% -0.8775% -0.7112] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [23.000 ms 23.025 ms 23.058 ms]
change: [-2.0394% -1.8012% -1.6015] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/walltime/pacing-true/same-seed: Change within noise threshold. time: [23.542 ms 23.571 ms 23.615 ms]
change: [-0.8301% -0.6798% -0.4924] (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 severeDownload data for |
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 6d76b52. 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 |
2 participants
No description provided.