Conversation
…or_each Cover the previously untested branches: - removing a held entry via the in-use path (entry.remove()) - held entries without a value (skipped, kept until guard drop) - for_each visiting/skipping held entries after guard release - white-box tests for the defensive unheld-valueless state (refcnt == 0, no value), unreachable through the public API
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==========================================
+ Coverage 99.29% 99.91% +0.62%
==========================================
Files 3 3
Lines 2272 2422 +150
==========================================
+ Hits 2256 2420 +164
+ Misses 16 2 -14 ☔ View full report in Codecov by Harness. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds targeted tests to exercise previously uncovered branches in LockMap::retain and LruLockMap::{retain, for_each}, including behavior around held entries, valueless entries, and defensive internal states. This improves confidence in concurrent behavior and helps ensure the “in-use” code paths are covered.
Changes:
- Add tests ensuring held entries are removed via the in-use path (
entry.remove()) inretain. - Add tests verifying valueless held entries are skipped (and cleaned up on guard drop), and that
for_eachvisits held entries after guard release. - Add white-box tests that simulate the defensive “unheld + valueless” internal state and validate
retain/for_eachbehavior (and LRU list consistency forLruLockMap).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/lockmap.rs |
Adds new retain tests covering held-entry removal, valueless held-entry skipping, and defensive unheld-valueless state. |
src/lru_lockmap.rs |
Adds analogous retain tests plus for_each tests and an LRU-consistency assertion for defensive unheld-valueless removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1592
to
+1599
| let retainer = { | ||
| let map = map.clone(); | ||
| std::thread::spawn(move || map.retain(|_, v| *v % 2 == 0)) | ||
| }; | ||
|
|
||
| std::thread::sleep(std::time::Duration::from_millis(10)); | ||
| drop(held); | ||
| retainer.join().unwrap(); |
Comment on lines
+1615
to
+1629
| let calls = Arc::new(AtomicU32::new(0)); | ||
| let retainer = { | ||
| let map = map.clone(); | ||
| let calls = calls.clone(); | ||
| std::thread::spawn(move || { | ||
| map.retain(|_, _| { | ||
| calls.fetch_add(1, Ordering::AcqRel); | ||
| false | ||
| }) | ||
| }) | ||
| }; | ||
|
|
||
| std::thread::sleep(std::time::Duration::from_millis(10)); | ||
| drop(held); | ||
| retainer.join().unwrap(); |
Comment on lines
+1649
to
+1654
| if s.key == 1 { | ||
| // SAFETY: the shard lock is held and refcnt == 0, so no | ||
| // guard exists and none can be created concurrently. | ||
| unsafe { (*s.value.get()).take() }; | ||
| s.set_value_state(false); | ||
| } |
Comment on lines
+2508
to
+2515
| let retainer = { | ||
| let cache = cache.clone(); | ||
| std::thread::spawn(move || cache.retain(|_, v| *v % 2 == 0)) | ||
| }; | ||
|
|
||
| std::thread::sleep(std::time::Duration::from_millis(10)); | ||
| drop(held); | ||
| retainer.join().unwrap(); |
Comment on lines
+2531
to
+2545
| let calls = Arc::new(AtomicU32::new(0)); | ||
| let retainer = { | ||
| let cache = cache.clone(); | ||
| let calls = calls.clone(); | ||
| std::thread::spawn(move || { | ||
| cache.retain(|_, _| { | ||
| calls.fetch_add(1, Ordering::AcqRel); | ||
| false | ||
| }) | ||
| }) | ||
| }; | ||
|
|
||
| std::thread::sleep(std::time::Duration::from_millis(10)); | ||
| drop(held); | ||
| retainer.join().unwrap(); |
Comment on lines
+2565
to
+2570
| if s.key == 1 { | ||
| // SAFETY: the shard lock is held and refcnt == 0, so no | ||
| // guard exists and none can be created concurrently. | ||
| unsafe { (*s.value.get()).take() }; | ||
| s.set_value_state(false); | ||
| } |
Comment on lines
+2601
to
+2613
| let visitor = { | ||
| let cache = cache.clone(); | ||
| std::thread::spawn(move || { | ||
| let mut visited = Vec::new(); | ||
| cache.for_each(|k, v| visited.push((*k, *v))); | ||
| visited.sort(); | ||
| visited | ||
| }) | ||
| }; | ||
|
|
||
| std::thread::sleep(std::time::Duration::from_millis(10)); | ||
| drop(held); | ||
| assert_eq!(visitor.join().unwrap(), vec![(1, 10), (2, 20)]); |
Comment on lines
+2623
to
+2634
| let visitor = { | ||
| let cache = cache.clone(); | ||
| std::thread::spawn(move || { | ||
| let mut visited = Vec::new(); | ||
| cache.for_each(|k, v| visited.push((*k, *v))); | ||
| visited | ||
| }) | ||
| }; | ||
|
|
||
| std::thread::sleep(std::time::Duration::from_millis(10)); | ||
| drop(held); | ||
| assert_eq!(visitor.join().unwrap(), vec![(1, 10)]); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cover the previously untested branches: