Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.03%. Comparing base (a47e8fa) to head (c57878c).
⚠️ Report is 414 commits behind head on unstable.

@@             Coverage Diff              @@
##           unstable    #2611      +/-   ##
============================================
- Coverage     72.24%   72.03%   -0.22%     
============================================
  Files           127      128       +1     
  Lines         70820    71041     +221     
============================================
+ Hits          51167    51175       +8     
- Misses        19653    19866     +213     

@uriyage let's also add unittest for this?

@uriyage let's also add unittest for this?

Done

Many times I've tried to reject adding dependencies from the low-level datastructures to the whole of the server globals and everything. Dependencies should be in the other direction, from high level to lower level components.

I somewhat agree with Viktor. We can add a debugAssert to severassert.h if we want, that is just a no-op.

I was also dubious of this PR, since it's just adding a defensive check. @uriyage @ranshid Are we still moving forward with this?

I somewhat agree with Viktor. We can add a debugAssert to severassert.h if we want, that is just a no-op.

We can just use assert instead. The check in the asserts are very light-weight, not even heavier than checking if the debugging is enabled.

Debug assert depends on a server config. That would prevent usage of this hashtable in non-server code (valkey-cli, valkey-benchmark).

@madolson According to a commment above, Uri explained why:

We have a use case where we don't save the iterator state and attempt to run it again.

I suppose we could wait for this use case to be upstreamed and then include this change at the same time.

Couldn't they then just save the state?

I agree, I feel like I would rather close this and re-open if that use case becomes public. I'm not sure what that case is.

Can someone more clearly lay out the scenario here? I'm thinking that this is a case of: 1) creating a safe iterator on the hashtable, 2) deleting or moving the underlying table, 3) continuing to call hashtableNext. Is this right? If so, I don't think this solves the problem.

With a safe iterator, it's necessary to ensure that the iterator gets deleted before the underlying hashtable is deleted. If not, when we delete the iterator, we will try to re-enable rehashing on the (deleted) hashtable.

One possible way to address this is to keep a list of all "safe" iterators associated with each hashtable. When the hashtable is deleted, it could then mark the iterators as invalid. This could also be done for unsafe iterators, but I don't think that's necessary as unsafe iterators, by their nature, are created/used/deleted in a short time.

I've done a review on the iterator, and these are the things that need to be addressed. There are a few potential issues with long-lived (safe) iterators. One use case for this is for defrag which iterates over hashes in an incremental manner - spanning across timer invocations.

Issue 1: Rehashing isn't paused immediately

Ref:

At this code reference, the logic indicates that if it's a safe iterator, that once we perform the first call to hashtableNext that we should pause rehashing. Rather than pausing when the iterator is created, the pause is deferred until we start iterating. This is potentially dangerous. Code, like kvstore, looks at the current rehashing paused state to determine if it's safe to delete the table. This makes it possible for: 1) iterator created, 2) kvstore deletes the table, 3) iteration begins, and the isSafe() clause dereferences the deleted hash table.

Issue 2: Empty hashtables remain in inconsistent state

Ref:

With an empty hash table, the iterator returns false (indicating that iteration has completed). However, index/table still remain in the initial state - as if iteration has not yet begun. This makes it possible for an iterator to return false (indicating completion) but then later, if called again, begin returning newly added values. This likely isn't dangerous, because the caller is unlikely to call hashtableNext a second time after it has already returned false. But it is inconsistent, and easily fixed.

Issue 3: Deletion of hashtable makes hashtableNext unsafe

Ref:

Once the iterator has started, hashtableNext relies on both iter->bucket and iter->hashtable to remain valid. It looks like iter->bucket is ok - it's handled internally that buckets aren't deleted if rehashing is paused. However there's no protection for iter->hashtable. This requires that every user of a safe iterator must be aware of hashtable deletion and avoid calling hashtableNext once the table has been deleted.

I'll suggest that the proper solution is to have a hashtable maintain a list of safe iterators. If the hashtable gets deleted, each of the iterators should be marked as completed such that future calls to hashtableNext will simply return false, without accessing iter->hashtable.

Issue 4: Deletion of hashtable prevents deletion of the iterator

Ref:

This is fairly serious. As it stands, every user of a safe iterator must be aware of a hashtable deletion BEFORE it happens. Once the hashtable has been deleted, it becomes impossible to safely delete the iterator. The iterator deletion attempts to unpause rehashing and accesses the (deleted) hashtable.

I'll suggest that the solution for this is identical to the proposed solution for issue 3. Once the iterator is marked as completed, it should not attempt to access iter->hashtable at deletion time.

@zuiderkwast @rainsupreme - please review.

That's a pretty thorough analysis Jim! I guess there are maybe a few scenarios in OSS where it feels like we might delete the hashtable while the iterator exists, though I haven't fully investigated these:

• pubsub: client disconnect while iterating unsubscribe all?
• vset: could be deleted while processing expiry?
• modules: subcommands hashtable could be freed while being iterated - can modules add/remove commands or be loaded/unloaded at any time?

Adding tracking for safe iterator invalidation should be pretty lightweight in terms of performance and memory. I think we should add it, with unit tests. Also, I'm pretty sure this issue exists for dict's safe iterator as well - dict will be worth fixing if hashtable is.

One other potential issue is with defragmentation. If the hashtable is moved during defragmentation, any active safe iterators will access the deleted memory. I don't think this can happen with defrag's own iterator, but if there's a potential for other iterators, this might be a concern. (e.g. forkless save?)

I think we should fix these bugs found by Jim in a separate PR. Essentially, each hashtable needs to keep track of a list of the safe iterators. (The same problems exist with dict, I imagine.)

However, I don't think we allow long-lived iterators currently. Only a few operations are allowed while iterating (i.e. while the iterator exists) so letting the event loop run and later resume iteration is not currently supported, as I read these doc comments: https://github.com/valkey-io/valkey/blob/9.0/src/hashtable.c#L1960-L1965

I think this falls under the same category as ensuring someone doesn't call hashtableNext after it's returned false, but I don't mind if it's a separate PR.

At least then let's fix the doc comments to clarify this: Other activities (like event loop, defrag, threads) that could defrag/delete the hashtable must not be allowed to run while the iterator is considered valid.

This has been addressed more properly in #3551. Closing.