Add non-blocking cache methods (non-blocking feature)#112
Add non-blocking cache methods (non-blocking feature)#112fsdvh wants to merge 9 commits intoarthurprs:masterfrom
non-blocking feature)#112Conversation
|
Let me know what you think, @arthurprs. Thank you in advance! |
non-blocking feature)
|
Thanks for the PR. In general I like the additions 👍 Do you think there's a practical benefit putting them behind the feature flag? |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in non-blocking Cargo feature that exposes non-blocking cache operations, allowing callers to skip work when a shard lock is contended instead of blocking. This extends the existing synchronous cache API with contention-aware variants while keeping the default (blocking) behavior unchanged.
Changes:
- Introduces a
non-blockingfeature flag inCargo.toml. - Adds
try_read/try_writeto the internalRwLockwrapper (feature-gated). - Adds
ContendedResultplusCache::try_*methods that returnContendedon lock contention rather than blocking.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Cargo.toml | Adds the non-blocking feature flag. |
| src/rw_lock.rs | Implements non-blocking try_read/try_write on the lock wrapper under the new feature. |
| src/sync.rs | Introduces ContendedResult and new Cache::try_* non-blocking operations gated by the feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[cfg(not(feature = "parking_lot"))] | ||
| { | ||
| self.0.try_read().ok().map(RwLockReadGuard) | ||
| } |
There was a problem hiding this comment.
try_read() maps any Err from the non-parking_lot backends to None via .ok(), which silently treats a poisoned lock the same as contention. This changes behavior vs read() (which panics on poisoning) and also makes the docs inaccurate (poisoning isn't “held by a writer”). Consider matching on TryLockError and only returning None for WouldBlock, while panicking (or otherwise explicitly handling) Poisoned to keep semantics consistent.
| /// Attempts to check if a key exists in the cache without blocking. | ||
| /// Returns [`ContendedResult::Ok(true)`] if present, [`ContendedResult::Ok(false)`] if absent, | ||
| /// or [`ContendedResult::Contended`] if the shard lock could not be acquired without blocking. | ||
| #[cfg(feature = "non-blocking")] | ||
| pub fn try_contains_key<Q>(&self, key: &Q) -> ContendedResult<bool> | ||
| where | ||
| Q: Hash + Equivalent<Key> + ?Sized, | ||
| { | ||
| let Some((shard, hash)) = self.shard_for(key) else { | ||
| return ContendedResult::Ok(false); | ||
| }; | ||
|
|
||
| shard | ||
| .try_read() | ||
| .map(|guard| ContendedResult::Ok(guard.contains(hash, key))) | ||
| .unwrap_or(ContendedResult::Contended) | ||
| } |
There was a problem hiding this comment.
New non-blocking APIs (try_contains_key/try_get/try_peek/try_remove/try_insert*) add important behavior (returning Contended instead of blocking) but there are no feature-gated tests exercising the contended path. Add #[cfg(feature = "non-blocking")] unit tests that hold a shard read/write lock (tests can use the private shard_for helper) and assert these methods return Contended without blocking, plus a basic success-path assertion.
| [features] | ||
| default = ["ahash", "parking_lot"] | ||
| non-blocking = [] | ||
| sharded-lock = ["dep:crossbeam-utils"] | ||
| shuttle = ["dep:shuttle"] | ||
| stats = [] |
There was a problem hiding this comment.
The new non-blocking feature gates public API items, but docs.rs is configured to build docs with only features = ["stats"]. That means the new ContendedResult and Cache::try_* methods won’t appear in published docs by default. Consider adding non-blocking to package.metadata.docs.rs.features (or documenting that users must enable it locally) so the new API is discoverable.
Putting it under a feature flag for convenience, if you think it belongs in the default feature set, I have no objections. 😄 |
Adds opt-in, non-blocking variants of the core cache operations via a new
non-blockingCargo feature.Motivation
Under high write contention a caller may prefer to skip a cache operation rather than block waiting for a shard lock. The existing API offers no way to express this — every operation blocks until the lock is acquired.
Changes
Cargo.tomlnon-blocking = []feature flag (off by default).src/rw_lock.rstry_read/try_writeon the internalRwLockwrapper, gated on#[cfg(feature = "non-blocking")].Option<Guard>across all three backing implementations (parking_lot,crossbeam ShardedLock,std::sync).src/sync.rsContendedResult<Val>enum with variantsOk(Val)andContended, plusok()/is_contended()helpers.Cache, each returningContendedResultand falling back toContendedinstead of blocking:try_contains_keytry_gettry_peektry_removetry_inserttry_insert_with_lifecycleUsage