Feature/fix memory leak

CalculatedProperties/CalculatedProperty.cs

@@ -121,7 +121,7 @@ public DebugView(CalculatedProperty<T> property)
 
             public HashSet<ISourceProperty> Sources { get { return _property._sources; } } 
 
-            public HashSet<ITargetProperty> Targets { get { return _base.Targets; } }
+            public List<ITargetProperty> Targets { get { return _base.Targets; } }
 
             public bool ListeningToCollectionChanged { get { return _property._collectionChangedHandler != null; } }
         }

CalculatedProperties/Internal/SourcePropertyBase.cs

@@ -2,6 +2,8 @@
 using System.Collections.Generic;
 using System.ComponentModel;
 using System.Diagnostics;
+using System.Linq;
+using System.Runtime.CompilerServices;
 using System.Threading;
 
 namespace CalculatedProperties.Internal
@@ -13,7 +15,8 @@ public abstract class SourcePropertyBase : ISourceProperty
     {
         private readonly int _threadId;
         private readonly Action<PropertyChangedEventArgs> _onPropertyChanged;
-        private readonly HashSet<ITargetProperty> _targets;
+        private readonly ConditionalWeakTable<ITargetProperty, ITargetProperty> _targetsLookup;
+        private readonly List<WeakReference> _targetsList;
         private PropertyChangedEventArgs _args;
         private string _propertyName;
 
@@ -25,7 +28,8 @@ protected SourcePropertyBase(Action<PropertyChangedEventArgs> onPropertyChanged)
         {
             _threadId = Thread.CurrentThread.ManagedThreadId;
             _onPropertyChanged = onPropertyChanged;
-            _targets = new HashSet<ITargetProperty>();
+            _targetsLookup = new ConditionalWeakTable<ITargetProperty, ITargetProperty>();
+            _targetsList = new List<WeakReference>();
         }
 
         /// <summary>
@@ -73,12 +77,15 @@ public virtual void Invalidate()
                 // Queue OnNotifyPropertyChanged.
                 (PropertyChangedNotificationManager.Instance as IPropertyChangedNotificationManager).Register(this);
 
-                // Invalidate all targets.
-                foreach (var target in _targets)
-                    target.Invalidate();
+                InvalidateAllTargets();
             }
         }
 
+        private void InvalidateAllTargets()
+        {
+            foreach (ITargetProperty target in GetAliveTargets())
+                target.Invalidate();
+        }
         /// <summary>
         /// Invalidates this property and the transitive closure of all its target properties. If notifications are not deferred, then this method will raise <see cref="INotifyPropertyChanged.PropertyChanged"/> for all affected properties before returning. <see cref="INotifyPropertyChanged.PropertyChanged"/> is not raised for this property.
         /// </summary>
@@ -87,20 +94,22 @@ public virtual void InvalidateTargets()
             // Ensure notifications are deferred.
             using (PropertyChangedNotificationManager.Instance.DeferNotifications())
             {
-                // Invalidate all targets.
-                foreach (var target in _targets)
-                    target.Invalidate();
+                InvalidateAllTargets();
             }
         }
 
         void ISourceProperty.AddTarget(ITargetProperty targetProperty)
         {
-            _targets.Add(targetProperty);
+            if (!ContainsTarget(targetProperty))
+            {
+                _targetsLookup.Add(targetProperty, targetProperty);
+                _targetsList.Add(new WeakReference(targetProperty));
+            }
         }
 
         void ISourceProperty.RemoveTarget(ITargetProperty targetProperty)
         {
-            _targets.Remove(targetProperty);
+            _targetsLookup.Remove(targetProperty);
         }
 
         /// <summary>
@@ -127,7 +136,19 @@ public DebugView(SourcePropertyBase property)
             /// <summary>
             /// Gets the target properties.
             /// </summary>
-            public HashSet<ITargetProperty> Targets { get { return _property._targets; } }
+            public List<ITargetProperty> Targets { get { return _property.GetAliveTargets(); } }
+        }
+
+        private List<ITargetProperty> GetAliveTargets()
+        {
+            _targetsList.RemoveAll(@ref => !ContainsTarget((ITargetProperty) @ref.Target));
+            return _targetsList.Select(@ref => (ITargetProperty) @ref.Target).Where(t => t != null).ToList();
+        }
+
+        private bool ContainsTarget(ITargetProperty target)
+        {
+            ITargetProperty cachedTarget;
+            return target != null && _targetsLookup.TryGetValue(target, out cachedTarget);
         }
     }
 }

CalculatedProperties/Properties/AssemblyInfo.cs

@@ -4,5 +4,5 @@
 [assembly: AssemblyCompany("Stephen Cleary")]
 [assembly: AssemblyDescription("Easy-to-use calculated properties for MVVM apps (.NET 4, MonoTouch, MonoDroid, Windows 8, Windows Phone 8.1, Windows Phone Silverlight 8.0, and Silverlight 5).")]
 
-[assembly: AssemblyVersion("1.0.0")]
-[assembly: AssemblyInformationalVersion("1.0.0")]
+[assembly: AssemblyVersion("1.0.1")]
+[assembly: AssemblyInformationalVersion("1.0.1")]

CalculatedProperties/TriggerProperty.cs

@@ -102,7 +102,7 @@ public DebugView(TriggerProperty<T> property)
 
             public T Value { get { return _property._value; } }
 
-            public HashSet<ITargetProperty> Targets { get { return _base.Targets; } }
+            public List<ITargetProperty> Targets { get { return _base.Targets; } }
 
             public bool ListeningToCollectionChanged { get { return _property._collectionChangedHandler != null; } }
         }

Unit Tests/MemoryLeakUnitTests.cs

@@ -0,0 +1,87 @@
+using System;
+using System.Collections.Generic;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Unit_Tests
+{
+    [TestClass]
+    public class MemoryLeakUnitTests
+    {
+        [TestMethod]
+        public void LongLivingPublisherAllowsToGarbageCollectShortLivingSubscribers()
+        {
+            FullGarbageCollection();
+
+            var longLivingVm = new LongLivingVm();
+
+            var notifications = new List<string>();
+            Action transientScopeAction = () =>
+            {
+                var transientObject = new ShortLivingViewModel(longLivingVm);
+                transientObject.PropertyChanged += (sender, args) => notifications.Add(args.PropertyName);
+                // emulate read from UI
+                var tmp = transientObject.FullName;
+                Assert.IsTrue(notifications.Count == 0);
+
+                longLivingVm.FirstName = "Mister";
+                Assert.IsTrue(notifications.Count == 1 && notifications[0] == nameof(ShortLivingViewModel.FullName));
+
+                // we have one instance of short living object
+                Assert.IsTrue(ShortLivingViewModel.InstanceCount == 1);
+                // scope finished: eligible for GC
+            };
+            transientScopeAction();
+
+            FullGarbageCollection();
+
+            notifications.Clear();
+            longLivingVm.FirstName = "Twister";
+
+            // transient listener has been GCed and didn't receive any notifications
+            Assert.IsTrue(notifications.Count == 0);
+            Assert.IsTrue(ShortLivingViewModel.InstanceCount == 0);
+        }
+
+        private static void FullGarbageCollection()
+        {
+            GC.Collect();
+            GC.WaitForPendingFinalizers();
+            GC.Collect();
+        }
+    }
+
+    class ShortLivingViewModel : ViewModelBase
+    {
+        public static int InstanceCount = 0;
+
+        ~ShortLivingViewModel()
+        {
+            InstanceCount--;
+        }
+
+        public ShortLivingViewModel(LongLivingVm longLivingVm)
+        {
+            InstanceCount++;
+            _longLivingVm = longLivingVm;
+        }
+
+        private readonly LongLivingVm _longLivingVm;
+
+        public string FullName => Properties.Calculated(() => $"{_longLivingVm.FirstName} {_longLivingVm.LastName}");
+    }
+
+    class LongLivingVm : ViewModelBase
+    {
+        public string FirstName
+        {
+            get { return Properties.Get((string)null); }
+            set { Properties.Set(value); }
+        }
+
+        public string LastName
+        {
+            get { return Properties.Get((string)null); }
+            set { Properties.Set(value); }
+        }
+    }
+}

Unit Tests/PerformanceUnitTests.cs

@@ -0,0 +1,101 @@
+using System;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.Threading;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Unit_Tests
+{
+    [TestClass]
+    public class PerformanceUnitTests
+    {
+        [TestMethod]
+        public void TriggerPropertyWithThousandsOfTargetsHasAcceptableRewiringTime()
+        {
+            SourceViewModel source = new SourceViewModel();
+
+            CreateAndRewireManyTargets(source);
+
+            // GC can collect all targets which went out of scope
+
+            GC.Collect();
+            GC.WaitForPendingFinalizers();
+            GC.Collect();
+
+            Assert.IsTrue(TargetViewModel.InstanceCount == 0);
+        }
+
+        private static void CreateAndRewireManyTargets(SourceViewModel source)
+        {
+            const int size = 50000;
+
+            // populate source.BaseValue collection of targets is populated
+            Stopwatch sw = new Stopwatch();
+            sw.Start();
+
+            TargetViewModel[] manyTargets = new TargetViewModel[2 * size];
+            for (int i = 0; i < size; i++)
+            {
+                TargetViewModel target = new TargetViewModel {Source = source};
+                var tmp = target.DerivedValue;
+                manyTargets[i] = target;
+            }
+            sw.Stop();
+            Console.WriteLine("Initial wiring time: " + sw.Elapsed);
+
+            Assert.IsTrue(TargetViewModel.InstanceCount == size);
+
+            // emulate massive rewiring: adding new targets and resetting old
+            sw.Restart();
+
+            for (int i = 0; i < size; i++)
+            {
+                // unplug a target
+                manyTargets[i].Source = null;
+                var tmp = manyTargets[i].DerivedValue;
+
+                // plug a new target and keep it alive in array
+                TargetViewModel target = new TargetViewModel {Source = source};
+                manyTargets[i + size] = target;
+                tmp = target.DerivedValue;
+            }
+            sw.Stop();
+            Console.WriteLine("Rewiring time: " + sw.Elapsed);
+
+            // rewiring should take under a second on fast dev machine but let's say two seconds
+            Assert.IsTrue(sw.Elapsed.TotalSeconds < 2);
+        }
+
+        class SourceViewModel : ViewModelBase
+        {
+            public int BaseValue
+            {
+                get { return Properties.Get(0); }
+                set { Properties.Set(value); }
+            }
+        }
+
+        class TargetViewModel : ViewModelBase
+        {
+            public static int InstanceCount = 0;
+
+            ~TargetViewModel()
+            {
+                Interlocked.Decrement(ref InstanceCount);
+            }
+
+            public TargetViewModel()
+            {
+                Interlocked.Increment(ref InstanceCount);
+            }
+
+            public SourceViewModel Source
+            {
+                get { return Properties.Get((SourceViewModel)null); }
+                set { Properties.Set(value); }
+            }
+
+            public int DerivedValue => Properties.Calculated(() => Source?.BaseValue ?? 0 + 5);
+        }
+    }
+}

Unit Tests/Unit Tests.csproj

@@ -53,6 +53,8 @@
     <Compile Include="BindingListUnitTests.cs" />
     <Compile Include="CollectionUnitTests.cs" />
     <Compile Include="ComparerUnitTests.cs" />
+    <Compile Include="PerformanceUnitTests.cs" />
+    <Compile Include="MemoryLeakUnitTests.cs" />
     <Compile Include="TeeUnitTests.cs" />
     <Compile Include="ChainUnitTests.cs" />
     <Compile Include="BranchUnitTests.cs" />