Skip to content

fix: serialize PropertyObservable / PropertyChangingObservable initial-emit with concurrent handlers#39

Open
dwcullop wants to merge 1 commit into
reactiveui:mainfrom
dwcullop:bugfix/propertyobservable_race
Open

fix: serialize PropertyObservable / PropertyChangingObservable initial-emit with concurrent handlers#39
dwcullop wants to merge 1 commit into
reactiveui:mainfrom
dwcullop:bugfix/propertyobservable_race

Conversation

@dwcullop

@dwcullop dwcullop commented Jun 14, 2026

Copy link
Copy Markdown

Fixes #38.

PropertyObservable<T>.Subscription attaches the source's PropertyChanged handler before reading and emitting the initial value, but the constructor's read-state-emit sequence is not synchronized with the handler it just attached. A mutation on a background thread that fires PropertyChanged between the attach and the constructor's state writes produces two emissions of the same value: the racing handler sees _hasValue == false, skips the distinct check, and emits; the constructor then re-emits the same value with no dedup. With distinctUntilChanged: true the downstream observer expects no two consecutive equal values; the race lets one through. A related variant lands when the mutation falls between the constructor's read and its emit, in which case the constructor delivers a stale value after the handler's fresher emit.

This serializes the constructor's initial-emit and the handler's body under a shared per-instance gate, extracted into an EmitCurrent helper called from both the constructor and OnPropertyChanged. Any racing handler runs either fully before or fully after the constructor's emit. The distinct check now also applies on the initial emit, so the case where a racing handler emits first under the original distinct-skipping branch (taken because _hasValue was still false) no longer produces a duplicate.

PropertyChangingObservable<T> receives the same EmitCurrent pattern for shape-consistency. The fire-PropertyChanging-then-write semantics on a normal INPC setter make the analogous out-of-order case naturally unreachable on that type (handler runs synchronously and completes its emit before the setter writes, so the main thread can only read the new value after the handler emit is already through the downstream observer), but the gate keeps the two subscription types consistent and guards atypical user implementations.

Tests

Two multi-threaded stress tests in PropertyObservableSubscribeRaceProbeTests, each 5_000 iterations of PropertyObservable<T> with distinctUntilChanged: true against a mutator thread writing the property 32 times.

Subscribe_ConcurrentMutationDuringInitialEmit_DoesNotEmitConsecutiveDuplicate asserts the dedup contract directly: no consecutive duplicate emission ever reaches the downstream observer. Without the fix it produces roughly 100-300 iterations with a consecutive duplicate per run on my machine; with the fix it produces zero.

Subscribe_AfterSettlement_LastObservedValueMatchesProperty asserts the end-to-end invariant that the subscriber's last observed value equals the property's final value once everything has settled. The mutator does a synthetic PropertyChanged re-fire after a synchronization barrier so the assertion measures the subscription's behaviour rather than the orthogonal INPC event-accessor visibility race (the mutator's PropertyChanged.Invoke can otherwise read a stale snapshot of the delegate that omits the just-attached handler). With the fix this test passes; the test does not by itself differentiate fixed from unfixed code (duplicate emissions don't change the final value), so it documents the invariant rather than acting as a regression probe.

The 481 pre-existing Observables tests continue to pass.

@dwcullop dwcullop marked this pull request as ready for review June 14, 2026 10:09
…hanging handlers

PropertyObservable<T>.Subscription and PropertyChangingObservable<T>.Subscription
attached the source's change handler before reading and emitting the initial value
(which is the correct order, avoiding the lost-update race). But the constructor's
"read state and emit" sequence was not synchronized with the handler it just
attached, so a mutation on a background thread that fired PropertyChanged between
the attach and the constructor's state writes produced a duplicate emission
(violating distinctUntilChanged: true).

This serializes the constructor's read/state/emit and the handler's body under a
shared gate, so any racing handler runs either fully before or fully after the
constructor's initial emit. To collapse the case where a racing handler emits
first under the original distinct-skipping branch (taken because _hasValue was
still false at that moment), the constructor's emit now itself applies the
distinct check: if the value it just read matches what the racing handler already
delivered, it skips re-emitting.

PropertyChangingObservable receives the same gate so its handler and constructor
emit cannot interleave. The fix-PropertyChanging-then-write semantics make the
out-of-order case naturally unreachable on that type, but the gate keeps the two
subscription types consistent and guards atypical user implementations.

Includes a multi-threaded stress test for PropertyObservable that produces
~150-300 consecutive-duplicate emissions per 5_000 iterations without the fix
and zero with it.
@dwcullop dwcullop force-pushed the bugfix/propertyobservable_race branch from 4f6b4e7 to d155de0 Compare June 14, 2026 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: (Race condition) PropertyObservable<T> emits a duplicate value

1 participant