Skip to content

aimdb-sync: drop try_set, pre-resolve the producer, finish the doc pass #212

Description

@lxsaah

Context

#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.
  • set()'s doc has to be rewritten anyway; see §3.

3. Documentation left over from the #208 review

set() describes behavior it no longer has

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).
  • Neither method says that BufferLagged is 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 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.
  • Lines 274, 287-289DbError::SetTimeout and DbError::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 DbError into SyncError.
  • 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

  1. try_set, try_set_value and SyncError::SetTimeout are gone, with the
    aimdb-sync CHANGELOG recording the breaking removal.
  2. 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.
  3. No doc in the crate describes blocking, a channel, or a buffer-full error;
    the four blocking consumer methods have # Panics.
  4. 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.
  5. make check green, including the --no-default-features no_std build that
    Add std feature gate and no_std clip #205 established.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions