Repository navigation
Fix: WhenAnyValue subscribes to PropertyChanged before reading the initial value - #4381
Conversation
…itial value ExpressionChainSink.Level.SetParent read the property and emitted the kicker value before subscribing to PropertyChanged. Any mutation that fired PropertyChanged between the two was raised against an empty handler list and silently lost, leaving downstream observers pinned to the pre-write value until the next change. Subscribe first, then read. A racing handler emit that arrives queued behind the chain sink's gate then delivers the same post-mutation value the kicker just emitted, so add a one-shot "suppress next emission if equal to the kicker" guard that fires independently of the user-facing isDistinct flag. Repro tests cover deterministic single-thread and multi-threaded stress paths. Fixes reactiveui#4380
dwcullop
changed the title
|
Thanks for the PR. I got a major refactor incoming (mostly all internal) where we swap over internally to using ReactiveUI.Primitives, basically still keeps IObservable but swaps out a lot of the internals. So would like to get this fix in before that refactor. We also have a source generated version of the WhenAny pipeline in the project in ReactiveUI.Binding.SourceGenerators mind having a peak and seeing if the issue is also there @dwcullop |
|
The fails are false negatives. Going to get your fixes in as part of the refactor. |
|
Happy to help. 😄 Let me know if you want a fix for the I'll take a look at the Binding SourceGen repo as well and see if I find any issues. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fixes #4380.
ExpressionChainSink.Level.SetParentreads the property and emits the kicker value before subscribing toPropertyChanged. Any mutation that firesPropertyChangedbetween the two is raised against an empty handler list and silently lost, leaving downstream observers pinned to the pre-write value until the next change. The race fires non-deterministically when the property can be written from a thread other than the one callingSubscribe(typical MVVM pattern: a view model aggregating data from background services).This swaps the order to subscribe-then-read. A racing handler emit that arrives queued behind the chain sink's gate then delivers the same post-mutation value the kicker just emitted, so a one-shot
_suppressNextIfSameAsLastguard collapses the single racing duplicate. The guard fires independently of the user-facingisDistinctflag, so existing distinct semantics are unchanged.Scope
The same defect exists in the released
23.2.28code (viaNestedObservedChanges.StartWith(kicker)thenSubscribe(NotifyForProperty)), but that code path is being replaced by the custom-reactive-sinks rework onmain. This PR fixes the defect in the rework's new location so the next release ships with the fix. Backporting to a23.2.xhotfix branch (if one is maintained) would be a separate change.Tests
In
WhenAnyValueSubscribeRaceTests:WhenAnyValue_MutationBetweenInitialEmitAndHandlerAttach_IsLost— deterministic single-thread repro onReactiveObject...._IsLost_PlainInpc— same shape with a hand-rolledINotifyPropertyChangedsource, to rule outReactiveObjectspecifics.WhenAnyValue_ConcurrentMutationDuringSubscribe_NeverLosesFinalValue_Stress— 2 000-iteration multi-threaded stress (~7.5% failure rate without the fix; 0% with it).All three fail without the fix and pass with it. Full
ReactiveUI.Testssuite (1 899 pre-existing + 3 new) passes locally onnet8.0.