diff --git a/.github/instructions/dynamicdata-cache.instructions.md b/.github/instructions/dynamicdata-cache.instructions.md index 45b1874bb..8965c07a0 100644 --- a/.github/instructions/dynamicdata-cache.instructions.md +++ b/.github/instructions/dynamicdata-cache.instructions.md @@ -826,6 +826,8 @@ Filters Update changes based on reference equality or a custom predicate. If fil | `WhenValueChanged(expr)` | Like above but emits just the property value (no sender). | | `WhenAnyPropertyChanged()` | Emits the item when **any** property changes (no specific property). | +If synchronous initialization fails, every event handler attached during that initialization is released. Subscriber callback exceptions propagate rather than becoming property-access errors. + --- ## Writing a New Cache Operator diff --git a/.github/instructions/dynamicdata-list.instructions.md b/.github/instructions/dynamicdata-list.instructions.md index 1f969b3ff..8b5dac176 100644 --- a/.github/instructions/dynamicdata-list.instructions.md +++ b/.github/instructions/dynamicdata-list.instructions.md @@ -500,6 +500,8 @@ myObservable.ToObservableChangeSet(expireAfter: item => TimeSpan.FromMinutes(5)) ### Property Observation +If synchronous initialization fails, every event handler attached during that initialization is released. Subscriber callback exceptions propagate rather than becoming property-access errors. + ```csharp // Observe a property on all items (requires INotifyPropertyChanged) list.Connect() diff --git a/.github/instructions/rx.instructions.md b/.github/instructions/rx.instructions.md index f5f3a21c8..fdd5be77e 100644 --- a/.github/instructions/rx.instructions.md +++ b/.github/instructions/rx.instructions.md @@ -262,6 +262,8 @@ primary.Dispose(); // decrement — resource still alive (dep2 still holds) dep2.Dispose(); // decrement to 0 — resource disposed! ``` +For event-based subscriptions, establish ownership before attaching handlers, reading user properties, or emitting initial values. Initialization can throw before `Observable.Create` receives the subscription disposable. A scoped `RefCountDisposable` can own activation and transfer a dependent lease to Rx only after activation succeeds, ensuring failed initialization releases every installed handler without replacing subscriber exceptions with `OnError`. + ### BooleanDisposable / CancellationDisposable ```csharp diff --git a/src/DynamicData.Tests/Binding/WhenPropertyChangedBehaviorFixture.Initialization.cs b/src/DynamicData.Tests/Binding/WhenPropertyChangedBehaviorFixture.Initialization.cs new file mode 100644 index 000000000..21e34479b --- /dev/null +++ b/src/DynamicData.Tests/Binding/WhenPropertyChangedBehaviorFixture.Initialization.cs @@ -0,0 +1,305 @@ +// Copyright (c) 2011-2025 Roland Pheasant. All rights reserved. +// Roland Pheasant licenses this file to you under the MIT license. +// See the LICENSE file in the project root for full license information. + +using System; +using System.ComponentModel; +using System.Linq; +using System.Reactive.Concurrency; +using System.Reactive.Linq; + +using DynamicData.Binding; +using DynamicData.Tests.Utilities; + +using FluentAssertions; + +using Xunit; + +namespace DynamicData.Tests.Binding; + +public sealed partial class WhenPropertyChangedBehaviorFixture +{ + /// Verifies that a throwing initial observer leaves no property-change handler attached. + [Fact] + public void Shallow_InitialObserverThrows_DetachesHandler() + { + // Arrange + var amount = _randomizer.Double(); + var model = new ObservablePrice { Amount = amount }; + var error = new InvalidOperationException(); + var results = new ValueRecordingObserver(ImmediateScheduler.Instance); + IObserver observer = results; + var source = model.WhenValueChanged(static price => price.Amount); + + // Act + Action subscribe = () => + { + using var subscription = source.Subscribe(value => + { + observer.OnNext(value); + throw error; + }, observer.OnError); + }; + + // Assert + subscribe.Should().Throw(because: "observer failures must escape Subscribe") + .Which.Should().BeSameAs(error, because: "the original observer failure must be preserved"); + results.RecordedValues.Should().Equal(new[] { amount }, because: "the failure occurs during initial delivery"); + results.Error.Should().BeNull(because: "an observer failure must not be converted into an OnError notification"); + model.WasSubscribed.Should().BeTrue(because: "registration must precede the initial value read"); + model.HandlerCount.Should().Be(0, because: "a throwing Subscribe cannot return a disposable to its caller"); + } + + /// Verifies that a throwing initial observer releases property-change handlers at every chain level. + [Fact] + public void DeepChain_InitialObserverThrows_DetachesEveryHandler() + { + // Arrange + var amount = _randomizer.Double(); + var leaf = new ObservablePrice { Amount = amount }; + var child = new ObservablePrice { Child = leaf }; + var root = new ObservablePrice { Child = child }; + var models = new[] { root, child, leaf }; + var error = new InvalidOperationException(); + var results = new ValueRecordingObserver(ImmediateScheduler.Instance); + IObserver observer = results; + var source = root.WhenValueChanged(static price => price.Child!.Child!.Amount); + + // Act + Action subscribe = () => + { + using var subscription = source.Subscribe(value => + { + observer.OnNext(value); + throw error; + }, observer.OnError); + }; + + // Assert + subscribe.Should().Throw(because: "observer failures must escape Subscribe") + .Which.Should().BeSameAs(error, because: "the original observer failure must be preserved"); + results.RecordedValues.Should().Equal(new[] { amount }, because: "the failure occurs during initial delivery"); + results.Error.Should().BeNull(because: "an observer failure must not be converted into an OnError notification"); + models.Should().OnlyContain(model => model.WasSubscribed, because: "each observable level must be registered before it is read"); + models.Select(model => model.HandlerCount).Should().OnlyContain(count => count == 0, + because: "failed initialization must release every handler, not just the root handler"); + } + + /// Verifies that an initial getter failure releases the handler when the default error handler throws. + [Fact] + public void Shallow_InitialGetterThrows_DefaultErrorHandler_DetachesHandler() + { + // Arrange + var error = new InvalidOperationException(); + var model = new ObservablePrice { Amount = _randomizer.Double(), ReadError = error }; + var source = model.WhenValueChanged(static price => price.Amount); + + // Act + Action subscribe = () => + { + using var subscription = source.Subscribe(); + }; + + // Assert + subscribe.Should().Throw(because: "the default Rx error handler must rethrow the getter failure") + .Which.Should().BeSameAs(error, because: "the original getter failure must be preserved"); + model.WasSubscribed.Should().BeTrue(because: "registration must precede the initial value read"); + model.HandlerCount.Should().Be(0, because: "failed initialization must not retain the event handler"); + } + + /// Verifies that a failing chain getter releases every handler when the default error handler throws. + /// Whether subscribing requests an initial value notification. + /// Whether an intermediate getter fails before the leaf can be subscribed. + [Theory] + [InlineData(true, false)] + [InlineData(true, true)] + [InlineData(false, true)] + public void DeepChain_InitialGetterThrows_DefaultErrorHandler_DetachesEveryHandler(bool notifyOnInitialValue, bool failBeforeLeaf) + { + // Arrange + var error = new InvalidOperationException(); + var leaf = new ObservablePrice { Amount = _randomizer.Double(), ReadError = failBeforeLeaf ? null : error }; + var child = new ObservablePrice { Child = leaf, ChildReadError = failBeforeLeaf ? error : null }; + var root = new ObservablePrice { Child = child }; + var models = new[] { root, child, leaf }; + var source = root.WhenValueChanged(static price => price.Child!.Child!.Amount, notifyOnInitialValue); + + // Act + Action subscribe = () => + { + using var subscription = source.Subscribe(); + }; + + // Assert + subscribe.Should().Throw(because: "the default Rx error handler must rethrow initialization failures") + .Which.GetBaseException().Should().BeSameAs(error, because: "the failure must originate in the observed getter"); + root.WasSubscribed.Should().BeTrue(because: "the root handler must attach before its child is read"); + child.WasSubscribed.Should().BeTrue(because: "the intermediate handler must attach before its child is read"); + leaf.WasSubscribed.Should().Be(!failBeforeLeaf, because: "the leaf is reachable only if the intermediate getter succeeds"); + models.Select(model => model.HandlerCount).Should().OnlyContain(count => count == 0, + because: "failed initialization must release handlers at every visited level"); + } + + /// Verifies that a handled initial getter failure terminates observation and releases its event handler. + [Fact] + public void Shallow_InitialGetterThrows_ErrorIsRecordedAndHandlerDetached() + { + // Arrange + var error = new InvalidOperationException(); + var model = new ObservablePrice { Amount = _randomizer.Double(), ReadError = error }; + + // Act + using var subscription = model.WhenPropertyChanged(static price => price.Amount) + .RecordValues(out var results); + + // Assert + results.Error.Should().BeSameAs(error, because: "getter failures must be delivered through OnError"); + results.RecordedValues.Should().BeEmpty(because: "the initial getter did not produce a value"); + results.HasCompleted.Should().BeFalse(because: "OnError is the terminal notification"); + model.WasSubscribed.Should().BeTrue(because: "registration must precede the initial value read"); + model.HandlerCount.Should().Be(0, because: "OnError must release the handler before Subscribe returns"); + } + + /// Verifies that a handled chain getter failure terminates observation and releases every event handler. + /// Whether subscribing requests an initial value notification. + /// Whether an intermediate getter fails before the leaf can be subscribed. + [Theory] + [InlineData(true, false)] + [InlineData(true, true)] + [InlineData(false, true)] + public void DeepChain_InitialGetterThrows_ErrorIsRecordedAndEveryHandlerDetached(bool notifyOnInitialValue, bool failBeforeLeaf) + { + // Arrange + var error = new InvalidOperationException(); + var leaf = new ObservablePrice { Amount = _randomizer.Double(), ReadError = failBeforeLeaf ? null : error }; + var child = new ObservablePrice { Child = leaf, ChildReadError = failBeforeLeaf ? error : null }; + var root = new ObservablePrice { Child = child }; + var models = new[] { root, child, leaf }; + + // Act + using var subscription = root.WhenPropertyChanged(static price => price.Child!.Child!.Amount, notifyOnInitialValue) + .RecordValues(out var results); + + // Assert + results.Error.Should().NotBeNull(because: "chain getter failures must be delivered through OnError"); + results.Error!.GetBaseException().Should().BeSameAs(error, because: "the failure must originate in the observed getter"); + results.RecordedValues.Should().BeEmpty(because: "the chain did not produce an obtainable value"); + results.HasCompleted.Should().BeFalse(because: "OnError is the terminal notification"); + root.WasSubscribed.Should().BeTrue(because: "the root handler must attach before its child is read"); + child.WasSubscribed.Should().BeTrue(because: "the intermediate handler must attach before its child is read"); + leaf.WasSubscribed.Should().Be(!failBeforeLeaf, because: "the leaf is reachable only if the intermediate getter succeeds"); + models.Select(model => model.HandlerCount).Should().OnlyContain(count => count == 0, + because: "OnError must release every handler before Subscribe returns"); + } + + /// Verifies that live property handlers belong to the returned subscription until it is disposed. + /// Whether the observed property is reached through intermediate objects. + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Subscription_ExplicitDisposal_ReleasesHandlers(bool deepChain) + { + // Arrange + var amount = _randomizer.Double(); + var leaf = new ObservablePrice { Amount = amount }; + var child = new ObservablePrice { Child = leaf }; + var root = new ObservablePrice { Child = child }; + var models = deepChain ? new[] { root, child, leaf } : new[] { leaf }; + var source = deepChain + ? root.WhenValueChanged(static price => price.Child!.Child!.Amount) + : leaf.WhenValueChanged(static price => price.Amount); + using var subscription = source.RecordValues(out var results); + var attachedHandlerCounts = models.Select(model => model.HandlerCount).ToArray(); + + // Act + subscription.Dispose(); + + // Assert + attachedHandlerCounts.Should().OnlyContain(count => count == 1, because: "each visited object must stay subscribed after initialization"); + models.Select(model => model.HandlerCount).Should().OnlyContain(count => count == 0, + because: "disposing the returned subscription must release every retained handler"); + results.RecordedValues.Should().Equal(new[] { amount }, because: "initialization must publish the observed value"); + results.Error.Should().BeNull(because: "explicit disposal is not an observation failure"); + results.HasCompleted.Should().BeFalse(because: "unsubscribing does not publish a completion notification"); + } + + /// Verifies that synchronous completion during initial delivery releases every property handler. + /// Whether the observed property is reached through intermediate objects. + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Subscription_TakeInitialValue_ReleasesHandlers(bool deepChain) + { + // Arrange + var amount = _randomizer.Double(); + var leaf = new ObservablePrice { Amount = amount }; + var child = new ObservablePrice { Child = leaf }; + var root = new ObservablePrice { Child = child }; + var models = deepChain ? new[] { root, child, leaf } : new[] { leaf }; + var source = deepChain + ? root.WhenValueChanged(static price => price.Child!.Child!.Amount) + : leaf.WhenValueChanged(static price => price.Amount); + + // Act + using var subscription = source.Take(1) + .RecordValues(out var results); + + // Assert + results.RecordedValues.Should().Equal(new[] { amount }, because: "the requested initial value must be delivered"); + results.Error.Should().BeNull(because: "taking an initial value is normal completion"); + results.HasCompleted.Should().BeTrue(because: "Take completes after receiving its requested value"); + models.Should().OnlyContain(model => model.WasSubscribed, because: "handlers must attach before initial delivery"); + models.Select(model => model.HandlerCount).Should().OnlyContain(count => count == 0, + because: "synchronous completion must release handlers before Subscribe returns"); + } + + /// An observable input whose custom event accessors expose property subscription lifetimes. + public sealed class ObservablePrice : INotifyPropertyChanged + { + private double _amount; + private ObservablePrice? _child; + private PropertyChangedEventHandler? _propertyChanged; + + /// + public event PropertyChangedEventHandler? PropertyChanged + { + add + { + WasSubscribed = true; + _propertyChanged += value; + } + + remove => _propertyChanged -= value; + } + + /// Gets or sets the amount and raises a property-change notification when set. + public double Amount + { + get => ReadError is null ? _amount : throw ReadError; + set + { + _amount = value; + _propertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Amount))); + } + } + + /// Gets the next object in a nested property path. + public ObservablePrice? Child + { + get => ChildReadError is null ? _child : throw ChildReadError; + init => _child = value; + } + + /// Gets an optional failure raised when reading . + public InvalidOperationException? ChildReadError { get; init; } + + /// Gets the number of event handlers retained by this object. + public int HandlerCount => _propertyChanged?.GetInvocationList().Length ?? 0; + + /// Gets an optional failure raised when reading . + public InvalidOperationException? ReadError { get; init; } + + /// Gets whether any observer has registered a property-change handler. + public bool WasSubscribed { get; private set; } + } +} diff --git a/src/DynamicData.Tests/Binding/WhenPropertyChangedBehaviorFixture.cs b/src/DynamicData.Tests/Binding/WhenPropertyChangedBehaviorFixture.cs index 718df9537..1ce5fcde7 100644 --- a/src/DynamicData.Tests/Binding/WhenPropertyChangedBehaviorFixture.cs +++ b/src/DynamicData.Tests/Binding/WhenPropertyChangedBehaviorFixture.cs @@ -6,20 +6,35 @@ using System.Collections.Generic; using System.ComponentModel; using System.Linq; + +using Bogus; + using DynamicData.Binding; using DynamicData.Tests.Utilities; using FluentAssertions; using Xunit; +using Xunit.Abstractions; namespace DynamicData.Tests.Binding; /// /// Single-threaded contract tests for : -/// handler attachment ordering, no-dedup semantics, deep-chain re-walks on swaps. +/// handler attachment ordering, subscription cleanup, no-dedup semantics, and deep-chain swaps. /// -public sealed class WhenPropertyChangedBehaviorFixture +public sealed partial class WhenPropertyChangedBehaviorFixture { + private readonly Randomizer _randomizer; + + /// Initializes deterministic inputs for property-observation contracts. + /// Receives the seed used to generate test inputs. + public WhenPropertyChangedBehaviorFixture(ITestOutputHelper output) + { + const int seed = 0x35C1_709B; + _randomizer = new Randomizer(seed); + output.WriteLine($"{nameof(WhenPropertyChangedBehaviorFixture)} seed: 0x{seed:X8}"); + } + [Fact] public void Shallow_NotifyInitialFalse_SubscribesHandlerBeforeReturning() { diff --git a/src/DynamicData/Binding/NotifyPropertyChangedEx.cs b/src/DynamicData/Binding/NotifyPropertyChangedEx.cs index 3c3ac0924..7d547b2bb 100644 --- a/src/DynamicData/Binding/NotifyPropertyChangedEx.cs +++ b/src/DynamicData/Binding/NotifyPropertyChangedEx.cs @@ -1,4 +1,4 @@ -// Copyright (c) 2011-2025 Roland Pheasant. All rights reserved. +// Copyright (c) 2011-2025 Roland Pheasant. All rights reserved. // Roland Pheasant licenses this file to you under the MIT license. // See the LICENSE file in the project root for full license information. @@ -245,6 +245,12 @@ public static class NotifyPropertyChangedEx /// For an object like Parent.Child.Sibling, sibling is an object so if Child is null, the value null and obtainable and is returned as null. /// A observable which also notifies when the property value changes. /// propertyAccessor. + /// + /// Event handlers installed during subscription are detached if initialization fails before an + /// can be returned. Exceptions thrown by subscriber callbacks propagate; + /// they are not converted into property-access error notifications. + /// + /// public static IObservable> WhenPropertyChanged(this TObject source, Expression> propertyAccessor, bool notifyOnInitialValue = true, Func? fallbackValue = null) where TObject : INotifyPropertyChanged { @@ -267,6 +273,11 @@ public static IObservable> WhenPropertyChanged /// For example when observing Parent.Child.Age, if Child is null the value is unobtainable as Age is a struct and cannot be set to Null. /// For an object like Parent.Child.Sibling, sibling is an object so if Child is null, the value null and obtainable and is returned as null. /// An observable which emits the results. + /// + /// Supports the initialization cleanup described by + /// . + /// + /// public static IObservable WhenValueChanged(this TObject source, Expression> propertyAccessor, bool notifyOnInitialValue = true, Func? fallbackValue = null) where TObject : INotifyPropertyChanged { diff --git a/src/DynamicData/Binding/ObservablePropertyFactory.cs b/src/DynamicData/Binding/ObservablePropertyFactory.cs index 4dd3fbd68..4fd8fc4a1 100644 --- a/src/DynamicData/Binding/ObservablePropertyFactory.cs +++ b/src/DynamicData/Binding/ObservablePropertyFactory.cs @@ -22,8 +22,16 @@ public ObservablePropertyFactory(Func valueAccessor, Observa { // chain is leaf-first (output of SplitIntoSteps). Reverse once to root-to-leaf order. var rootToLeaf = chain.AsEnumerable().Reverse().ToArray(); - _factory = (source, notifyInitial) => Observable.Create>( - observer => new DeepChainSubscription(observer, source, rootToLeaf, valueAccessor, notifyInitial)); + _factory = (source, notifyInitial) => Observable.Create>(observer => + { + var subscription = new DeepChainSubscription(observer, source, rootToLeaf, valueAccessor, notifyInitial); + + // The scope owns initialization; only successful activation hands a dependent lease to Rx. + using var lifetime = new RefCountDisposable(subscription); + subscription.Start(); + + return lifetime.GetDisposable(); + }); } public ObservablePropertyFactory(Expression> expression) @@ -33,8 +41,16 @@ public ObservablePropertyFactory(Expression> expression // the high-frequency single-property hot path. var memberName = expression.GetProperty().Name; var accessor = expression.Compile(); - _factory = (source, notifyInitial) => Observable.Create>( - observer => new SinglePropertySubscription(observer, source, memberName, accessor, notifyInitial)); + _factory = (source, notifyInitial) => Observable.Create>(observer => + { + var subscription = new SinglePropertySubscription(observer, source, memberName, accessor); + + // The scope releases the handler if activation throws before Rx can receive its lease. + using var lifetime = new RefCountDisposable(subscription); + subscription.Start(notifyInitial); + + return lifetime.GetDisposable(); + }); } public IObservable> Create(TObject source, bool notifyInitial) => _factory(source, notifyInitial); @@ -43,7 +59,7 @@ public ObservablePropertyFactory(Expression> expression // event through a DeliveryQueue. Used for x => x.Prop (depth == 1) where SharedDeliveryQueue // and Observable.FromEventPattern would be needless overhead on the hot path. // - // notifyInitial only controls whether the constructor synthesises an initial emission. There + // notifyInitial only controls whether Start synthesises an initial emission. There // is no equality dedup at the subscribe seam: a same-valued PropertyChanged firing in the // subscribe window is a legitimate event and must be delivered. The "never drop events" // contract takes precedence over avoiding a benign duplicate. @@ -58,15 +74,23 @@ public SinglePropertySubscription( IObserver> observer, TObject source, string memberName, - Func accessor, - bool notifyInitial) + Func accessor) { _source = source; _memberName = memberName; _accessor = accessor; _queue = new DeliveryQueue>(observer); + } - // Attach PropertyChanged handler FIRST so events during the initial read are not missed. + public void Dispose() + { + _source.PropertyChanged -= OnPropertyChanged; + _queue.Dispose(); + } + + public void Start(bool notifyInitial) + { + // Attach before reading so changes during initialization are captured. _source.PropertyChanged += OnPropertyChanged; if (notifyInitial) @@ -75,12 +99,6 @@ public SinglePropertySubscription( } } - public void Dispose() - { - _source.PropertyChanged -= OnPropertyChanged; - _queue.Dispose(); - } - private void OnPropertyChanged(object? sender, PropertyChangedEventArgs args) { if (args.PropertyName == _memberName) @@ -186,12 +204,6 @@ public DeepChainSubscription( var level = i; _levelCallbacks[i] = _ => _signalSub.OnNext(level); } - - // Kick off initial chain setup via the drainer. The subscribe thread becomes the - // drainer (no one else is draining yet on a fresh subscription) and runs - // ProcessSignal(InitialSetupSignal) synchronously, which attaches the chain and - // emits the initial value. - _signalSub.OnNext(InitialSetupSignal); } public void Dispose() @@ -206,6 +218,9 @@ public void Dispose() _sharedQueue.Dispose(); } + // Initial setup uses the same delivery ordering and reentrancy as subsequent changes. + public void Start() => _signalSub.OnNext(InitialSetupSignal); + private void ProcessSignal(int level) { // Drainer thread. The chain walk (Invoker / notifier Factory / ReadCurrent's accessor)