Merge pull request #93 from nventive/dev/jpl/dont-throw

fix: DynamicProperties no longer throw ObjectDisposedExceptions

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.
 

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<TestEntity>(), 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<InvalidOperationException>();
-				actOb.Should().Throw<InvalidOperationException>();
-				actP.Should().Throw<InvalidOperationException>();
-			}
-			else
-			{
-				actTask.Should().NotThrow();
-				actOb.Should().NotThrow();
-				actP.Should().NotThrow();
-			}
-		}
 	}
 }

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<IDynamicProperty>();
-			var property = new DynamicProperty<TestEntity>(DefaultPropertyName, throwOnDisposed: throwOnDisposed);
+			var property = new DynamicProperty<TestEntity>(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<ObjectDisposedException>();
-			}
-			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<TestEntity>(DefaultPropertyName, myValue);
+
+			// Act
+			property.Dispose();
+			property.Value = new TestEntity();
+
+			// Assert
+			property.Value.Should().Be(myValue);
 		}
 
 		[Fact]

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<IDynamicProperty>();
+			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<ObjectDisposedException>();
-			}
-			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<ObjectDisposedException>(() => property.Value = new object());
+			// Assert
+			property.Value.Should().Be(myValue);
 		}
 
 		[Fact]

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);
 

src/DynamicMvvm/Property/DynamicProperty.T.cs

@@ -26,8 +26,9 @@ public DynamicProperty(string name, T value = default)
 		/// <param name="name">Name</param>
 		/// <param name="value">Initial value</param>
 		/// <param name="throwOnDisposed">Whether a <see cref="ObjectDisposedException"/> should be thrown when <see cref="Value"/> is changed after being disposed.</param>
+		[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)
 		{
 		}
 

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;
 		}
 
 		/// <summary>
@@ -40,16 +38,10 @@ public DynamicProperty(string name, object value = default)
 		/// <param name="name">Name</param>
 		/// <param name="value">Initial value</param>
 		/// <param name="throwOnDisposed">Whether a <see cref="ObjectDisposedException"/> should be thrown when <see cref="Value"/> is changed after being disposed.</param>
+		[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;
 		}
 
 		/// <inheritdoc />
@@ -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))

src/DynamicMvvm/Property/DynamicPropertyFactory.cs

@@ -13,8 +13,6 @@ namespace Chinook.DynamicMvvm
 	[Preserve(AllMembers = true)]
 	public class DynamicPropertyFactory : IDynamicPropertyFactory
 	{
-		private readonly bool _throwOnDisposed;
-
 		/// <summary>
 		/// Initializes a new instance of the <see cref="DynamicPropertyFactory"/> class.
 		/// </summary>
@@ -23,28 +21,27 @@ public class DynamicPropertyFactory : IDynamicPropertyFactory
 		/// </remarks>
 		public DynamicPropertyFactory()
 		{
-			_throwOnDisposed = true;
 		}
 
 		/// <summary>
 		/// Initializes a new instance of the <see cref="DynamicPropertyFactory"/> class.
 		/// </summary>
 		/// <param name="throwOnDisposed">Whether a <see cref="ObjectDisposedException"/> should be thrown when <see cref="IDynamicProperty.Value"/> is changed after being disposed.</param>
+		[Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)]
 		public DynamicPropertyFactory(bool throwOnDisposed)
 		{
-			_throwOnDisposed = throwOnDisposed;
 		}
 
 		/// <inheritdoc />
 		public virtual IDynamicProperty Create<T>(string name, T initialValue = default, IViewModel viewModel = null)
-			=> new DynamicProperty<T>(name, _throwOnDisposed, initialValue);
+			=> new DynamicProperty<T>(name, initialValue);
 
 		/// <inheritdoc />
 		public virtual IDynamicProperty CreateFromTask<T>(string name, Func<CancellationToken, Task<T>> source, T initialValue = default, IViewModel viewModel = null)
-			=> new DynamicPropertyFromTask<T>(name, source, _throwOnDisposed, initialValue);
+			=> new DynamicPropertyFromTask<T>(name, source, initialValue);
 
 		/// <inheritdoc />
 		public virtual IDynamicProperty CreateFromObservable<T>(string name, IObservable<T> source, T initialValue = default, IViewModel viewModel = null)
-			=> new DynamicPropertyFromObservable<T>(name, source, _throwOnDisposed, initialValue);
+			=> new DynamicPropertyFromObservable<T>(name, source, initialValue);
 	}
 }

src/DynamicMvvm/Property/DynamicPropertyFromObservable.cs

@@ -43,16 +43,10 @@ public DynamicPropertyFromObservable(string name, IObservable<T> source, T initi
 		/// <param name="source">Source</param>
 		/// <param name="initialValue">Initial value</param>
 		/// <param name="throwOnDisposed">Whether a <see cref="ObjectDisposedException"/> should be thrown when <see cref="IDynamicProperty.Value"/> is changed after being disposed.</param>
+		[Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)]
 		public DynamicPropertyFromObservable(string name, IObservable<T> 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);
 		}
 
 		/// <inheritdoc />

src/DynamicMvvm/Property/DynamicPropertyFromTask.cs

@@ -43,17 +43,10 @@ public DynamicPropertyFromTask(string name, Func<CancellationToken, Task<T>> sou
 		/// <param name="source">Source</param>
 		/// <param name="initialValue">Initial value</param>
 		/// <param name="throwOnDisposed">Whether a <see cref="ObjectDisposedException"/> should be thrown when <see cref="IDynamicProperty.Value"/> is changed after being disposed.</param>
+		[Obsolete("This constructor is obsolete. The throwOnDisposed parameter is no longer used.", error: false)]
 		public DynamicPropertyFromTask(string name, Func<CancellationToken, Task<T>> 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<CancellationToken, Task<T>> source)

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<IViewModel> _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;
 		}
 
 		/// <summary>
@@ -45,17 +43,10 @@ public ValueChangedOnBackgroundTaskDynamicProperty(string name, IViewModel viewM
 		/// <param name="viewModel">The <see cref="IViewModel"/> used to determine dispatcher access.</param>
 		/// <param name="value">The initial value of this property.</param>
 		/// <param name="throwOnDisposed">Whether a <see cref="ObjectDisposedException"/> should be thrown when <see cref="Value"/> is changed after being disposed.</param>
+		[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<IViewModel>(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
 		/// <param name="viewModel">The <see cref="IViewModel"/> used to determine dispatcher access.</param>
 		/// <param name="value">The initial value of this property.</param>
 		/// <param name="throwOnDisposed">Whether a <see cref="ObjectDisposedException"/> should be thrown when <see cref="Value"/> is changed after being disposed.</param>
+		[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)
 		{
 		}