Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/future/cache.rs (1)
1305-1333:⚠️ Potential issue | 🔴 Critical
try_get_with_entrycan return anEntrythat panics onkey()At Line 1331,
need_keyisfalse, so this new public API can returnEntry<K, V>with no key materialized. CallingEntry::key()then panics (Bug: Key is None), which is a correctness issue for the exposed method.🔧 Suggested fix
pub async fn try_get_with<F, E>(&self, key: K, init: F) -> Result<V, Arc<E>> where F: Future<Output = Result<V, E>>, E: Send + Sync + 'static, { - self.try_get_with_entry(key, init).await.map(Entry::into_value) + futures_util::pin_mut!(init); + let hash = self.base.hash(&key); + let key = Arc::new(key); + self.get_or_try_insert_with_hash_and_fun(key, hash, init, false) + .await + .map(Entry::into_value) } pub async fn try_get_with_entry<F, E>(&self, key: K, init: F) -> Result<Entry<K, V>, Arc<E>> where F: Future<Output = Result<V, E>>, E: Send + Sync + 'static, { futures_util::pin_mut!(init); let hash = self.base.hash(&key); let key = Arc::new(key); - self.get_or_try_insert_with_hash_and_fun(key, hash, init, false) + self.get_or_try_insert_with_hash_and_fun(key, hash, init, true) .await }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/future/cache.rs` around lines 1305 - 1333, The public method try_get_with_entry currently calls get_or_try_insert_with_hash_and_fun with need_key set to false, which can return an Entry lacking the key and cause Entry::key() to panic; change the call in try_get_with_entry to request the key (set need_key = true) so the returned Entry always contains the key materialized, ensuring Entry::key() is safe for callers (adjust get_or_try_insert_with_hash_and_fun signature/usage if needed to accept the true flag).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/future/cache.rs`:
- Around line 1305-1333: The public method try_get_with_entry currently calls
get_or_try_insert_with_hash_and_fun with need_key set to false, which can return
an Entry lacking the key and cause Entry::key() to panic; change the call in
try_get_with_entry to request the key (set need_key = true) so the returned
Entry always contains the key materialized, ensuring Entry::key() is safe for
callers (adjust get_or_try_insert_with_hash_and_fun signature/usage if needed to
accept the true flag).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75eaad1c-ac86-4633-b64c-7fed5144cd04
📒 Files selected for processing (1)
src/future/cache.rs
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
👋 I have some code that requires to know whether an entry is fresh from the cache or not.
I decided to expose that functionality in a new public function
try_get_with_entry. I'll appreciate it if this code could make it back into upstream. I think it's pretty trivial to maintain since it's just the code intry_get_withwithout themapexposed in a public function.Summary by CodeRabbit
New Features
Refactor