fix: Don't encode large RTT guesses in tickets #2114

larseggert · 2024-09-16T09:31:11Z

Because under lossy conditions (e.g., QNS handshakeloss test), the guess can be multiple times the actual RTT, which when encoded in the resumption ticket will cause an extremely slow second handshake, often causing the test to time out.

Broken out of #1998
Fixes #2088

Because under lossy conditions (e.g., QNS `handshakeloss` test), the guess can be multiple times the actual RTT, which when encoded in the resumption ticket will cause an extremely slow second handshake, often causing the test to time out. Broken out of mozilla#1998 Fixes mozilla#2088

github-actions · 2024-09-16T09:49:10Z

Failed Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest as server

All results

Succeeded Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest vs. aioquic: H DC LR C20 M S R 3 B U A L2 C1 C2 6 V2
neqo-latest vs. go-x-net: H DC LR M B U A L2 C2 6
neqo-latest vs. haproxy: H DC LR C20 M S R Z 3 B U A L1 L2 C1 C2 6 V2
neqo-latest vs. kwik: H DC LR C20 M S R Z 3 B U A L1 L2 C1 C2 6 V2
neqo-latest vs. lsquic: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6 V2
neqo-latest vs. msquic: H DC LR C20 M S R Z B U L1 L2 C1 C2 6 V2
neqo-latest vs. mvfst: H DC LR M R Z 3 B U L2 C2 6
neqo-latest vs. neqo: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6 V2
neqo-latest vs. neqo-latest: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6 V2
neqo-latest vs. nginx: H DC LR C20 M S R Z 3 B U A L1 L2 C1 C2 6
neqo-latest vs. ngtcp2: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6 V2
neqo-latest vs. picoquic: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6 V2
neqo-latest vs. quic-go: H DC LR C20 M S R Z 3 B U A L1 L2 C1 C2 6
neqo-latest vs. quiche: H DC LR C20 M S R Z 3 B U A L1 L2 C1 C2 6
neqo-latest vs. quinn: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6
neqo-latest vs. s2n-quic: H DC LR C20 M S R 3 B U E A L1 L2 C1 C2 6
neqo-latest vs. xquic: H DC LR C20 M R Z 3 B U L1 L2 C1 C2 6

neqo-latest as server

aioquic vs. neqo-latest: H DC LR C20 M S R Z 3 B A L1 L2 C1 C2 6 V2
go-x-net vs. neqo-latest: H DC LR M B U A L2 C2 6
kwik vs. neqo-latest: H DC LR C20 M S R Z 3 B U A L1 L2 C1 C2 6 V2
lsquic vs. neqo-latest: H DC LR M S R 3 B E A L1 L2 C1 C2 6 V2
msquic vs. neqo-latest: H DC LR C20 M S R Z B A L1 L2 C1 C2 6 V2
mvfst vs. neqo-latest: H DC LR M 3 B L2 C2 6
neqo vs. neqo-latest: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6 V2
ngtcp2 vs. neqo-latest: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6 V2
picoquic vs. neqo-latest: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6 V2
quic-go vs. neqo-latest: H DC LR C20 M S R Z 3 B U A L1 L2 C1 C2 6
quiche vs. neqo-latest: H DC LR M S R Z 3 B A L1 L2 C1 C2 6
quinn vs. neqo-latest: H DC LR C20 M S R Z 3 B U E A L1 L2 C1 C2 6
s2n-quic vs. neqo-latest: H DC LR M S R 3 B E A L1 L2 C1 C2 6
xquic vs. neqo-latest: H DC LR C20 S R Z 3 B U A L1 L2 C1 C2 6

Unsupported Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest vs. aioquic: E
neqo-latest vs. go-x-net: C20 S R Z 3 E L1 C1 V2
neqo-latest vs. haproxy: E
neqo-latest vs. kwik: E
neqo-latest vs. msquic: 3 E
neqo-latest vs. mvfst: C20 S E V2
neqo-latest vs. nginx: E V2
neqo-latest vs. quic-go: E V2
neqo-latest vs. quiche: E V2
neqo-latest vs. quinn: V2
neqo-latest vs. s2n-quic: Z V2
neqo-latest vs. xquic: S E V2

neqo-latest as server

aioquic vs. neqo-latest: U E
chrome vs. neqo-latest: H DC LR C20 M S R Z B U E A L1 L2 C1 C2 6 V2
go-x-net vs. neqo-latest: C20 S R Z 3 E L1 C1 V2
kwik vs. neqo-latest: E
lsquic vs. neqo-latest: C20 Z U
msquic vs. neqo-latest: 3 E
mvfst vs. neqo-latest: C20 S R U E V2
quic-go vs. neqo-latest: E V2
quiche vs. neqo-latest: C20 U E V2
s2n-quic vs. neqo-latest: C20 Z U V2
xquic vs. neqo-latest: E V2

codecov · 2024-09-16T09:57:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.35%. Comparing base (0cb89a9) to head (6de4d70).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2114   +/-   ##
=======================================
  Coverage   95.35%   95.35%           
=======================================
  Files         112      112           
  Lines       36316    36334   +18     
=======================================
+ Hits        34628    34647   +19     
+ Misses       1688     1687    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-09-16T10:42:58Z

Benchmark results

coalesce_acked_from_zero 1+1 entries:

       time:   [99.188 ns 99.537 ns 99.890 ns]
Found 13 outliers among 100 measurements (13.00%)
  10 (10.00%) high mild
  3 (3.00%) high severe

coalesce_acked_from_zero 3+1 entries:

       time:   [116.94 ns 117.30 ns 117.68 ns]
Found 15 outliers among 100 measurements (15.00%)
  3 (3.00%) low mild
  12 (12.00%) high severe

coalesce_acked_from_zero 10+1 entries:

       time:   [116.54 ns 117.73 ns 119.75 ns]
Found 14 outliers among 100 measurements (14.00%)
  4 (4.00%) low severe
  1 (1.00%) low mild
  2 (2.00%) high mild
  7 (7.00%) high severe

coalesce_acked_from_zero 1000+1 entries:

       time:   [97.464 ns 97.579 ns 97.710 ns]
Found 15 outliers among 100 measurements (15.00%)
  9 (9.00%) high mild
  6 (6.00%) high severe

RxStreamOrderer::inbound_frame():

       time:   [110.92 ms 111.06 ms 111.28 ms]
Found 8 outliers among 100 measurements (8.00%)
  7 (7.00%) low mild
  1 (1.00%) high severe

transfer/pacing-false/varying-seeds:

       time:   [26.854 ms 27.948 ms 29.038 ms]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) low mild

transfer/pacing-true/varying-seeds:

       time:   [33.862 ms 35.583 ms 37.332 ms]
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

transfer/pacing-false/same-seed:

       time:   [32.313 ms 33.059 ms 33.798 ms]

transfer/pacing-true/same-seed:

       time:   [40.343 ms 42.721 ms 45.125 ms]

1-conn/1-100mb-resp (aka. Download)/client:

       time:   [114.10 ms 114.42 ms 114.74 ms]
       thrpt:  [871.52 MiB/s 873.96 MiB/s 876.43 MiB/s]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) low mild

1-conn/10_000-parallel-1b-resp (aka. RPS)/client:

       time:   [313.76 ms 317.20 ms 320.61 ms]
       thrpt:  [31.190 Kelem/s 31.525 Kelem/s 31.871 Kelem/s]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) low mild

1-conn/1-1b-resp (aka. HPS)/client:

       time:   [33.876 ms 34.118 ms 34.381 ms]
       thrpt:  [29.086  elem/s 29.310  elem/s 29.520  elem/s]
Found 9 outliers among 100 measurements (9.00%)
  6 (6.00%) high mild
  3 (3.00%) high severe

Client/server transfer results

Transfer of 33554432 bytes over loopback.

Client	Server	CC	Pacing	Mean [ms]	Min [ms]	Max [ms]	Relative
msquic	msquic			182.0 ± 109.8	102.1	516.5	1.00
neqo	msquic	reno	on	287.1 ± 90.9	217.9	461.5	1.00
neqo	msquic	reno		241.8 ± 56.0	206.2	415.1	1.00
neqo	msquic	cubic	on	237.9 ± 68.3	206.6	451.5	1.00
neqo	msquic	cubic		226.0 ± 12.0	208.4	244.6	1.00
msquic	neqo	reno	on	166.8 ± 90.6	96.1	380.7	1.00
msquic	neqo	reno		140.4 ± 88.9	83.6	338.6	1.00
msquic	neqo	cubic	on	195.3 ± 127.1	98.0	530.4	1.00
msquic	neqo	cubic		160.8 ± 123.4	83.8	610.1	1.00
neqo	neqo	reno	on	204.2 ± 106.4	127.6	429.3	1.00
neqo	neqo	reno		234.5 ± 154.9	142.9	609.4	1.00
neqo	neqo	cubic	on	213.8 ± 89.0	123.6	426.2	1.00
neqo	neqo	cubic		207.6 ± 94.9	130.8	411.3	1.00

⬇️ Download logs

github-actions · 2024-09-16T11:50:50Z

Firefox builds for this PR

The following builds are available for testing. Crossed-out builds did not succeed.

Linux: Debug Release
macOS: Debug Release
Windows: Debug Release

larseggert · 2024-09-16T12:35:17Z

This needs more work. I thought the code here

neqo/neqo-transport/src/path.rs

Lines 971 to 990 in 75372c2

 if self.rtt.first_sample_time().is_none() { 

 // When discarding a packet there might not be a good RTT estimate. 

 // But discards only occur after receiving something, so that means 

 // that there is some RTT information, which is better than nothing. 

 // Two cases: 1. at the client when handling a Retry and 

 // 2. at the server when disposing the Initial packet number space. 

 qinfo!( 

 [self], 

 "discarding a packet without an RTT estimate; guessing RTT={:?}", 

 now - sent.time_sent() 

 ); 

 stats.rtt_init_guess = true; 

 self.rtt.update( 

 &self.qlog, 

 now - sent.time_sent(), 

 Duration::new(0, 0), 

 false, 

 now, 

 ); 

 }

was an optimization, but the idle_timeout_crazy_rtt test hangs when I remove it.

@martinthomson do you remember why this was put in? This kind of RTT guessing isn't using packet number and so becomes very wrong very quickly when there is loss.

neqo-transport/src/connection/mod.rs

mxinden

Thank you for the elaborate test!

neqo-transport/src/rtt.rs

larseggert requested review from KershawChang, martinthomson and mxinden as code owners September 16, 2024 09:31

larseggert marked this pull request as draft September 16, 2024 12:30

Fixes & tests

502a04a

larseggert marked this pull request as ready for review September 16, 2024 13:19

mxinden reviewed Sep 16, 2024

View reviewed changes

neqo-transport/src/connection/mod.rs Show resolved Hide resolved

neqo-transport/src/connection/mod.rs Outdated Show resolved Hide resolved

Suggestion from @mxinden

536a648

larseggert requested a review from mxinden September 17, 2024 11:01

Fix

df225a0

mxinden approved these changes Sep 17, 2024

View reviewed changes

neqo-transport/src/rtt.rs Show resolved Hide resolved

larseggert added this pull request to the merge queue Sep 17, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 17, 2024

larseggert added this pull request to the merge queue Sep 17, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 17, 2024

Merge branch 'main' into fix-no-large-rtt-guesses

6de4d70

larseggert enabled auto-merge September 17, 2024 12:37

larseggert added this pull request to the merge queue Sep 17, 2024

Merged via the queue into mozilla:main with commit b72b3ba Sep 17, 2024
56 checks passed

larseggert deleted the fix-no-large-rtt-guesses branch September 17, 2024 13:31

mxinden mentioned this pull request Oct 3, 2024

chore: prepare v0.9.1 release #2148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Don't encode large RTT guesses in tickets #2114

fix: Don't encode large RTT guesses in tickets #2114

larseggert commented Sep 16, 2024

github-actions bot commented Sep 16, 2024 •

edited

Loading

Succeeded Interop Tests

neqo-latest as client

neqo-latest as server

Unsupported Interop Tests

neqo-latest as client

neqo-latest as server

codecov bot commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

larseggert commented Sep 16, 2024

mxinden left a comment

fix: Don't encode large RTT guesses in tickets #2114

fix: Don't encode large RTT guesses in tickets #2114

Conversation

larseggert commented Sep 16, 2024

github-actions bot commented Sep 16, 2024 • edited Loading

Failed Interop Tests

neqo-latest as client

neqo-latest as server

Succeeded Interop Tests

neqo-latest as client

neqo-latest as server

Unsupported Interop Tests

neqo-latest as client

neqo-latest as server

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

github-actions bot commented Sep 16, 2024 • edited Loading

Benchmark results

Client/server transfer results

github-actions bot commented Sep 16, 2024 • edited Loading

Firefox builds for this PR

larseggert commented Sep 16, 2024

mxinden left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading