Fix HeartbeatStorage cache replacement race - #16759
dajiaohuang wants to merge 2 commits into
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prevents a deinitializing HeartbeatStorage instance from evicting a newer cached instance with the same identifier by introducing a stable cacheIdentity (UUID) to track instances. It wraps cached instances in a new HeartbeatStorageCacheEntry struct and only removes them during deinitialization if the identities match. A unit test was added to verify this behavior. Feedback suggests optimizing the removeCachedInstance method to use a single dictionary lookup instead of two to improve performance.
| guard cachedInstances[id]?.cacheIdentity == cacheIdentity else { return } | ||
| cachedInstances.removeValue(forKey: id) |
There was a problem hiding this comment.
We can optimize this method to perform only a single dictionary lookup instead of two. By retrieving the dictionary index first, we can check the identity and remove the entry using the index in O(1) time, avoiding the second hash lookup while holding the lock.
if let index = cachedInstances.index(forKey: id),
cachedInstances[index].value.cacheIdentity == cacheIdentity {
cachedInstances.remove(at: index)
}
|
Thanks for the contribution! A few housekeeping items:
|
paulb777
left a comment
paulb777
left a comment
There was a problem hiding this comment.
Thanks! The race is real in principle (a dying instance's deinit can remove a newer cache entry for the same ID), though hitting it requires deleteApp and configure for the same app ID racing on different threads, and the worst case is a lost or duplicated heartbeat. The fix itself looks correct.
Following up on my earlier comment: since there's no user-visible effect, I'd also be fine with dropping the CHANGELOG entry instead of moving it.
| // Removes the instance if it was cached. | ||
| Self.cachedInstances.withLock { value in | ||
| value.removeValue(forKey: id) | ||
| Self.removeCachedInstance(id: id, cacheIdentity: cacheIdentity, from: &value) |
There was a problem hiding this comment.
Optional simplification: would it be enough to drop the cleanup in deinit entirely? getInstance already replaces an entry whose weak reference is nil, and leftover entries are bounded by the number of app IDs. That removes the race without the extra identity bookkeeping.
This branch has not been deployed
2 participants
Summary
Prevent a stale
HeartbeatStorageinstance from removing a newer cache entry with the same ID.getInstancereplaces a dead weak cache value while holding the cache lock. If the old instance'sdeinitis already waiting for that lock, its unconditional removal by ID can then evict the replacement. A later lookup can create another storage object with a separate serial queue for the same file.Weak references can already be nil when
deinitruns, so cleanup compares a per-instance generation token retained in the cache entry. A unit test models a replacement entry whose weak object is nil, then verifies that stale cleanup preserves it and matching cleanup removes it.Validation
git diff --checkpassed.