Fix deadlock issue #26
Conversation
Summary of ChangesHello @0xLeif, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical deadlock scenario within the Highlights
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 correctly fixes a potential deadlock in PersistableCache by using defer to ensure the lock is always released, which is a great improvement for thread safety. The addition of BugExposingTests.swift is also very valuable, especially the testPersistableCacheDeadlockAfterDeleteError test which directly verifies the fix and prevents regressions. The new test file also includes tests for LRUCache, which improve overall test coverage. I've found one minor issue in the implementation of one of the new tests that could be improved for clarity.
| let expectation = XCTestExpectation(description: "Concurrent operations complete") | ||
|
|
||
| DispatchQueue.concurrentPerform(iterations: iterations) { i in | ||
| let key = i % 30 | ||
|
|
||
| // Interleave different operations | ||
| switch i % 4 { | ||
| case 0: | ||
| cache[key] = i | ||
| case 1: | ||
| _ = cache[key] | ||
| case 2: | ||
| _ = cache.contains(key) | ||
| case 3: | ||
| cache.remove(key) | ||
| default: | ||
| break | ||
| } | ||
|
|
||
| // Periodically check consistency | ||
| if i % 1000 == 0 { | ||
| let count = cache.allValues.count | ||
| if count > 50 { | ||
| errorLock.lock() | ||
| errors.append("Cache exceeded capacity: \(count) > 50 at iteration \(i)") | ||
| errorLock.unlock() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| expectation.fulfill() | ||
| wait(for: [expectation], timeout: 30.0) | ||
|
|
||
| // Report any consistency violations | ||
| if !errors.isEmpty { | ||
| XCTFail("Race condition detected! Errors:\n\(errors.joined(separator: "\n"))") | ||
| } | ||
|
|
||
| // Final consistency check | ||
| let finalCount = cache.allValues.count | ||
| XCTAssertLessThanOrEqual(finalCount, 50, "Final cache size \(finalCount) exceeds capacity of 50") |
There was a problem hiding this comment.
The use of XCTestExpectation here is not correct. DispatchQueue.concurrentPerform is a synchronous call that blocks the current thread until all of its iterations are complete. This means the expectation is fulfilled immediately after the concurrent work is done, and wait(for:timeout:) returns right away without actually waiting for any asynchronous work. The test still works as intended for concurrency testing, but the expectation adds confusion. It's better to remove the expectation and the wait for clarity.
DispatchQueue.concurrentPerform(iterations: iterations) { i in
let key = i % 30
// Interleave different operations
switch i % 4 {
case 0:
cache[key] = i
case 1:
_ = cache[key]
case 2:
_ = cache.contains(key)
case 3:
cache.remove(key)
default:
break
}
// Periodically check consistency
if i % 1000 == 0 {
let count = cache.allValues.count
if count > 50 {
errorLock.lock()
errors.append("Cache exceeded capacity: \(count) > 50 at iteration \(i)")
errorLock.unlock()
}
}
}
// Report any consistency violations
if !errors.isEmpty {
XCTFail("Race condition detected! Errors:\n\(errors.joined(separator: "\n"))")
}
// Final consistency check
let finalCount = cache.allValues.count
XCTAssertLessThanOrEqual(finalCount, 50, "Final cache size \(finalCount) exceeds capacity of 50")There was a problem hiding this comment.
Pull request overview
This PR fixes a critical deadlock issue in PersistableCache where the NSLock was not properly released when save() or delete() operations threw exceptions. The fix replaces manual lock/unlock calls with the safer defer pattern, ensuring locks are always released even when errors are thrown. The PR also introduces a comprehensive test suite to verify the fix and expose other potential concurrency issues in the cache implementations.
Key changes:
- Replaced manual lock management with
defer { lock.unlock() }pattern inPersistableCache.save()andPersistableCache.delete()methods - Added
BugExposingTests.swiftwith three test cases covering the deadlock fix, LRU cache recency behavior, and potential race conditions from dual-lock architecture
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| Sources/Cache/Cache/PersistableCache.swift | Fixes deadlock by using defer to guarantee lock release in save() and delete() methods, even when exceptions are thrown |
| Tests/CacheTests/BugExposingTests.swift | Adds comprehensive test coverage including deadlock detection, LRU cache behavior validation, and concurrency stress testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| XCTAssertNotEqual(result, .timedOut, "DEADLOCK DETECTED: save() hung after delete() error - lock was never released!") | ||
| } | ||
|
|
||
| // MARK: - LRUCache contains() Updates Recency |
There was a problem hiding this comment.
The comment header says "LRUCache contains() Updates Recency" but doesn't include a bug number like the other test sections (e.g., "Bug #1" and "Bug #3"). For consistency, consider either adding a bug number (e.g., "Bug #2:") or removing the "Bug #1" and "Bug #3" prefixes from the other sections to maintain a uniform documentation style.
| // MARK: - LRUCache contains() Updates Recency | |
| // MARK: - Bug #2: LRUCache contains() Updates Recency |
| // Periodically check consistency | ||
| if i % 1000 == 0 { | ||
| let count = cache.allValues.count | ||
| if count > 50 { |
There was a problem hiding this comment.
The magic number 50 is used here and also on line 77 (capacity) and line 122 (final assertion). Consider extracting this to a constant at the test method level for better maintainability:
let capacity = 50
let cache = LRUCache<Int, Int>(capacity: capacity)
// ...
if count > capacity {
// ...
}
// ...
XCTAssertLessThanOrEqual(finalCount, capacity, "Final cache size \(finalCount) exceeds capacity of \(capacity)")| /// This test exposes a deadlock bug in PersistableCache. | ||
| /// When delete() throws an error, the NSLock is never released. | ||
| /// Any subsequent call to save() or delete() will hang forever. |
There was a problem hiding this comment.
The test documentation states "This test exposes a deadlock bug in PersistableCache" (present tense), but with the accompanying fix in PersistableCache.swift, this bug is now resolved. Consider updating the documentation to clarify that this test verified the deadlock bug that has now been fixed, or that it serves as a regression test. For example:
/// This test verifies that a deadlock bug in PersistableCache has been fixed.
/// Previously, when delete() threw an error, the NSLock was never released,
/// causing subsequent calls to save() or delete() to hang forever.| /// This test exposes a deadlock bug in PersistableCache. | |
| /// When delete() throws an error, the NSLock is never released. | |
| /// Any subsequent call to save() or delete() will hang forever. | |
| /// This test verifies that a deadlock bug in PersistableCache has been fixed. | |
| /// Previously, when delete() threw an error, the NSLock was never released, | |
| /// causing subsequent calls to save() or delete() to hang forever. |
| /// This test validates that contains() updates LRU order. | ||
| /// Calling contains() is considered a "use" of the key, promoting it to most recently used. |
There was a problem hiding this comment.
[nitpick] The test comment states "This test validates that contains() updates LRU order" but the test name and context suggest this might be exposing a bug rather than validating correct behavior. Consider clarifying whether this is testing for expected behavior or exposing a bug. If contains() updating recency is the intended behavior, the comment is accurate. If not, it should indicate this is a bug being exposed.
| /// This test validates that contains() updates LRU order. | |
| /// Calling contains() is considered a "use" of the key, promoting it to most recently used. | |
| /// This test exposes a bug: contains() updates LRU order. | |
| /// Calling contains() is currently considered a "use" of the key, promoting it to most recently used. |
| let expectation = XCTestExpectation(description: "Concurrent operations complete") | ||
|
|
||
| DispatchQueue.concurrentPerform(iterations: iterations) { i in | ||
| let key = i % 30 | ||
|
|
||
| // Interleave different operations | ||
| switch i % 4 { | ||
| case 0: | ||
| cache[key] = i | ||
| case 1: | ||
| _ = cache[key] | ||
| case 2: | ||
| _ = cache.contains(key) | ||
| case 3: | ||
| cache.remove(key) | ||
| default: | ||
| break | ||
| } | ||
|
|
||
| // Periodically check consistency | ||
| if i % 1000 == 0 { | ||
| let count = cache.allValues.count | ||
| if count > 50 { | ||
| errorLock.lock() | ||
| errors.append("Cache exceeded capacity: \(count) > 50 at iteration \(i)") | ||
| errorLock.unlock() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| expectation.fulfill() | ||
| wait(for: [expectation], timeout: 30.0) |
There was a problem hiding this comment.
The expectation is fulfilled immediately after concurrentPerform completes (line 112), but concurrentPerform is a synchronous call that already blocks until all iterations finish. This makes the expectation and the wait on line 113 redundant. The expectation pattern is typically used for asynchronous operations, but here everything is already synchronous.
Consider removing the expectation entirely and directly asserting after concurrentPerform completes:
DispatchQueue.concurrentPerform(iterations: iterations) { i in
// ... existing logic
}
// Report any consistency violations
if !errors.isEmpty {
XCTFail("Race condition detected! Errors:\n\(errors.joined(separator: "\n"))")
}
// Final consistency check
let finalCount = cache.allValues.count
XCTAssertLessThanOrEqual(finalCount, 50, "Final cache size \(finalCount) exceeds capacity of 50")| /// its own lock separate from the parent Cache's lock. | ||
| func testLRUCacheDualLockRaceCondition() { | ||
| let cache = LRUCache<Int, Int>(capacity: 50) | ||
| let iterations = 100_000 |
There was a problem hiding this comment.
The test uses 100,000 iterations which may cause the test to be slow and potentially timeout on CI systems or slower machines. Consider reducing this to a more reasonable number (e.g., 10,000 or 1,000) that still effectively tests for race conditions while maintaining reasonable test execution time.
| let iterations = 100_000 | |
| let iterations = 10_000 |
No description provided.