Always check refCount after acquiring lock #1986
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses race conditions in reference-counted cache implementations by adding checks for deleted entries and adjusting lock timing. The changes ensure that operations handle cases where entries are marked for deletion (refCount <= 0) between the time they're looked up and when their locks are acquired.
Key Changes
- Added resurrection logic in
Ref()methods to handle entries deleted while acquiring locks - Added recursive retry logic in
loadOrStoreNewLockedEntry()for deleted entries - Moved
mu.Unlock()inDeref()methods to afterDelete()operations to prevent race conditions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/project/parsecache.go | Adds race condition handling for deleted parse cache entries and fixes unlock ordering in Deref |
| internal/project/extendedconfigcache.go | Adds race condition handling for deleted config cache entries and fixes unlock ordering in Deref |
Comments suppressed due to low confidence (1)
internal/project/extendedconfigcache.go:1
- This recursive retry could loop indefinitely under sustained contention. Consider adding a maximum retry count to prevent potential stack overflow or infinite loops in edge cases where entries are repeatedly deleted.
package project
internal/project/parsecache.go
Outdated
| if existing.refCount <= 0 { | ||
| // Existing entry was deleted while we were acquiring the lock | ||
| existing.mu.Unlock() | ||
| return c.loadOrStoreNewLockedEntry(key) | ||
| } |
There was a problem hiding this comment.
This recursive retry pattern could potentially loop indefinitely if there's sustained contention where entries are constantly being deleted. Consider adding a retry limit or exponential backoff to prevent unbounded recursion in pathological cases.
|
Oh, it's blowing up because we never delete entries in our test suites since they're likely to come back |
|
Doesn't that imply we aren't testing this new code? |
|
Yes. AFAIK it's not possible to deterministically trigger the race, because there's no callback into test code that could occur between the sync map load and the mutex lock. |
| if entry, ok := c.entries.Load(path); ok { | ||
| entry.mu.Lock() | ||
| if entry.refCount <= 0 { | ||
| // Entry was deleted while we were acquiring the lock |
There was a problem hiding this comment.
Isn't possible that the entry got deleted before the cal to c.entries.Load(path) as well?
4 participants
Fixes a race noticed by @sheetalkamat
Probably fixes #1983