Repository navigation
Add lightweight post-removal observer to future::Cache - #607
javaquasar wants to merge 2 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughFuture caches gain an optional synchronous post-removal observer. It receives the removed key, value, and removal cause. Removal paths invoke it alongside any configured asynchronous eviction listener. The change also adds tests, an example, and documentation. ChangesPost-removal observation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant CacheBuilder
participant BaseCache
participant PostRemovalObserver
participant AsyncEvictionListener
CacheBuilder->>BaseCache: pass configured observer during construction
BaseCache->>PostRemovalObserver: invoke synchronously with key, value, and cause
BaseCache->>AsyncEvictionListener: await listener when configured
Merge Risk: ⚪ Minimal · up to No identified issue currently prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The observer is opt-in, receives data already available to eviction listeners, and contains callback panics. No introduced security defect was established. Safe integration still depends on nonblocking, non-reentrant callbacks and application-owned handling of delivery failures; downstream integrations were not available for review. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 65 functions across 8 files. (1 skipped: 1 unsupported.)
✨ 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
Summary
This PR adds a lightweight synchronous
post_removal_observertofuture::Cachefor callersthat only need to publish a small removal record without enabling eviction-listener futures or the
listener-only per-key lock map.
The public contract is intentionally left open for maintainer feedback. Discussion:
#606
Motivation
The existing eviction listener is the right API for asynchronous work and per-key serialization.
For a nonblocking accounting callback, however, a no-op listener has measurable fixed allocation
cost. In five fresh release processes per mode/operation pair, median allocation count relative to
notification-off changed by:
The benchmark classifies allocation ownership; it is not an elapsed-time performance claim. Raw
data, environment metadata, exact commands, and checksums are linked from the Discussion.
Prototype contract
future::Cache.Arc<K>, clonedV, andRemovalCauseafter logical removal.Explicit,Replaced,Expired, andSize.Validation
Entry/compute paths, cancellation, concurrency, panic isolation, ordering, custom hashers, and a
bounded nonblocking queue.
cargo test --all-featurescargo test --locked --no-default-features --features futurecargo clippy --lib --tests --all-features --all-targets -- -D warningscargo fmt --all -- --checkcargo run --example post_removal_observer_async --features futureRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --features futurecargo package --allow-dirty --features future --no-verifyThe fork's Miri workflow reproduces an existing sync-only
timer_wheel_panic_test/parking_lot_corefutex failure. The failing stack contains neither theobserver nor
future::Cachecode.Review focus
I would especially appreciate maintainer guidance on:
future::Cache-only scope;run_pending_taskssemantics.Summary by CodeRabbit