fix: serialize PropertyObservable / PropertyChangingObservable initial-emit with concurrent handlers#39
Open
dwcullop wants to merge 1 commit into
Open
Conversation
…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.
4f6b4e7 to
d155de0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #38.
PropertyObservable<T>.Subscriptionattaches the source'sPropertyChangedhandler 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 firesPropertyChangedbetween 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. WithdistinctUntilChanged: truethe 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
EmitCurrenthelper called from both the constructor andOnPropertyChanged. 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_hasValuewas stillfalse) no longer produces a duplicate.PropertyChangingObservable<T>receives the sameEmitCurrentpattern for shape-consistency. Thefire-PropertyChanging-then-writesemantics 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 ofPropertyObservable<T>withdistinctUntilChanged: trueagainst a mutator thread writing the property 32 times.Subscribe_ConcurrentMutationDuringInitialEmit_DoesNotEmitConsecutiveDuplicateasserts 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_LastObservedValueMatchesPropertyasserts 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 syntheticPropertyChangedre-fire after a synchronization barrier so the assertion measures the subscription's behaviour rather than the orthogonal INPC event-accessor visibility race (the mutator'sPropertyChanged.Invokecan 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
Observablestests continue to pass.