From 34576920ad84fd1adae6902c8e2bb5764b3abf32 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Levesque Date: Fri, 15 Dec 2023 11:54:59 -0500 Subject: [PATCH] fix: DynamicProperties no longer throw an ObjectDisposedException when we set their Value while they're disposed. --- BREAKING_CHANGES.md | 11 +++++ .../Property/DynamicPropertyFactoryTests.cs | 45 ------------------- .../Property/DynamicPropertyTests.T.cs | 32 +++++++------ .../Property/DynamicPropertyTests.cs | 30 ++++++------- src/DynamicMvvm/LoggerMessages.cs | 3 ++ src/DynamicMvvm/Property/DynamicProperty.T.cs | 3 +- src/DynamicMvvm/Property/DynamicProperty.cs | 22 ++------- .../Property/DynamicPropertyFactory.cs | 11 ++--- .../Property/DynamicPropertyFromObservable.cs | 10 +---- .../Property/DynamicPropertyFromTask.cs | 11 +---- ...eChangedOnBackgroundTaskDynamicProperty.cs | 26 +++-------- ...dOnBackgroundTaskDynamicPropertyFactory.cs | 11 ++--- ...groundTaskDynamicPropertyFromObservable.cs | 10 +---- ...OnBackgroundTaskDynamicPropertyFromTask.cs | 11 +---- 14 files changed, 75 insertions(+), 161 deletions(-) diff --git a/BREAKING_CHANGES.md b/BREAKING_CHANGES.md index aeb3764..592b60e 100644 --- a/BREAKING_CHANGES.md +++ b/BREAKING_CHANGES.md @@ -1,8 +1,19 @@ # Breaking Changes +## 1.4.1 +- Dynamic properties no longer throw an `ObjectDisposedException` when we set their `Value` while they're disposed. + - We've discovered that this safeguard is not needed and was causing unjustified issues when using dynamic properties in a multi-threaded environment. This is especially true with the _DynamicPropertyFromObservable_ variant, which can easily be disposed from a different thread than the one the observable source uses. + - This change renders obsolete the `throwOnDisposed` parameter used in several constructors of `IDynamicProperty` and `IDynamicPropertyFactory` implementations. + Those overloads are still present in the library, but they are marked as obsolete and will be removed in a future version. + - You can still observe the events where a dynamic property is set while it's disposed by using logs. The event id is 32, the log level is `Debug`, and the message template is `"Skipped value setter on the property '{PropertyName}' because it's disposed."` + +This breaking changes doesn't change the API definition. + ## 1.3.0 - The NuGet reference to `Microsoft.Extensions.Logging.Abstractions` now requires version 6.0.0 and up. +This breaking change doesn't change the API definition. + ## 0.11.0 - `DecoratorCommandStrategy` not longer exists. Use `DelegatingCommandStrategy` instead. diff --git a/src/DynamicMvvm.Tests/Property/DynamicPropertyFactoryTests.cs b/src/DynamicMvvm.Tests/Property/DynamicPropertyFactoryTests.cs index 10b0817..b2fd15d 100644 --- a/src/DynamicMvvm.Tests/Property/DynamicPropertyFactoryTests.cs +++ b/src/DynamicMvvm.Tests/Property/DynamicPropertyFactoryTests.cs @@ -80,50 +80,5 @@ public void It_Creates_From_Task_With_Value() property.Value.Should().Be(myValue); } - - [Theory] - [InlineData(true)] - [InlineData(false)] - public void It_Creates_Property_That_Throws_On_Set_When_Disposed_Only_If_ThrowOnDisposed_Is_True(bool throwOnDisposed) - { - // Arrange - var factory = new DynamicPropertyFactory(throwOnDisposed); - - var taskProperty = factory.CreateFromTask(DefaultPropertyName, _ => Task.FromResult(new TestEntity()), new TestEntity()); - var obProperty = factory.CreateFromObservable(DefaultPropertyName, new Subject(), new TestEntity()); - var property = factory.Create(DefaultPropertyName, new TestEntity()); - - // Act - taskProperty.Dispose(); - obProperty.Dispose(); - property.Dispose(); - - Action actTask = () => - { - taskProperty.Value = new TestEntity(); - }; - Action actOb = () => - { - obProperty.Value = new TestEntity(); - }; - Action actP = () => - { - property.Value = new TestEntity(); - }; - - // Assert - if (throwOnDisposed) - { - actTask.Should().Throw(); - actOb.Should().Throw(); - actP.Should().Throw(); - } - else - { - actTask.Should().NotThrow(); - actOb.Should().NotThrow(); - actP.Should().NotThrow(); - } - } } } diff --git a/src/DynamicMvvm.Tests/Property/DynamicPropertyTests.T.cs b/src/DynamicMvvm.Tests/Property/DynamicPropertyTests.T.cs index ddc7669..c747d2f 100644 --- a/src/DynamicMvvm.Tests/Property/DynamicPropertyTests.T.cs +++ b/src/DynamicMvvm.Tests/Property/DynamicPropertyTests.T.cs @@ -79,14 +79,12 @@ void OnValueChanged(IDynamicProperty p) } } - [Theory] - [InlineData(true)] - [InlineData(false)] - public void It_Throws_When_Value_Set_After_Disposed_Only_If_ThrowOnDisposed_Is_True(bool throwOnDisposed) + [Fact] + public void It_Doesnt_Throw_ObjectDisposedException_Upon_Setting_Value_While_Disposed() { // Arrange var receivedValues = new List(); - var property = new DynamicProperty(DefaultPropertyName, throwOnDisposed: throwOnDisposed); + var property = new DynamicProperty(DefaultPropertyName); // Act property.Dispose(); @@ -96,14 +94,22 @@ public void It_Throws_When_Value_Set_After_Disposed_Only_If_ThrowOnDisposed_Is_T }; // Assert - if (throwOnDisposed) - { - act.Should().Throw(); - } - else - { - act.Should().NotThrow(); - } + act.Should().NotThrow(); + } + + [Fact] + public void It_Doesnt_Change_Value_When_Disposed() + { + // Arrange + var myValue = new TestEntity(); + var property = new DynamicProperty(DefaultPropertyName, myValue); + + // Act + property.Dispose(); + property.Value = new TestEntity(); + + // Assert + property.Value.Should().Be(myValue); } [Fact] diff --git a/src/DynamicMvvm.Tests/Property/DynamicPropertyTests.cs b/src/DynamicMvvm.Tests/Property/DynamicPropertyTests.cs index cd2d1fc..6e15dce 100644 --- a/src/DynamicMvvm.Tests/Property/DynamicPropertyTests.cs +++ b/src/DynamicMvvm.Tests/Property/DynamicPropertyTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Chinook.DynamicMvvm.Tests.Helpers; using FluentAssertions; using Xunit; @@ -78,13 +79,12 @@ void OnValueChanged(IDynamicProperty p) } } - [Theory] - [InlineData(true)] - [InlineData(false)] - public void It_Throws_When_Value_Set_After_Disposed_Only_If_ThrowOnDisposed_Is_True(bool throwOnDisposed) + [Fact] + public void It_Doesnt_Throw_ObjectDisposedException_Upon_Setting_Value_While_Disposed() { // Arrange - var property = new DynamicProperty(DefaultPropertyName, throwOnDisposed: throwOnDisposed); + var receivedValues = new List(); + var property = new DynamicProperty(DefaultPropertyName); // Act property.Dispose(); @@ -94,24 +94,22 @@ public void It_Throws_When_Value_Set_After_Disposed_Only_If_ThrowOnDisposed_Is_T }; // Assert - if (throwOnDisposed) - { - act.Should().Throw(); - } - else - { - act.Should().NotThrow(); - } + act.Should().NotThrow(); } [Fact] - public void It_Throws_When_Value_Set_After_Disposed() + public void It_Doesnt_Change_Value_When_Disposed() { - var property = new DynamicProperty(DefaultPropertyName); + // Arrange + var myValue = new object(); + var property = new DynamicProperty(DefaultPropertyName, myValue); + // Act property.Dispose(); + property.Value = new object(); - Assert.Throws(() => property.Value = new object()); + // Assert + property.Value.Should().Be(myValue); } [Fact] diff --git a/src/DynamicMvvm/LoggerMessages.cs b/src/DynamicMvvm/LoggerMessages.cs index a8e0ba5..ae292f8 100644 --- a/src/DynamicMvvm/LoggerMessages.cs +++ b/src/DynamicMvvm/LoggerMessages.cs @@ -61,6 +61,9 @@ internal static partial class LoggerMessages [LoggerMessage(31, LogLevel.Error, "Source task failed for property '{PropertyName}'.")] public static partial void LogDynamicPropertySourceTaskFailed(this ILogger logger, string propertyName, Exception exception); + [LoggerMessage(32, LogLevel.Debug, "Skipped value setter on the property '{PropertyName}' because it's disposed.")] + public static partial void LogDynamicPropertySkippedValueSetterBecauseDisposed(this ILogger logger, string propertyName); + [LoggerMessage(40, LogLevel.Debug, "Deactivated observable source of property '{PropertyName}'.")] public static partial void LogDeactivatedObservableSource(this ILogger logger, string propertyName); diff --git a/src/DynamicMvvm/Property/DynamicProperty.T.cs b/src/DynamicMvvm/Property/DynamicProperty.T.cs index 5c04208..fcd425b 100644 --- a/src/DynamicMvvm/Property/DynamicProperty.T.cs +++ b/src/DynamicMvvm/Property/DynamicProperty.T.cs @@ -26,8 +26,9 @@ public DynamicProperty(string name, T value = default) /// Name /// Initial value /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public DynamicProperty(string name, bool throwOnDisposed, T value = default) - : base(name, throwOnDisposed, value) + : base(name, value) { } diff --git a/src/DynamicMvvm/Property/DynamicProperty.cs b/src/DynamicMvvm/Property/DynamicProperty.cs index 89f0607..381e9a2 100644 --- a/src/DynamicMvvm/Property/DynamicProperty.cs +++ b/src/DynamicMvvm/Property/DynamicProperty.cs @@ -9,7 +9,6 @@ namespace Chinook.DynamicMvvm public class DynamicProperty : IDynamicProperty { private static readonly DiagnosticSource _diagnostics = new DiagnosticListener("Chinook.DynamicMvvm.IDynamicProperty"); - private readonly bool _throwOnDisposed; private object _value; private bool _isDisposed; @@ -31,7 +30,6 @@ public DynamicProperty(string name, object value = default) { _diagnostics.Write("Created", Name); } - _throwOnDisposed = true; } /// @@ -40,16 +38,10 @@ public DynamicProperty(string name, object value = default) /// Name /// Initial value /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public DynamicProperty(string name, bool throwOnDisposed, object value = default) + : this(name, value) { - Name = name; - _value = value; - - if (_diagnostics.IsEnabled("Created")) - { - _diagnostics.Write("Created", Name); - } - _throwOnDisposed = throwOnDisposed; } /// @@ -63,14 +55,8 @@ public object Value { if (_isDisposed) { - if (_throwOnDisposed) - { - throw new ObjectDisposedException(Name); - } - else - { - return; - } + this.Log().LogDynamicPropertySkippedValueSetterBecauseDisposed(Name); + return; } if (!Equals(value, _value)) diff --git a/src/DynamicMvvm/Property/DynamicPropertyFactory.cs b/src/DynamicMvvm/Property/DynamicPropertyFactory.cs index eac887d..2b743cf 100644 --- a/src/DynamicMvvm/Property/DynamicPropertyFactory.cs +++ b/src/DynamicMvvm/Property/DynamicPropertyFactory.cs @@ -13,8 +13,6 @@ namespace Chinook.DynamicMvvm [Preserve(AllMembers = true)] public class DynamicPropertyFactory : IDynamicPropertyFactory { - private readonly bool _throwOnDisposed; - /// /// Initializes a new instance of the class. /// @@ -23,28 +21,27 @@ public class DynamicPropertyFactory : IDynamicPropertyFactory /// public DynamicPropertyFactory() { - _throwOnDisposed = true; } /// /// Initializes a new instance of the class. /// /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public DynamicPropertyFactory(bool throwOnDisposed) { - _throwOnDisposed = throwOnDisposed; } /// public virtual IDynamicProperty Create(string name, T initialValue = default, IViewModel viewModel = null) - => new DynamicProperty(name, _throwOnDisposed, initialValue); + => new DynamicProperty(name, initialValue); /// public virtual IDynamicProperty CreateFromTask(string name, Func> source, T initialValue = default, IViewModel viewModel = null) - => new DynamicPropertyFromTask(name, source, _throwOnDisposed, initialValue); + => new DynamicPropertyFromTask(name, source, initialValue); /// public virtual IDynamicProperty CreateFromObservable(string name, IObservable source, T initialValue = default, IViewModel viewModel = null) - => new DynamicPropertyFromObservable(name, source, _throwOnDisposed, initialValue); + => new DynamicPropertyFromObservable(name, source, initialValue); } } diff --git a/src/DynamicMvvm/Property/DynamicPropertyFromObservable.cs b/src/DynamicMvvm/Property/DynamicPropertyFromObservable.cs index f60bd55..c99bfcd 100644 --- a/src/DynamicMvvm/Property/DynamicPropertyFromObservable.cs +++ b/src/DynamicMvvm/Property/DynamicPropertyFromObservable.cs @@ -43,16 +43,10 @@ public DynamicPropertyFromObservable(string name, IObservable source, T initi /// Source /// Initial value /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public DynamicPropertyFromObservable(string name, IObservable source, bool throwOnDisposed, T initialValue = default) - : base(name, throwOnDisposed, initialValue) + : this(name, source, initialValue) { - if (source is null) - { - throw new ArgumentNullException(nameof(source)); - } - - _propertyObserver = new DynamicPropertyObserver(this); - _subscription = source.Subscribe(_propertyObserver); } /// diff --git a/src/DynamicMvvm/Property/DynamicPropertyFromTask.cs b/src/DynamicMvvm/Property/DynamicPropertyFromTask.cs index e03b3df..88b30ac 100644 --- a/src/DynamicMvvm/Property/DynamicPropertyFromTask.cs +++ b/src/DynamicMvvm/Property/DynamicPropertyFromTask.cs @@ -43,17 +43,10 @@ public DynamicPropertyFromTask(string name, Func> sou /// Source /// Initial value /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public DynamicPropertyFromTask(string name, Func> source, bool throwOnDisposed, T initialValue = default) - : base(name, throwOnDisposed, initialValue) + : this(name, source, initialValue) { - if (source is null) - { - throw new ArgumentNullException(nameof(source)); - } - - _cancellationTokenSource = new CancellationTokenSource(); - - _ = SetValueFromSource(_cancellationTokenSource.Token, source); } private async Task SetValueFromSource(CancellationToken ct, Func> source) diff --git a/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicProperty.cs b/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicProperty.cs index f7bfdc3..4f053f9 100644 --- a/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicProperty.cs +++ b/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicProperty.cs @@ -11,7 +11,6 @@ public class ValueChangedOnBackgroundTaskDynamicProperty : IDynamicProperty { private static readonly DiagnosticSource _diagnostics = new DiagnosticListener("Chinook.DynamicMvvm.IDynamicProperty"); private readonly WeakReference _viewModel; - private readonly bool _throwOnDisposed; private object _value; private bool _isDisposed; @@ -35,7 +34,6 @@ public ValueChangedOnBackgroundTaskDynamicProperty(string name, IViewModel viewM { _diagnostics.Write("Created", Name); } - _throwOnDisposed = true; } /// @@ -45,17 +43,10 @@ public ValueChangedOnBackgroundTaskDynamicProperty(string name, IViewModel viewM /// The used to determine dispatcher access. /// The initial value of this property. /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public ValueChangedOnBackgroundTaskDynamicProperty(string name, IViewModel viewModel, bool throwOnDisposed, object value = default) + : this(name, viewModel, value) { - Name = name; - _viewModel = new WeakReference(viewModel); - _value = value; - - if (_diagnostics.IsEnabled("Created")) - { - _diagnostics.Write("Created", Name); - } - _throwOnDisposed = throwOnDisposed; } protected bool IsOnDispatcher => _viewModel.TryGetTarget(out var vm) && (vm.Dispatcher?.GetHasDispatcherAccess() ?? false); @@ -71,14 +62,8 @@ public virtual object Value { if (_isDisposed) { - if (_throwOnDisposed) - { - throw new ObjectDisposedException(Name); - } - else - { - return; - } + this.Log().LogDynamicPropertySkippedValueSetterBecauseDisposed(Name); + return; } if (!Equals(value, _value)) @@ -174,8 +159,9 @@ public ValueChangedOnBackgroundTaskDynamicProperty(string name, IViewModel viewM /// The used to determine dispatcher access. /// The initial value of this property. /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public ValueChangedOnBackgroundTaskDynamicProperty(string name, IViewModel viewModel, bool throwOnDisposed, T value = default) - : base(name, viewModel, throwOnDisposed, value) + : base(name, viewModel, value) { } diff --git a/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFactory.cs b/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFactory.cs index 7506359..8a65b03 100644 --- a/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFactory.cs +++ b/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFactory.cs @@ -10,8 +10,6 @@ namespace Chinook.DynamicMvvm.Implementations [Preserve(AllMembers = true)] public class ValueChangedOnBackgroundTaskDynamicPropertyFactory : IDynamicPropertyFactory { - private readonly bool _throwOnDisposed; - /// /// Initializes a new instance of the class. /// @@ -20,34 +18,33 @@ public class ValueChangedOnBackgroundTaskDynamicPropertyFactory : IDynamicProper /// public ValueChangedOnBackgroundTaskDynamicPropertyFactory() { - _throwOnDisposed = true; } /// /// Initializes a new instance of the class. /// /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public ValueChangedOnBackgroundTaskDynamicPropertyFactory(bool throwOnDisposed = true) { - _throwOnDisposed = throwOnDisposed; } /// public virtual IDynamicProperty Create(string name, T initialValue = default, IViewModel viewModel = null) { - return new ValueChangedOnBackgroundTaskDynamicProperty(name, viewModel, _throwOnDisposed, initialValue); + return new ValueChangedOnBackgroundTaskDynamicProperty(name, viewModel, initialValue); } /// public virtual IDynamicProperty CreateFromObservable(string name, IObservable source, T initialValue = default, IViewModel viewModel = null) { - return new ValueChangedOnBackgroundTaskDynamicPropertyFromObservable(name, source, viewModel, _throwOnDisposed, initialValue); + return new ValueChangedOnBackgroundTaskDynamicPropertyFromObservable(name, source, viewModel, initialValue); } /// public virtual IDynamicProperty CreateFromTask(string name, Func> source, T initialValue = default, IViewModel viewModel = null) { - return new ValueChangedOnBackgroundTaskDynamicPropertyFromTask(name, source, viewModel, _throwOnDisposed, initialValue); + return new ValueChangedOnBackgroundTaskDynamicPropertyFromTask(name, source, viewModel, initialValue); } } } diff --git a/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFromObservable.cs b/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFromObservable.cs index 7d87a2c..290425c 100644 --- a/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFromObservable.cs +++ b/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFromObservable.cs @@ -42,16 +42,10 @@ public ValueChangedOnBackgroundTaskDynamicPropertyFromObservable(string name, IO /// The used to determine dispatcher access. /// The initial value of this property. /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public ValueChangedOnBackgroundTaskDynamicPropertyFromObservable(string name, IObservable source, IViewModel viewModel, bool throwOnDisposed, T initialValue = default) - : base(name, viewModel, throwOnDisposed, initialValue) + : this(name, source, viewModel, initialValue) { - if (source is null) - { - throw new ArgumentNullException(nameof(source)); - } - - _propertyObserver = new DynamicPropertyFromObservable.DynamicPropertyObserver(this); - _subscription = source.Subscribe(_propertyObserver); } /// diff --git a/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFromTask.cs b/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFromTask.cs index 7179a14..9e97e5a 100644 --- a/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFromTask.cs +++ b/src/DynamicMvvm/Property/ValueChangedOnBackgroundTask/ValueChangedOnBackgroundTaskDynamicPropertyFromTask.cs @@ -45,17 +45,10 @@ public ValueChangedOnBackgroundTaskDynamicPropertyFromTask(string name, FuncThe used to determine dispatcher access. /// The initial value of this property. /// Whether a should be thrown when is changed after being disposed. + [Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)] public ValueChangedOnBackgroundTaskDynamicPropertyFromTask(string name, Func> source, IViewModel viewModel, bool throwOnDisposed, T initialValue = default) - : base(name, viewModel, throwOnDisposed, initialValue) + : this(name, source, viewModel, initialValue) { - if (source is null) - { - throw new ArgumentNullException(nameof(source)); - } - - _cancellationTokenSource = new CancellationTokenSource(); - - _ = SetValueFromSource(_cancellationTokenSource.Token, source); } private async Task SetValueFromSource(CancellationToken ct, Func> source)