You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#208 removed the internal channel bridge in aimdb-sync: blocking calls now go
straight to the runtime through the block_on seam, and SyncProducer<T> holds
a Weak<AimDb> plus the record key, resolving the record on every set().
Two things were deliberately left out of that PR and agreed as follow-up work:
pre-resolving the producer, and dropping try_set now that no buffer can refuse
a value. Both change the same doc surface, so the documentation items the review
raised are collected here rather than fixed twice — along with the crate README,
which the review never opened and which still describes the deleted bridge.
The sections are commit-sized and listed in the order they should land: §1
deletes surface that §2 would otherwise have to modify, both settle the wording
that §3 then fixes, and §4 rewrites the one file no one has looked at since the
bridge existed. §5 is optional and must not hold the branch. Line references are
as of 15d6efd.
1. Drop try_set
AimDb::produce is infallible on the push (aimdb-core/src/builder.rs:927) and
every buffer today overwrites rather than refusing, so set() and try_set()
are the same operation with different error mapping.
Remove SyncProducer::try_set and try_set_value.
Remove SyncError::SetTimeout, which is unreachable once try_set is gone.
Drop the try_set bullets from lib.rs:16 and lib.rs:141,153-154, and the
feature line in examples/sync-api-demo/src/main.rs:171.
Tests: test_runtime_shutdown_error_non_blocking
(tests/integration_test.rs:251) loses its producer half, integration_test.rs:185 switches to set, and try_set_value_is_non_blocking_and_produces
(tests/settable_integration.rs:67) goes with try_set_value.
aimdb-sync/README.md:146,274,287 documents try_set against the pre-Simplify the implementation of aimdb-sync #208 DbError::SetTimeout. Leave it to §4 — the whole file needs rewriting, not
three line fixes.
This also removes an incidental cost: try_set calls db.producer(&self.key)?
per call, which takes impl Into<String> and so clones the key every time.
TryProduceError::Full stays in core; only the facade wrapper goes. No
buffer anywhere in the tree overrides WriteHandle::try_push
(aimdb-core/src/buffer/traits.rs:170) — the only overrides are the two test
doubles in that file's own mod tests — so Full is unreachable at every
layer today, not just this one. The core variants stay because they are the
extension point a bounded, non-overwriting buffer would be built against, and
their docs should be corrected to say so. The facade wrapper is the opposite
trade: it duplicates set() for as long as no such buffer exists, and
re-adding it if one lands is cheap.
SyncError is not #[non_exhaustive] (error.rs:12), so removing a variant
breaks callers that match exhaustively — a minor bump at 0.6.0. Re-adding try_set if a bounded buffer ever lands is cheap; that is the argument for
removing it now rather than documenting a method that does nothing distinct.
2. Pre-resolve the producer
SyncProducer::set() currently does db.upgrade() + a keyed record lookup per
call. Resolve the record once at construction and hold the write handle, the way AimDb::producer() already does (aimdb-core/src/builder.rs:967) and the way AimDbHandle::consumer() already does for the read side.
Consequences to handle:
AimDbHandle::producer() becomes genuinely fallible again — RecordKeyNotFound
and TypeMismatch move from set() to construction time. Restore its # Errors
block (dropped in Simplify the implementation of aimdb-sync #208 precisely because nothing could fail). This, not
throughput, is the reason to do it: the failure surfaces where the key is named.
Keep the Weak<AimDb> alongside the pre-resolved handle. Runtime-shutdown
detection has no other signal on the write side: WriteHandle::push returns (), and TryProduceError::Closed has no implementor either, for the same
reason as Full above. The consumer gets away with pre-resolving only because Reader::recv reports DbError::BufferClosed (consumer.rs:63). A
pre-resolved write handle keeps the record's buffer alive, so without the Weak a post-shutdown set() would silently push into nothing. test_runtime_shutdown_error (tests/integration_test.rs:233) covers this and
must keep passing unchanged.
aimdb-sync/src/producer.rs:62-71 still promises blocking, guaranteed eventual
delivery, and a buffer-full error. None of the three can happen — the lookup is
the only fallible part and the push is infallible. Same wording survives at producer.rs:27 and producer.rs:88.
The matching bullet in aimdb-sync/src/lib.rs:15 ("Blocking send, waits if
channel is full") is the last surviving mention of channel in the crate's
prose. lib.rs:150-152 needs the same fix: the error half is right, the
blocking half is not.
# Panics on the blocking consumer methods
Waiter::block_on (waiter.rs:15) uses Handle::block_on, which aborts with "Cannot start a runtime from within a runtime" if the calling thread is
already driving tasks. All four blocking consumer methods — get, get_with_timeout, get_latest, get_latest_with_timeout — carry this, and
the crate has no # Panics section anywhere today.
Not a new panic class: SyncProducer::set() has always done runtime_handle.block_on and panicked identically. #208 moved it from the write
path to the read path rather than adding it. Document it here. Whether get()
should also try reader.try_recv() first and enter block_on only on BufferEmpty is a separate question that wants a measurement first; the docs are
correct under either outcome, since such a fast path narrows when the panic fires
without removing it.
Consumer error docs
get_latest_with_timeout's # Errors omits SyncError::Db, which get_latest
lists (consumer.rs:260-263).
get_latest_with_timeout's timeout bounds the first value only; the drain that
follows is unbounded. Worth one sentence. In practice the drain outruns a hot
producer (a ring read is cheaper than a keyed produce), so this is a doc fix,
not a code fix.
Small stuff
Typos in new prose: "occuring" (consumer.rs:213, consumer.rs:298, tests/integration_test.rs:344), "occured" (consumer.rs:215, consumer.rs:328),
"succesfully" (consumer.rs:243).
consumer.rs:149: the async { ... .await } wrapper is redundant — block_on(tokio::time::timeout(...)) works directly, the field borrows are disjoint.
handle.rs:266 uses # Errors (wrapped in SyncError::Db); the rest of the
crate uses a plain # Errors.
4. Rewrite aimdb-sync/README.md
The #208 review never touched this file — it appears in none of the twelve
review threads, so every finding the reviewer raised against lib.rs and the
module docs survives here untouched. It still documents the channel bridge as
the crate's architecture:
Lines 27-32 — a Channel Bridge (tokio::sync::mpsc + std::sync::mpsc) box
in the architecture diagram. This is the same defect the reviewer caught in lib.rs's diagram ("still routes both directions through Channels, which is
the design this PR removes") and had fixed there. The README copy was missed.
Lines 317-329 — a Channel Capacity configuration section for producer_with_capacity / consumer_with_capacity / DEFAULT_SYNC_CHANNEL_CAPACITY, which aimdb-sync/CHANGELOG.md:19 records as removed API under the same PR. The section documents a call that no longer
compiles.
Line 449 — "Avoid Blocking: Use try_* methods in latency-sensitive
paths", which becomes wrong for producers the moment §1 lands.
The performance claims are the part that matters
Lines 444-446 state "Channel crossing adds ~1-10μs latency" and "Memory: One
tokio::mpsc channel per producer, one std::mpsc channel per consumer". Both
describe a bridge that no longer exists, and the first is a live latency claim
sitting unqualified in a published crate README.
This is the same family as the ~100–500μs per operation claim #208 removed
from the crate docs, and it should go the same way: delete it in this pass
rather than correct it. There is no current number to replace it with — aimdb-bench has no aimdb-sync target — and none should be estimated. Any
figure that later returns to this README must cite the benchmark that produced
it. Do not carry a placeholder forward.
Rewrite the file against the post-#208 design: the block_on seam, set()
pushing straight into the record's buffer, SyncConsumer holding a Reader<T>
and blocking only when a read has to wait, and the &mut self / no-Clone
consumer signature the CHANGELOG already records. The Optimization Tips section
should survive only where its advice is still true.
5. Optional: cover the mid-drain lag path
drain_remaining (consumer.rs:327) skips BufferLagged and keeps draining —
the fix for the review finding that a lag mid-drain used to end the loop early
and return a value that wasn't the latest. It has no test: the two get_latest
tests only reach the lag branch in get_catch_up, confirmed by instrumenting the
branch. Triggering it needs a producer overrunning the reader between the first
read and the drain, which is hard to make deterministic — worth doing only if it
can be done without a racy test. Do not hold the branch for it.
Acceptance criteria
try_set, try_set_value and SyncError::SetTimeout are gone, with the aimdb-sync CHANGELOG recording the breaking removal.
SyncProducer<T> holds a pre-resolved write handle and the Weak<AimDb>; test_runtime_shutdown_error passes unchanged and AimDbHandle::producer()
has its # Errors block back.
No doc in the crate describes blocking, a channel, or a buffer-full error;
the four blocking consumer methods have # Panics.
grep -i channel aimdb-sync/README.md returns nothing outside the shutdown
plumbing, no removed API is documented as callable, and the crate carries no
latency or memory-footprint claim that is not backed by a current benchmark.
Context
#208 removed the internal channel bridge in
aimdb-sync: blocking calls now gostraight to the runtime through the
block_onseam, andSyncProducer<T>holdsa
Weak<AimDb>plus the record key, resolving the record on everyset().Two things were deliberately left out of that PR and agreed as follow-up work:
pre-resolving the producer, and dropping
try_setnow that no buffer can refusea value. Both change the same doc surface, so the documentation items the review
raised are collected here rather than fixed twice — along with the crate README,
which the review never opened and which still describes the deleted bridge.
The sections are commit-sized and listed in the order they should land: §1
deletes surface that §2 would otherwise have to modify, both settle the wording
that §3 then fixes, and §4 rewrites the one file no one has looked at since the
bridge existed. §5 is optional and must not hold the branch. Line references are
as of
15d6efd.1. Drop
try_setAimDb::produceis infallible on the push (aimdb-core/src/builder.rs:927) andevery buffer today overwrites rather than refusing, so
set()andtry_set()are the same operation with different error mapping.
SyncProducer::try_setandtry_set_value.SyncError::SetTimeout, which is unreachable oncetry_setis gone.try_setbullets fromlib.rs:16andlib.rs:141,153-154, and thefeature line in
examples/sync-api-demo/src/main.rs:171.test_runtime_shutdown_error_non_blocking(
tests/integration_test.rs:251) loses its producer half,integration_test.rs:185switches toset, andtry_set_value_is_non_blocking_and_produces(
tests/settable_integration.rs:67) goes withtry_set_value.aimdb-sync/README.md:146,274,287documentstry_setagainst the pre-Simplify the implementation of aimdb-sync #208DbError::SetTimeout. Leave it to §4 — the whole file needs rewriting, notthree line fixes.
This also removes an incidental cost:
try_setcallsdb.producer(&self.key)?per call, which takes
impl Into<String>and so clones the key every time.TryProduceError::Fullstays in core; only the facade wrapper goes. Nobuffer anywhere in the tree overrides
WriteHandle::try_push(
aimdb-core/src/buffer/traits.rs:170) — the only overrides are the two testdoubles in that file's own
mod tests— soFullis unreachable at everylayer today, not just this one. The core variants stay because they are the
extension point a bounded, non-overwriting buffer would be built against, and
their docs should be corrected to say so. The facade wrapper is the opposite
trade: it duplicates
set()for as long as no such buffer exists, andre-adding it if one lands is cheap.
SyncErroris not#[non_exhaustive](error.rs:12), so removing a variantbreaks callers that match exhaustively — a minor bump at
0.6.0. Re-addingtry_setif a bounded buffer ever lands is cheap; that is the argument forremoving it now rather than documenting a method that does nothing distinct.
2. Pre-resolve the producer
SyncProducer::set()currently doesdb.upgrade()+ a keyed record lookup percall. Resolve the record once at construction and hold the write handle, the way
AimDb::producer()already does (aimdb-core/src/builder.rs:967) and the wayAimDbHandle::consumer()already does for the read side.Consequences to handle:
AimDbHandle::producer()becomes genuinely fallible again —RecordKeyNotFoundand
TypeMismatchmove fromset()to construction time. Restore its# Errorsblock (dropped in Simplify the implementation of aimdb-sync #208 precisely because nothing could fail). This, not
throughput, is the reason to do it: the failure surfaces where the key is named.
Weak<AimDb>alongside the pre-resolved handle. Runtime-shutdowndetection has no other signal on the write side:
WriteHandle::pushreturns(), andTryProduceError::Closedhas no implementor either, for the samereason as
Fullabove. The consumer gets away with pre-resolving only becauseReader::recvreportsDbError::BufferClosed(consumer.rs:63). Apre-resolved write handle keeps the record's buffer alive, so without the
Weaka post-shutdownset()would silently push into nothing.test_runtime_shutdown_error(tests/integration_test.rs:233) covers this andmust keep passing unchanged.
set()'s doc has to be rewritten anyway; see §3.3. Documentation left over from the #208 review
set()describes behavior it no longer hasaimdb-sync/src/producer.rs:62-71still promises blocking, guaranteed eventualdelivery, and a buffer-full error. None of the three can happen — the lookup is
the only fallible part and the push is infallible. Same wording survives at
producer.rs:27andproducer.rs:88.The matching bullet in
aimdb-sync/src/lib.rs:15("Blocking send, waits ifchannel is full") is the last surviving mention of channel in the crate's
prose.
lib.rs:150-152needs the same fix: the error half is right, theblocking half is not.
# Panicson the blocking consumer methodsWaiter::block_on(waiter.rs:15) usesHandle::block_on, which aborts with"Cannot start a runtime from within a runtime" if the calling thread is
already driving tasks. All four blocking consumer methods —
get,get_with_timeout,get_latest,get_latest_with_timeout— carry this, andthe crate has no
# Panicssection anywhere today.Not a new panic class:
SyncProducer::set()has always doneruntime_handle.block_onand panicked identically. #208 moved it from the writepath to the read path rather than adding it. Document it here. Whether
get()should also try
reader.try_recv()first and enterblock_ononly onBufferEmptyis a separate question that wants a measurement first; the docs arecorrect under either outcome, since such a fast path narrows when the panic fires
without removing it.
Consumer error docs
get_latest_with_timeout's# ErrorsomitsSyncError::Db, whichget_latestlists (
consumer.rs:260-263).BufferLaggedis now skipped rather than returned —which is the behavior Simplify the implementation of aimdb-sync #208 introduced.
get_latest_with_timeout's timeout bounds the first value only; the drain thatfollows is unbounded. Worth one sentence. In practice the drain outruns a hot
producer (a ring read is cheaper than a keyed produce), so this is a doc fix,
not a code fix.
Small stuff
consumer.rs:213,consumer.rs:298,tests/integration_test.rs:344), "occured" (consumer.rs:215,consumer.rs:328),"succesfully" (
consumer.rs:243).consumer.rs:149: theasync { ... .await }wrapper is redundant —block_on(tokio::time::timeout(...))works directly, the field borrows are disjoint.handle.rs:266uses# Errors (wrapped in SyncError::Db); the rest of thecrate uses a plain
# Errors.4. Rewrite
aimdb-sync/README.mdThe #208 review never touched this file — it appears in none of the twelve
review threads, so every finding the reviewer raised against
lib.rsand themodule docs survives here untouched. It still documents the channel bridge as
the crate's architecture:
Channel Bridge (tokio::sync::mpsc + std::sync::mpsc)boxin the architecture diagram. This is the same defect the reviewer caught in
lib.rs's diagram ("still routes both directions through Channels, which isthe design this PR removes") and had fixed there. The README copy was missed.
Channel Capacityconfiguration section forproducer_with_capacity/consumer_with_capacity/DEFAULT_SYNC_CHANNEL_CAPACITY, whichaimdb-sync/CHANGELOG.md:19records asremoved API under the same PR. The section documents a call that no longer
compiles.
DbError::SetTimeoutandDbError::RecordNotFound,error names from before the 60 replace typeid storage with recordid recordkey architecture #62 rename and before design 038 §167 split the
sync variants out of
DbErrorintoSyncError.try_*methods in latency-sensitivepaths", which becomes wrong for producers the moment §1 lands.
The performance claims are the part that matters
Lines 444-446 state "Channel crossing adds ~1-10μs latency" and "Memory: One
tokio::mpsc channel per producer, one std::mpsc channel per consumer". Both
describe a bridge that no longer exists, and the first is a live latency claim
sitting unqualified in a published crate README.
This is the same family as the
~100–500μs per operationclaim #208 removedfrom the crate docs, and it should go the same way: delete it in this pass
rather than correct it. There is no current number to replace it with —
aimdb-benchhas noaimdb-synctarget — and none should be estimated. Anyfigure that later returns to this README must cite the benchmark that produced
it. Do not carry a placeholder forward.
Rewrite the file against the post-#208 design: the
block_onseam,set()pushing straight into the record's buffer,
SyncConsumerholding aReader<T>and blocking only when a read has to wait, and the
&mut self/ no-Cloneconsumer signature the CHANGELOG already records. The Optimization Tips section
should survive only where its advice is still true.
5. Optional: cover the mid-drain lag path
drain_remaining(consumer.rs:327) skipsBufferLaggedand keeps draining —the fix for the review finding that a lag mid-drain used to end the loop early
and return a value that wasn't the latest. It has no test: the two
get_latesttests only reach the lag branch in
get_catch_up, confirmed by instrumenting thebranch. Triggering it needs a producer overrunning the reader between the first
read and the drain, which is hard to make deterministic — worth doing only if it
can be done without a racy test. Do not hold the branch for it.
Acceptance criteria
try_set,try_set_valueandSyncError::SetTimeoutare gone, with theaimdb-syncCHANGELOG recording the breaking removal.SyncProducer<T>holds a pre-resolved write handle and theWeak<AimDb>;test_runtime_shutdown_errorpasses unchanged andAimDbHandle::producer()has its
# Errorsblock back.the four blocking consumer methods have
# Panics.grep -i channel aimdb-sync/README.mdreturns nothing outside the shutdownplumbing, no removed API is documented as callable, and the crate carries no
latency or memory-footprint claim that is not backed by a current benchmark.
make checkgreen, including the--no-default-featuresno_std build thatAdd std feature gate and no_std clip #205 established.