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 misuse of safe iterators #612

Merged
merged 6 commits into from
Jun 10, 2024
Merged

Conversation

madolson
Copy link
Member

@madolson madolson commented Jun 8, 2024

Safe iterators must call resetIterators when they are done being used. Fix one issue where a safe iterator was not correctly calling reset during cluster slot caching and fixed a second issue where reset iterator was being called twice. For the double release case, kvstoreIteratorNextDict is responsible for patching up the iterator, but we were calling it a second time in kvstoreIteratorNext.

In addition, I added some documentation around initializing iterators, added an assert to prevent double initialization, and remove a function from the public interface which isn't needed and might lead to incorrect usage of the safe iterators.

Bumping @srgsanky for finding it here: c478206#r142867004.

Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Copy link

codecov bot commented Jun 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.06%. Comparing base (54c9747) to head (2092b01).
Report is 2 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #612      +/-   ##
============================================
- Coverage     70.20%   70.06%   -0.15%     
============================================
  Files           110      110              
  Lines         60007    60037      +30     
============================================
- Hits          42130    42066      -64     
- Misses        17877    17971      +94     
Files Coverage Δ
src/cluster_legacy.c 86.37% <100.00%> (-0.08%) ⬇️
src/dict.c 97.43% <100.00%> (+<0.01%) ⬆️
src/kvstore.c 96.75% <100.00%> (-0.04%) ⬇️

... and 12 files with indirect coverage changes

Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

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

LGTM.

Not sure if you would like to rename dictInitSafeIterator as part of this PR or not - the safe word is misleading as we discussed offline.

@madolson
Copy link
Member Author

Not sure if you would like to rename dictInitSafeIterator as part of this PR or not - the safe word is misleading as we discussed offline.

I want to do a broader refactoring of that and fix the naming, but this is more scoped specifically to the bug fixes at least.

@madolson madolson merged commit a3f1535 into valkey-io:unstable Jun 10, 2024
18 checks passed
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.

2 participants