History consumes its Observer with no way to read it back, so every observer needs interior mutability #40

New Issue

2026-09-07T13:00:11Z

logaritmisk commented

2026-09-07 13:00:11 +00:00

Found while wiring up on_slice_processed in #21. The callback now fires, which makes a second problem visible: there is no ergonomic way to get at what the observer recorded.

The gap

HistoryBuilder::observer takes the observer by value (src/history.rs:125), stores it (:35, :192), and History exposes no accessor — no observer(), no into_observer(), no observer_mut(). Once handed over, the value is unreachable for the lifetime of the History.

Because Observer's methods all take &self, an observer that wants to accumulate anything needs interior mutability regardless. That part is fine and deliberate. The problem is the handle: the caller cannot keep one.

The obvious thing to reach for does not work:

let recorder = Arc::new(Recorder::default());
let mut h = History::builder()
    .observer(Arc::clone(&recorder))   // error[E0277]: the trait bound
    .build();                          // `Arc<Recorder>: Observer<i64>` is not satisfied

so callers must instead push the Arc inside the observer and make the observer itself Clone:

#[derive(Clone, Default)]
struct Recorder {
    slices: Arc<Mutex<Vec<(i64, usize, usize)>>>,
    // ...one Arc per field, forever
}

That is what tests/observer.rs does, and it works, but it is a workaround rather than a design: every field pays for its own allocation and lock, and the shape has to be rediscovered by each implementor. Nothing documents it.

What would fix it

Either of these, and they are not mutually exclusive:

impl<T: Time, O: Observer<T>> Observer<T> for Arc<O> — three lines, purely additive, no breakage. Makes the natural Arc::clone(&recorder) spelling compile and lets an observer hold plain fields behind one shared handle instead of one per field. Arc<O> is Send + Sync whenever O is, so the trait bound is already satisfied.
History::observer(&self) -> &O — also additive. Useful independently of the Arc question: it lets a caller inspect a configured observer, and pairs with an into_observer(self) -> O for reclaiming it after converge.

Option 1 is the smaller change and covers the common case. Option 2 is the more complete answer.

Worth also implementing the blanket impl for Box<O> and &O while in there, for the same reason — those are the other two shapes people reach for.

Acceptance

builder().observer(Arc::clone(&recorder)) compiles and the shared observer receives callbacks.
A test asserts that callbacks reach the caller's retained handle, not just some moved copy.
tests/observer.rs is simplified to whichever spelling lands, so the recommended pattern is the one demonstrated.

Not urgent

Nothing is broken — observation works today via the clone-with-Arc-fields pattern, and #21's tests prove the callbacks fire. This is an ergonomics and discoverability issue, and the fix is small and additive whenever someone wants it.

Found while wiring up `on_slice_processed` in #21. The callback now fires, which makes a second problem visible: there is no ergonomic way to *get at what the observer recorded*. ## The gap `HistoryBuilder::observer` takes the observer by value (`src/history.rs:125`), stores it (`:35`, `:192`), and `History` exposes no accessor — no `observer()`, no `into_observer()`, no `observer_mut()`. Once handed over, the value is unreachable for the lifetime of the `History`. Because `Observer`'s methods all take `&self`, an observer that wants to accumulate anything needs interior mutability regardless. That part is fine and deliberate. The problem is the *handle*: the caller cannot keep one. The obvious thing to reach for does not work: ```rust let recorder = Arc::new(Recorder::default()); let mut h = History::builder() .observer(Arc::clone(&recorder)) // error[E0277]: the trait bound .build(); // `Arc<Recorder>: Observer<i64>` is not satisfied ``` so callers must instead push the `Arc` *inside* the observer and make the observer itself `Clone`: ```rust #[derive(Clone, Default)] struct Recorder { slices: Arc<Mutex<Vec<(i64, usize, usize)>>>, // ...one Arc per field, forever } ``` That is what `tests/observer.rs` does, and it works, but it is a workaround rather than a design: every field pays for its own allocation and lock, and the shape has to be rediscovered by each implementor. Nothing documents it. ## What would fix it Either of these, and they are not mutually exclusive: 1. **`impl<T: Time, O: Observer<T>> Observer<T> for Arc<O>`** — three lines, purely additive, no breakage. Makes the natural `Arc::clone(&recorder)` spelling compile and lets an observer hold plain fields behind one shared handle instead of one per field. `Arc<O>` is `Send + Sync` whenever `O` is, so the trait bound is already satisfied. 2. **`History::observer(&self) -> &O`** — also additive. Useful independently of the `Arc` question: it lets a caller inspect a configured observer, and pairs with an `into_observer(self) -> O` for reclaiming it after `converge`. Option 1 is the smaller change and covers the common case. Option 2 is the more complete answer. Worth also implementing the blanket impl for `Box<O>` and `&O` while in there, for the same reason — those are the other two shapes people reach for. ## Acceptance - `builder().observer(Arc::clone(&recorder))` compiles and the shared observer receives callbacks. - A test asserts that callbacks reach the caller's retained handle, not just some moved copy. - `tests/observer.rs` is simplified to whichever spelling lands, so the recommended pattern is the one demonstrated. ## Not urgent Nothing is broken — observation works today via the clone-with-`Arc`-fields pattern, and #21's tests prove the callbacks fire. This is an ergonomics and discoverability issue, and the fix is small and additive whenever someone wants it.

logaritmisk referenced this issue from a commit

2026-09-07 13:13:35 +00:00

feat: let observers be shared, boxed, or borrowed

logaritmisk closed this issue

2026-09-07 13:13:35 +00:00

logaritmisk added the api enhancement labels 2026-09-07 13:53:43 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#40

History consumes its Observer with no way to read it back, so every observer needs interior mutability #40

The gap

What would fix it

Acceptance

Not urgent

`History` consumes its `Observer` with no way to read it back, so every observer needs interior mutability #40