-
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(qns): fallback if server is not sending NEW_TOKEN #1837
Conversation
Test failure reported in mozilla#1786 (comment). Signed-off-by: Max Inden <mail@max-inden.de>
Benchmark resultsPerformance differences relative to 4fc4d16.
Client/server transfer resultsTransfer of 134217728 bytes over loopback.
|
Currently debugging neqo -> msquic resumption test failure. |
Got it. I introduced this regression in #1676. While #1676 fixes the race condition around receiving the resumption token from the server, it removes the fallback using a session ticked 🤦. Yet another good reason for #1785 or #1847. Will push a patch in a bit. //CC @larseggert |
See https://github.com/mozilla/neqo/blob/main/neqo-transport/src/connection/mod.rs#L665-L676 for details. Fixes regression introduced in mozilla#1676.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1837 +/- ##
=======================================
Coverage 93.23% 93.23%
=======================================
Files 110 110
Lines 35749 35749
=======================================
+ Hits 33329 33330 +1
+ Misses 2420 2419 -1 ☔ View full report in Codecov by Sentry. |
#1676 ensured for the client to wait for a resumption token from the server if one is needed, that is when running the
resumption
QUIC Interop testcase. #1676 accidentally removed the fallback using a session ticked if no resumption token was available.https://github.com/mozilla/neqo/pull/1676/files#diff-64d071700a607884926de1b2caaf1588558fceda6d3a0e899cfb3c3b7e5ae41fL1353-L1359
This commit re-introduces the fallback and enables the
resumption
QUIC Interop testcase on CI.Along the way, it simplifies
Runner::run
and reserves theresume
command line argument for the QUIC Interop testcases only. Let me know if this adds too much noise to the actual fix. Happy to extract it into a separate pull request.Test failure reported in #1786 (comment).