dc: Prune peer map if we've pruned the id map #2319
Merged
Conversation
This file contains 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
This ensures that when we prune entries from the map, we bound the size of the peer set as well. This also updates our test coverage to include a small-size map, which causes us to need to tweak a few assertions to account for the now semi-random removals. I'm not very happy with the result, but I think it's OK for now. Mid-term it probably makes sense to figure out a better way to make sure our logic is sound (e.g., probability of removing a recently added peer should be near zero).
camshaft
previously approved these changes
Sep 13, 2024
|
Looks like there's a compilation issue https://github.com/aws/s2n-quic/actions/runs/10839334694/job/30079514118?pr=2319#step:10:549 |
Mark-Simulacrum
force-pushed
the
prune-peers
branch
from
f57495a to
005dbca
Compare
camshaft
approved these changes
Sep 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of changes:
This ensures that when we prune entries from the map, we bound the size of the peer set as well, if slightly eventually consistently (but that's currently true for both maps). Our expectation is that the source of "truth" is always the ids map -- the peers map is essentially just an index over it. On the other hand, the peers map represents the important entries, since they are the latest negotiated secret for a given peer.
Call-outs:
Follow-up work is planned to replace these maps to remove the eventual consistency and hard-constrain memory usage up front, but this patches an immediate memory leak, so prioritizing the simple fix first.
Testing:
Invariant checking is updated -- more changes than I'd like, but previously we weren't testing the overflow at all in those tests (impossible to fill the 500,000 entry map) -- so needed a good chunk of changes to account for that. It would be good to explore changing those tests, I'm not sure how to best write assertions for what is at the end of the day a somewhat random problem.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.