-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: Add type safety to FindMutable calls #5886
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but I think some comments are obsolete
src/server/string_family.cc
Outdated
const int64_t increment_ns = emission_interval_ns * quantity; // should be nonnegative | ||
|
||
auto res = db_slice.FindMutable(op_args.db_cntx, key); | ||
// Use type-safe FindMutable with OBJ_STRING (fixes #5316) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those comments are redundant, especially referencing the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
db_slice.Del(db_context, it); | ||
auto res_it = db_slice.FindMutable(db_context, key, OBJ_HASH); | ||
if (res_it) { | ||
res_it->post_updater.Run(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need to run it manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AutoUpdater dtor automatically calls Run() on destruction, which has a DCHECK that verifies the key still exists in the database.
Since we call db_slice.Del() earlier in the code (which removes the key), we must manually call post_updater.Run() before deletion to update memory accounting while the key is still valid. Otherwise, when res_it goes out of scope, the destructor will call Run() on an already-deleted key and the DCHECK will fail.
This pattern is used throughout the codebase whenever a key is deleted after FindMutable().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can unify somehow this code, I mean do delete in post_updater or call run during delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern (post_updater.Run() + Del()) appears many times across several files in the codebase, so unifying it makes sense.
Suggested approach:
Add a new method DelMutable() to DbSlice:
void DbSlice::DelMutable(Context cntx, ItAndUpdater& it_updater) {
it_updater.post_updater.Run();
Del(cntx, it_updater.it);
}
This would simplify code like:
// From
res_it->post_updater.Run();
db_slice.Del(op_args.db_cntx, res_it->it);
// To
db_slice.DelMutable(op_args.db_cntx, *res_it);
However, I'd suggest doing this as a separate follow-up PR after this one, because:
- This PR focuses on type safety fixes
- Refactoring many call sites is a larger change that deserves separate review
- Mixing two different goals increases risk and review complexity
Maybe better create a separate issue for this refactoring?
Fixes: #5316
Adds explicit type checking to
FindMutable
calls in string, set, and hash family commands to prevent accidentally deleting objects of the wrong type.Changes:
string_family.cc
:OpIncrBy
,OpThrottle
→ addedOBJ_STRING
set_family.cc
:OpAdd
→ addedOBJ_SET
hset_family.cc
:HRandField
→ addedOBJ_HASH
This ensures that operations fail fast with
WRONG_TYPE
instead of potentially corrupting data by operating on objects of unexpected types.