feat: unsync feature flag for ?Send contexts#16
Conversation
Gate the existing ?Send / Rc code paths (previously wasm32-only) behind a feature flag so native single-threaded runtimes (tokio::task::LocalSet, embedded async executors) get first-class support. Changes: - Cargo.toml: add `unsync = []` feature - backend/mod.rs: trait definitions use cfg(any(wasm32, unsync)) - client.rs: SharedBackend/SharedEncryption aliases use Rc when unsync - backend impls: use cfg_attr for async_trait vs async_trait(?Send) - error.rs: BackendError::source drops Send+Sync bounds when unsync - test mocks: shared()/new_with_handle() pattern for Arc/Rc-agnostic testing Closes #15
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
crates/cachekit/src/backend/mod.rs (1)
30-124:⚠️ Potential issue | 🟡 MinorAdd per-method documentation to the
?Sendtrait variants for consistency.Each of
Backend,TtlInspectable, andLockableBackendis defined twice with#[cfg]gating — once forSend + Synccontexts and once for?Send(wasm32/unsync). While trait duplication is the standard pattern for conditional Send bounds in async-trait, the?Sendvariants (lines 56-67, 89-97, 117-124) omit the per-method///doc comments present on the default variants.This causes documentation drift: running
cargo doc --features unsyncwill render with less per-method documentation than the default build, leading to inconsistent developer experience across feature flags.Porting the
///comments from theSend + Syncvariants to their?Sendcounterparts will keep the documentation consistent without requiring structural changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cachekit/src/backend/mod.rs` around lines 30 - 124, The ?Send trait variants are missing the per-method doc comments present on the Send+Sync versions; update the ?Send variants of Backend, TtlInspectable, and LockableBackend by copying the existing /// comments for each method (Backend::get, set, delete, exists, health; TtlInspectable::ttl, refresh_ttl; LockableBackend::acquire_lock, release_lock) from the Send+Sync definitions into their corresponding #[async_trait(?Send)] definitions so documentation is consistent across feature flags.
🧹 Nitpick comments (2)
crates/cachekit/tests/macro_tests.rs (2)
26-35: Minor:Arc<CountingInner>is used unconditionally even underunsync.The shared-state wrapper inside
CountingBackendis hard-coded tostd::sync::Arcregardless of whetherunsync/wasm32is active. It's harmless (the atomics + tokio mutex areSend + Syncso the?Sendtrait is satisfied either way), but it diverges from the PR's principle of flipping Arc↔Rc by cfg. Consider mirroring the same selection here for consistency with the production code paths the test suite exercises.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cachekit/tests/macro_tests.rs` around lines 26 - 35, The test uses std::sync::Arc for CountingBackend.inner unconditionally; mirror the crate's cfg switch so tests use Rc under unsync/wasm32: introduce a cfg-gated alias (e.g., type SharedCounting = std::sync::Arc<CountingInner> when not cfg(unsync) and type SharedCounting = std::rc::Rc<CountingInner> under cfg(unsync) or wasm32) and change CountingBackend.inner to that alias (and adjust derives/impls if needed for Clone/Default) so the test flips Arc↔Rc the same way production code does.
38-48: Optional: factor the Arc/Rc selection into a helper to mirrorMockBackend::into_shared.This block re-implements the same
cfg-based pointer selection thattests/common/mod.rsalready centralizes forMockBackend. Extracting a small helper (or movingCountingBackendnext toMockBackendincommon) would remove the duplication and keep the per-target pointer choice in one place.Also, since
CountingBackendderivesDefault, the explicit construction can be simplified.♻️ Suggested simplification
fn new_with_handle() -> (SharedBackend, Self) { - let backend = Self { - inner: std::sync::Arc::new(CountingInner::default()), - }; + let backend = Self::default(); let handle = backend.clone(); #[cfg(not(any(target_arch = "wasm32", feature = "unsync")))] let shared: SharedBackend = std::sync::Arc::new(backend); #[cfg(any(target_arch = "wasm32", feature = "unsync"))] let shared: SharedBackend = std::rc::Rc::new(backend); (shared, handle) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cachekit/tests/macro_tests.rs` around lines 38 - 48, The new_with_handle function duplicates cfg-based Arc/Rc selection already centralized by MockBackend::into_shared; refactor by calling that helper (or create a small into_shared for CountingBackend alongside MockBackend) to return the SharedBackend instead of repeating cfg blocks, and simplify construction by using CountingInner::default() (or Default::default()) when building the backend/handle; update references to SharedBackend, new_with_handle, CountingInner, and MockBackend::into_shared to locate and reuse the centralized pointer-selection logic.
🤖 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 `@crates/cachekit/src/backend/mod.rs`:
- Around line 30-124: The ?Send trait variants are missing the per-method doc
comments present on the Send+Sync versions; update the ?Send variants of
Backend, TtlInspectable, and LockableBackend by copying the existing ///
comments for each method (Backend::get, set, delete, exists, health;
TtlInspectable::ttl, refresh_ttl; LockableBackend::acquire_lock, release_lock)
from the Send+Sync definitions into their corresponding #[async_trait(?Send)]
definitions so documentation is consistent across feature flags.
---
Nitpick comments:
In `@crates/cachekit/tests/macro_tests.rs`:
- Around line 26-35: The test uses std::sync::Arc for CountingBackend.inner
unconditionally; mirror the crate's cfg switch so tests use Rc under
unsync/wasm32: introduce a cfg-gated alias (e.g., type SharedCounting =
std::sync::Arc<CountingInner> when not cfg(unsync) and type SharedCounting =
std::rc::Rc<CountingInner> under cfg(unsync) or wasm32) and change
CountingBackend.inner to that alias (and adjust derives/impls if needed for
Clone/Default) so the test flips Arc↔Rc the same way production code does.
- Around line 38-48: The new_with_handle function duplicates cfg-based Arc/Rc
selection already centralized by MockBackend::into_shared; refactor by calling
that helper (or create a small into_shared for CountingBackend alongside
MockBackend) to return the SharedBackend instead of repeating cfg blocks, and
simplify construction by using CountingInner::default() (or Default::default())
when building the backend/handle; update references to SharedBackend,
new_with_handle, CountingInner, and MockBackend::into_shared to locate and reuse
the centralized pointer-selection logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b6dd110-3f64-41c9-8e78-70c8eef6fd57
📒 Files selected for processing (12)
crates/cachekit/Cargo.tomlcrates/cachekit/src/backend/cachekitio.rscrates/cachekit/src/backend/cachekitio_lock.rscrates/cachekit/src/backend/cachekitio_ttl.rscrates/cachekit/src/backend/mod.rscrates/cachekit/src/backend/redis.rscrates/cachekit/src/client.rscrates/cachekit/src/error.rscrates/cachekit/tests/client_tests.rscrates/cachekit/tests/common/mod.rscrates/cachekit/tests/encryption_tests.rscrates/cachekit/tests/macro_tests.rs
The unsync/wasm32 variants of Backend, TtlInspectable, and LockableBackend were missing per-method doc comments, producing missing_docs warnings. Copy the docs from the Send+Sync definitions.
|
@coderabbitai re-review pls |
|
✅ Actions performedFull review triggered. |
1 participant
Summary
unsyncCargo feature flag that gates the existing?Send/Rccode paths (previously only active onwasm32) for use on native targetstokio::task::LocalSet, single-threaded runtimes, and embedded async executors to use cachekit-rs withoutSendboundscfg_attrto switch between#[async_trait]and#[async_trait(?Send)]on the same impl blocksAffected code
Cargo.tomlunsync = []featurebackend/mod.rsBackend,TtlInspectable,LockableBackendtrait defsclient.rsSharedBackend(ArcvsRc),SharedEncryption,from_env()error.rsBackendError::sourcedropsSend+Syncboundsbackend/{cachekitio,redis,cachekitio_lock,cachekitio_ttl}.rscfg_attron impl blockstests/shared()/new_with_handle()pattern for Arc/Rc-agnostic mockingUsage
Test plan
cargo test(default features) — 108 tests passcargo test --features unsync— 108 tests passcargo check --features unsync,redis,encryption,l1— compiles cleanCloses #15
Summary by CodeRabbit
New Features
unsyncCargo feature to toggle relaxed async behavior.Refactor
Sendrequirements when the feature is enabled.Tests