-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Check that the largest_acked was sent #2150
Conversation
This is a test and fix for the issue we're discussing with Avast. CC @mxinden
Failed Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2150 +/- ##
==========================================
+ Coverage 95.35% 95.39% +0.03%
==========================================
Files 112 112
Lines 36362 36364 +2
==========================================
+ Hits 34674 34688 +14
+ Misses 1688 1676 -12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Thank you.
Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the outstanding comments on the added unit test, this pull request looks good to me.
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
Benchmark resultsPerformance differences relative to bd02153. coalesce_acked_from_zero 1+1 entries: Change within noise threshold.time: [99.530 ns 99.919 ns 100.31 ns] change: [+0.3808% +0.7331% +1.0840%] (p = 0.00 < 0.05) coalesce_acked_from_zero 3+1 entries: Change within noise threshold.time: [117.40 ns 117.73 ns 118.10 ns] change: [+0.3123% +0.8458% +1.4374%] (p = 0.00 < 0.05) coalesce_acked_from_zero 10+1 entries: Change within noise threshold.time: [116.86 ns 117.15 ns 117.53 ns] change: [+0.0906% +0.5397% +0.9897%] (p = 0.01 < 0.05) coalesce_acked_from_zero 1000+1 entries: Change within noise threshold.time: [98.376 ns 98.520 ns 98.677 ns] change: [+0.5317% +1.4106% +2.3493%] (p = 0.00 < 0.05) RxStreamOrderer::inbound_frame(): 💔 Performance has regressed.time: [112.29 ms 112.35 ms 112.41 ms] change: [+1.0284% +1.1042% +1.1761%] (p = 0.00 < 0.05) transfer/pacing-false/varying-seeds: No change in performance detected.time: [26.752 ms 27.783 ms 28.811 ms] change: [-9.5487% -4.6858% +0.3675%] (p = 0.08 > 0.05) transfer/pacing-true/varying-seeds: No change in performance detected.time: [34.160 ms 35.884 ms 37.597 ms] change: [-11.860% -5.9244% +0.2607%] (p = 0.06 > 0.05) transfer/pacing-false/same-seed: No change in performance detected.time: [25.587 ms 26.321 ms 27.050 ms] change: [-4.5334% -0.3910% +3.5249%] (p = 0.85 > 0.05) transfer/pacing-true/same-seed: No change in performance detected.time: [40.924 ms 42.832 ms 44.744 ms] change: [-6.5364% -0.4233% +6.2079%] (p = 0.90 > 0.05) 1-conn/1-100mb-resp (aka. Download)/client: No change in performance detected.time: [113.44 ms 113.96 ms 114.47 ms] thrpt: [873.56 MiB/s 877.48 MiB/s 881.53 MiB/s] change: time: [-1.1277% -0.4615% +0.2393%] (p = 0.19 > 0.05) thrpt: [-0.2388% +0.4636% +1.1405%] 1-conn/10_000-parallel-1b-resp (aka. RPS)/client: Change within noise threshold.time: [309.77 ms 313.51 ms 317.24 ms] thrpt: [31.522 Kelem/s 31.897 Kelem/s 32.282 Kelem/s] change: time: [-3.7901% -2.0361% -0.3446%] (p = 0.02 < 0.05) thrpt: [+0.3458% +2.0785% +3.9394%] 1-conn/1-1b-resp (aka. HPS)/client: No change in performance detected.time: [34.040 ms 34.229 ms 34.440 ms] thrpt: [29.036 elem/s 29.215 elem/s 29.377 elem/s] change: time: [-0.3199% +0.5011% +1.3591%] (p = 0.24 > 0.05) thrpt: [-1.3408% -0.4986% +0.3210%] Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
This is a test and fix for the issue we're discussing with Avast.
It also includes an improvement to let
handle_ack
derivelargest_acked
on the fly, rather than needing to pass it in as an argument.CC @mxinden