Skip to content
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(server): Do not crash HRANDFIELD if some/all elements expired #2113

Merged
merged 2 commits into from
Nov 5, 2023

Conversation

chakaz
Copy link
Collaborator

@chakaz chakaz commented Nov 2, 2023

The issue is twofold:

  1. If all elements are expired, dividing by Size() will crash. However, we cannot know this in advance, as begin() will actually invalidate all elements, but we're already inside RandomPair() which must return a result (it should not be called if real size is 0, after expiration)
  2. We cannot use operator += before we know how many real elements are in the array. Today we do += rand() / Size(), and this in fact is problematic even if Size() is non-zero, but is greater than post-expiration size

Unfortunately, this solution will iterate over all elements in map for the multi-random retrieval function, to know how many non-expired elements (if any) are in the list. I don't think it's too bad, as we iterate there anyway (although not necessarily through all)

Fixes #2108 and other bugs.

@chakaz
Copy link
Collaborator Author

chakaz commented Nov 2, 2023

One such "more clever solution" would be to change the signature to optional<pair> RandomPair(), and also change the signature of Advance() to return whether it reached the end (or just check *this == end()), and then to change operator += to quietly exit if end() was reached.

However this is slightly subtle, and also I'm not a fan of quietly quitting without performing what we were asked for operator+=...

@chakaz chakaz requested a review from adiholden November 2, 2023 13:04
@chakaz
Copy link
Collaborator Author

chakaz commented Nov 2, 2023

I decided to, indeed, go with the slightly more complicated solution. It's not too bad, and definitely a better & safer API.
Will update the desc as well.

@@ -142,6 +162,8 @@ void StringMap::RandomPairsUnique(unsigned int count, std::vector<sds>& keys,

void StringMap::RandomPairs(unsigned int count, std::vector<sds>& keys, std::vector<sds>& vals,
bool with_value) {
CollectExpired();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a comment, this is O(n) function that may slow down RandomPairs for large sets?

Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@chakaz chakaz requested a review from romange November 3, 2023 06:18
@chakaz
Copy link
Collaborator Author

chakaz commented Nov 5, 2023

friendly ping :)
(I only added a comment as you asked)

@romange romange merged commit f809fb0 into main Nov 5, 2023
10 checks passed
@romange romange deleted the hset-expiry branch November 5, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StringMap could crash with operator+=
2 participants