Conversation
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements OnceMap, a concurrent hash map that ensures computation happens only once per key and caches results. It also reorganizes tests for the once module primitives and enhances singleflight::Group to support generic key queries.
Changes:
- Implements
OnceMapwith thread-safe single-execution-per-key semantics - Adds
try_workmethod tosingleflight::Groupfor fallible computations - Reorganizes tests by moving them from centralized test modules into their respective implementation submodules
- Enhances
singleflight::Group::forgetto accept generic key types via theBorrowtrait
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mea/src/once/once_map/mod.rs | New OnceMap implementation with compute/try_compute methods and HashMap-like interface |
| mea/src/once/once_map/tests.rs | Comprehensive tests for OnceMap covering basic operations, concurrency, and error handling |
| mea/src/once/once/mod.rs | Adds test module declaration |
| mea/src/once/once/tests.rs | Extracted Once tests from the centralized test file |
| mea/src/once/once_cell/mod.rs | Adds test module declaration |
| mea/src/once/once_cell/tests.rs | Removed Once tests that were moved to once/tests.rs, updated imports |
| mea/src/once/mod.rs | Adds once_map module and exports OnceMap |
| mea/src/singleflight/mod.rs | Adds try_work method for fallible operations, updates forget signature to support Borrow, removes comment |
| mea/src/lib.rs | Documents OnceMap in module-level documentation |
| README.md | Adds OnceMap to the list of primitives |
| CHANGELOG.md | Documents new features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+204
to
+237
| pub async fn try_work<E, F>(&self, key: K, func: F) -> Result<V, E> | ||
| where | ||
| F: AsyncFnOnce() -> Result<V, E>, | ||
| { | ||
| // 1. Get or create the OnceCell. | ||
| let cell = { | ||
| let mut map = self.map.lock(); | ||
| map.entry(key.clone()) | ||
| .or_insert_with(|| Arc::new(OnceCell::new())) | ||
| .clone() | ||
| }; | ||
|
|
||
| // 2. Try to initialize the cell. | ||
| // OnceCell::get_or_try_init guarantees that only one task executes the closure. | ||
| let res = cell | ||
| .get_or_try_init(async || { | ||
| let result = func().await?; | ||
|
|
||
| // Cleanup: remove the key from the map. | ||
| // We must ensure we remove the entry corresponding to *this* cell. | ||
| let mut map = self.map.lock(); | ||
| if let Some(existing) = map.get(&key) { | ||
| // Check if the map still points to our cell. | ||
| if Arc::ptr_eq(&cell, existing) { | ||
| map.remove(&key); | ||
| } | ||
| } | ||
|
|
||
| Ok(result) | ||
| }) | ||
| .await?; | ||
|
|
||
| Ok(res.clone()) | ||
| } |
There was a problem hiding this comment.
The new try_work method lacks test coverage. While there is a doc test example, there should be unit tests in the test module to verify the behavior, especially testing scenarios like: concurrent callers when the first fails and the second succeeds, behavior when both fail, and ensuring proper cleanup after failures.
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
1 participant
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.
Some pending issues:
onceprimitives tests. (DONE)Mutex<HashMap>andDashMap(and perhaps move the map interface to theinternalmodule). (LATER)Q: ?SizedwhereK: Borrow<Q>andQ: Hash + Eqlike std's HashMap. (DONE)