feat(s2n-quic-dc): throttle repeated successful handshakes #2845
Conversation
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
Mark-Simulacrum
force-pushed
the
dc-hs-throttle-success
branch
from
a8032ac to
7e10ff6
Compare
boquan-fang
reviewed
Oct 8, 2025
boquan-fang
reviewed
Oct 8, 2025
Mark-Simulacrum
force-pushed
the
dc-hs-throttle-success
branch
from
7e10ff6 to
30da67a
Compare
camshaft
approved these changes
Oct 9, 2025
boquan-fang
previously approved these changes
Oct 9, 2025
| rng.random_range(1..120) | ||
| rng.random_range(1000..120_000) | ||
| }; | ||
| tokio::time::sleep(Duration::from_secs(duration)).await; |
Contributor
There was a problem hiding this comment.
what was the reason for this change?
Collaborator
Author
There was a problem hiding this comment.
From the commit message:
As a drive-by improvement this also tweaks the delays to measure in
milliseconds, increasing the randomization.
Basically it's increasing the entropy in how much we randomize how long we'll sleep for. I doubt it matters much in practice though.
This mirrors prior work for *failing* handshakes by adding a throttle on outgoing, successful handshakes to a given peer. This is particularly relevant for workloads that happen to trigger replay detection relatively often, which would otherwise cause us to repeatedly handshake with the peer. It remains generally a good idea for us to switch to new secret material in case of e.g. spurious bitflips in memory or other issues that have broken the old entry in some way, so retaining the handshake-on-possible-replay makes sense, but doing so at a high rate increases CPU utilization for little benefit. As a drive-by improvement this also tweaks the delays to measure in milliseconds, increasing the randomization.
Mark-Simulacrum
force-pushed
the
dc-hs-throttle-success
branch
from
30da67a to
7ec75cf
Compare
boquan-fang
approved these changes
Oct 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Release Summary:
Resolved issues:
n/a
Description of changes:
This mirrors prior work for failing handshakes by adding a throttle on outgoing, successful handshakes to a given peer. This is particularly relevant for workloads that happen to trigger replay detection relatively often, which would otherwise cause us to repeatedly handshake with the peer.
It remains generally a good idea for us to switch to new secret material in case of e.g. spurious bitflips in memory or other issues that have broken the old entry in some way, so retaining the handshake-on-possible-replay makes sense, but doing so at a high rate increases CPU utilization for little benefit.
Call-outs:
n/a
Testing:
No particular testing added. This is exercise by most of our existing tests since the code is hit by anything that handshakes; I don't think we need additional, dedicated test coverage for it.
The stream tests are updated to disable this jitter as it breaks restart tests. As noted in comments, I think the tradeoff is reasonable, but open to other thoughts there (or other defaults instead of 1 minute).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.