Simplify the implementation of aimdb-sync

[fresheed]

Description

Note that some of existing/current behaviors are not entirely clear; I highlighted them below.

The changed made are:

SyncProducer

• New struct definition keeps a weak ref to AimDb and the key. This is needed to detect runtime shutdown upon producing (test_runtime_shutdown_error). If the underlying producer was pre-resolved (as brought up in discussion), it won't be that easy.
• Removed set_with_timeout
• Removed everything capacity-related

SyncConsumer

• Now it holds instances of Waiter and Reader, as suggested in the issue
• Note that the shutdown detection is not detected until the buffer has been cleared - should we keep that?. There is a new integration test for that: test_runtime_shutdown_after_produce_read_error. This is consistent with the behavior of tokio's broadcast.
• Clone and Sync implementations are removed
• Relaxed trait bound for the T generic
• signatures of all methods are changed to use &mut self
• The current implementation keeps the previous behavior of get_latest: if an error occurs after the initial read, the method returns the latest read value and not the error. Should we keep it that way?
• updated docs wrt. "thread safety"
• removed everything capacity-related
• Clarified error descriptions for all methods

Waiter

Implemented as described in the issue for std feature

handle

• Note that producer method cannot fail anymore. Should its signature be changed?
• Removed everything capacity-related
• extracted the runtime setup to a separate function (no behavior changes)
• The new method is marked as legacy, but it's used by attach - what to do with that?
• Added a separate function for handling errors that might occur on subscribe

unsafe impl

Removed everywhere from aimdb-sync. Now corresponding test modules contain some boilerplate to confirm that producer, consumer and handle implement Send and Sync (the latter is not for consumer).
I'm curious if it can be done with static_assertions crate instead.

integration tests

• Added aforementioned test for shutdown behavior of get
• Added a test for RuntimeShutdown on the non-blocking path too (try_set/try_get after detach()) — test_non_blocking_operations and test_runtime_shutdown_error each only covered half of it
• removed usages of thread::sleep - everything is blocking now, and in multithreaded tests consumers block on producers anyway
• added mut to consumers
• settable_integration was NOT run by cargo test before due to data-contracts feature. Making it run required changes in Cargo files - please check it

examples

• added mut to consumers
• fixed the closing println! summary: it advertised set_timeout as a producer capability (removed) and misnamed get_timeout (real name is get_with_timeout)

documentation

• Updated crate-level docs (lib.rs) to match the rewrite: per-type Send/Sync/Clone claims instead of a blanket "all types are Send + Sync" (Thread-Safe bullet, ## Safety section), the ## Overview/## Threading Model sections no longer describe a channel-based bridge (that's the block_on seam now), and fixed two method names that never existed (get_timeout/set_timeout → get_with_timeout, and dropped the set_timeout() bullet since there's no timeout variant on the producer)
• removed the overhead claim. Should we remeasure it? If so, I'd add a separate issue for that
• error.rs's doc comment mentioned "channel timeouts" among the facade-specific failures — dropped, since neither SetTimeout (buffer-full on try_set) nor GetTimeout (tokio timer / empty buffer) involve a channel anymore

Related Issue

Closes #200

Checklist

• I have read the CONTRIBUTING.md document.
• My code follows the project's coding standards.
• I have added tests to cover my changes.
• All new and existing tests passed (make check).
• I have updated the documentation accordingly.

[lxsaah]

Thanks for this @fresheed, it's a solid piece of work. Deleting the six unsafe impl Send/Sync blocks is worth the PR on its own; those were assertions rather than proofs and SyncConsumer losing Clone is the right call. −800/+345 against #200 is exactly the shape I was hoping for.

Two logistics notes before I do a detailed pass.

Please rebase first

main moved after you branched: #205 ("Add std feature gate and no_std clip") landed on top of your base and restructured the same four files. Right now cargo build -p aimdb-sync --no-default-features fails on your branch and merging main conflicts in consumer.rs, producer.rs, handle.rs and lib.rs. None of that is your fault, but I'd rather you rebase before we discuss details, so we're not reviewing wording that conflict resolution will rewrite anyway.

git fetch origin main && git rebase origin/main

Resolution notes:

• Imports: keep main's alloc::sync::Arc, core::fmt::Debug, core::time::Duration. Your diff reintroduces the std:: versions because your base predates the no_std work.
• lib.rs: keep main's #![cfg_attr(not(feature = "std"), no_std)] and the per-module #[cfg(feature = "std")] gates and add mod waiter; to that gated group. Then drop the #[cfg(feature = "std")] from waiter.rs itself — gating at the module declaration is the convention Add std feature gate and no_std clip #205 established.
• Doctests in lib.rs: main wraps them in #![cfg_attr(feature = "std", doc = "```no_run")] / #![cfg_attr(not(feature = "std"), doc = "```ignore")] pairs. Any doctest you rewrite needs to keep that wrapper.
• Cargo.toml: keep main's optional tokio/aimdb-tokio-adapter and the std = [...] feature, and please drop the self-dev-dependency (details below).

Verify with make check or the fast subset:

cargo build  -p aimdb-sync --no-default-features
cargo test   -p aimdb-sync
cargo test   -p aimdb-sync --no-default-features
cargo test   -p aimdb-sync --features data-contracts
cargo clippy -p aimdb-sync --all-targets -- -D warnings
cargo clippy -p aimdb-sync --no-default-features --all-targets -- -D warnings

The --no-default-features test should report 0 tests and ignored doctests. That's the signal the no_std gate is still intact.

Your questions

settable_integration / the Cargo change. Good catch and it was a real gap at your base, but please drop the self-dev-dependency. Main now runs cargo test -p aimdb-sync --features data-contracts directly (Makefile:175), so it's covered. The workaround also has a side effect worth knowing about: because the dev-dep edge requests default features, it re-enables std and turns cargo test -p aimdb-sync --no-default-features into a std build — which would silently disable our no_std verification. It also means plain cargo test stops exercising the default feature set.

Shutdown not observed until the buffer drains. Keep it. Matching broadcast is the right precedent and test_runtime_shutdown_after_produce_read_error documents it well.

get_latest returning the last value instead of a later error. Keep it for BufferEmpty. But note a related change I want to discuss next round: lag now reaches callers as SyncError::Db(BufferLagged { .. }) from get(), where the old forwarding task swallowed it — while your drain loop still swallows it. Worth reconciling.

Should producer() stop returning SyncResult? Keep the Result. It's another breaking change for no gain and if we pre-resolve the producer (which I think we should, for other reasons) it becomes fallible again.

new marked legacy but used by attach. AimDbSyncExt::attach is public API, so that path isn't legacy — just drop the label. (It's already pub(crate), which is the right visibility.)

static_assertions? It'd work (assert_impl_all!), but it needs a concrete type and a new dependency. const _: fn() = || { assert_send::<SyncConsumer<Foo>>(); }; gets the same compile-time check for free and doesn't need #[allow(dead_code)].

Re-measuring the overhead claim. Agreed on removing it and yes — separate issue.

After the rebase

Ping me and I'll do the full pass. I have a handful of items queued, mostly around the producer docs still describing backpressure that the new set() doesn't provide, error mapping in lift_subscribe_error and a CHANGELOG/version bump for the breaking API removals. Nothing that changes the approach; the approach is right.