Merge pull request #456 from zjklee/issues/452

Fix memory leak at Configuration subscribers

source/Handlebars.Test/Collections/WeakCollectionTests.cs

@@ -0,0 +1,82 @@
+using System;
+using System.Linq;
+using HandlebarsDotNet.Collections;
+using Xunit;
+
+namespace HandlebarsDotNet.Test.Collections
+{
+    public class WeakCollectionTests
+    {
+        private readonly WeakCollection<object> _collection = new WeakCollection<object>();
+
+        [Fact]
+        public void ObjectIsRemovedFromCollectionOnceNotReachable()
+        {
+            var _0 = new object();
+            _collection.Add(_0);
+
+            CallInItsOwnScope(() =>
+            {
+                var _1 = new object();
+                _collection.Add(_1);
+            
+                GC.Collect();
+                Assert.Equal(2, _collection.Size);
+                Assert.Equal(2, _collection.Count());
+            });
+
+            GC.Collect();
+            Assert.Equal(2, _collection.Size);
+            Assert.Single(_collection);
+        }
+
+        [Fact]
+        public void BucketInCollectionReused()
+        {
+            var _0 = new object();
+            _collection.Add(_0);
+
+            CallInItsOwnScope(() =>
+            {
+                _collection.Add(new object());
+                _collection.Add(new object());
+                _collection.Add(new object());
+            });
+
+            GC.Collect();
+
+            _ = _collection.Count(); // force enumeration to set inner index
+
+            var _2 = new object();
+            var _3 = new object();
+            _collection.Add(_2);
+            _collection.Add(_3);
+
+            GC.Collect();
+            Assert.Equal(4, _collection.Size);
+            Assert.Equal(3, _collection.Count());
+        }
+
+        [Fact]
+        public void BucketInCollectionReusedAfterRemove()
+        {
+            var _0 = new object();
+            var _1 = new object();
+            var _2 = new object();
+            _collection.Add(_0);
+            _collection.Add(_1);
+            _collection.Add(_2);
+
+            GC.Collect();
+
+            _collection.Remove(_0);
+            _collection.Remove(_1);
+            _collection.Add(new object());
+
+            Assert.Equal(3, _collection.Size);
+            Assert.Equal(2, _collection.Count());
+        }
+
+        private static void CallInItsOwnScope(Action scope) => scope();
+    }
+}

source/Handlebars/Collections/ObservableIndex.cs

@@ -18,13 +18,13 @@ public class ObservableIndex<TKey, TValue, TComparer> :
         private readonly ReaderWriterLockSlim _observersLock = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
         private readonly ReaderWriterLockSlim _itemsLock = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
 
-        private readonly List<IObserver<ObservableEvent<TValue>>> _observers;
+        private readonly WeakCollection<IObserver<ObservableEvent<TValue>>> _observers;
         private readonly DictionarySlim<TKey, TValue, TComparer> _inner;
 
         public ObservableIndex(TComparer comparer, IReadOnlyIndexed<TKey, TValue> outer = null)
         {
             _inner = outer != null ? new DictionarySlim<TKey, TValue, TComparer>(outer, comparer) : new DictionarySlim<TKey, TValue, TComparer>(comparer);
-            _observers = new List<IObserver<ObservableEvent<TValue>>>();
+            _observers = new WeakCollection<IObserver<ObservableEvent<TValue>>>();
             if (outer is IObservable<ObservableEvent<TValue>> observableDictionary)
             {
                 observableDictionary.Subscribe(this);
@@ -38,7 +38,7 @@ public IDisposable Subscribe(IObserver<ObservableEvent<TValue>> observer)
                 _observers.Add(observer);
             }
 
-            var disposableContainer = new DisposableContainer<List<IObserver<ObservableEvent<TValue>>>, ReaderWriterLockSlim>(
+            var disposableContainer = new DisposableContainer<WeakCollection<IObserver<ObservableEvent<TValue>>>, ReaderWriterLockSlim>(
                 _observers, _observersLock, (observers, @lock) =>
                 {
                     using (@lock.WriteLock())
@@ -55,11 +55,11 @@ private void Publish(ObservableEvent<TValue> @event)
         {
             using (_observersLock.ReadLock())
             {
-                for (var index = 0; index < _observers.Count; index++)
+                foreach (var observer in _observers)
                 {
                     try
                     {
-                        _observers[index].OnNext(@event);
+                        observer.OnNext(@event);
                     }
                     catch
                     {

source/Handlebars/Collections/ObservableList.cs

@@ -16,7 +16,7 @@ public class ObservableList<T> :
         private readonly ReaderWriterLockSlim _observersLock = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
         private readonly ReaderWriterLockSlim _itemsLock = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
 
-        private readonly List<IObserver<ObservableEvent<T>>> _observers = new List<IObserver<ObservableEvent<T>>>();
+        private readonly WeakCollection<IObserver<ObservableEvent<T>>> _observers = new WeakCollection<IObserver<ObservableEvent<T>>>();
         private readonly List<T> _inner;
 
         public ObservableList(IEnumerable<T> list = null)
@@ -83,7 +83,7 @@ public IDisposable Subscribe(IObserver<ObservableEvent<T>> observer)
                 _observers.Add(observer);
             }
 
-            var disposableContainer = new DisposableContainer<List<IObserver<ObservableEvent<T>>>, ReaderWriterLockSlim>(
+            var disposableContainer = new DisposableContainer<WeakCollection<IObserver<ObservableEvent<T>>>, ReaderWriterLockSlim>(
                 _observers, _observersLock, (observers, @lock) =>
                 {
                     using (@lock.WriteLock())
@@ -123,11 +123,11 @@ private void Publish(ObservableEvent<T> @event)
         {
             using (_observersLock.ReadLock())
             {
-                for (int index = 0; index < _observers.Count; index++)
+                foreach (var observer in _observers)
                 {
                     try
                     {
-                        _observers[index].OnNext(@event);
+                        observer.OnNext(@event);
                     }
                     catch
                     {

source/Handlebars/Collections/ObserverBuilder.cs

@@ -3,37 +3,32 @@
 
 namespace HandlebarsDotNet.Collections
 {
-    internal class ObserverBuilder<T>
+    internal static class ObserverBuilder<T>
     {
-        private readonly Dictionary<Type, List<Action<T>>> _handlers = new Dictionary<Type, List<Action<T>>>();
-
-        public ObserverBuilder<T> OnEvent<TEvent>(Action<TEvent> handler, Func<TEvent, bool> predicate = null) where TEvent: T
+        public static ObserverBuilder<T, TState> Create<TState>(TState state)
         {
-            if (!_handlers.TryGetValue(typeof(TEvent), out var handlers))
-            {
-                handlers = new List<Action<T>>();
-                _handlers.Add(typeof(TEvent), handlers);
-            }
-
-            handlers.Add(@event =>
-            {
-                if(predicate?.Invoke((TEvent) @event) ?? true) handler((TEvent) @event);
-            });
-
-            return this;
+            return new ObserverBuilder<T, TState>(state);
         }
-
-        public ObserverBuilder<T> OnEvent<TEvent, TState>(TState state, Action<TEvent, TState> handler, Func<TEvent, bool> predicate = null) where TEvent: T
+    }
+
+    internal class ObserverBuilder<T, TState>
+    {
+        private readonly TState _state;
+        private readonly Dictionary<Type, List<Action<T>>> _handlers = new Dictionary<Type, List<Action<T>>>();
+
+        public ObserverBuilder(TState state) => _state = state;
+
+        public ObserverBuilder<T, TState> OnEvent<TEvent>(Action<TEvent, TState> handler, Func<TEvent, bool> predicate = null) where TEvent: T
         {
             if (!_handlers.TryGetValue(typeof(TEvent), out var handlers))
             {
                 handlers = new List<Action<T>>();
                 _handlers.Add(typeof(TEvent), handlers);
             }
 
-            handlers.Add(@event =>
+            handlers.Add((@event) =>
             {
-                if(predicate?.Invoke((TEvent) @event) ?? true) handler((TEvent) @event, state);
+                if(predicate?.Invoke((TEvent) @event) ?? true) handler((TEvent) @event, _state);
             });
 
             return this;

source/Handlebars/Collections/WeakCollection.cs

@@ -0,0 +1,85 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+
+namespace HandlebarsDotNet.Collections
+{
+    public class WeakCollection<T> : IEnumerable<T> where T : class
+    {
+        private readonly List<WeakReference<T>> _store = new List<WeakReference<T>>();
+        private int _firstAvailableIndex = 0;
+
+        public int Size => _store.Count;
+
+        public void Add(T value)
+        {
+            for (var index = _firstAvailableIndex; index < _store.Count; index++)
+            {
+                if (_store[index] == null)
+                {
+                    _firstAvailableIndex = index + 1;
+                    _store[index] = new WeakReference<T>(value);
+                    return;
+                }
+
+                if (!_store[index].TryGetTarget(out _))
+                {
+                    _firstAvailableIndex = index + 1;
+                    _store[index].SetTarget(value);
+                    return;
+                }
+            }
+
+            _store.Add(new WeakReference<T>(value));
+            _firstAvailableIndex = _store.Count;
+        }
+
+        public void Remove(T value)
+        {
+            for (var index = 0; index < _store.Count; index++)
+            {
+                if (_store[index] == null)
+                {
+                    _firstAvailableIndex = Math.Min(_firstAvailableIndex, index);
+                    continue;
+                }
+
+                if (!_store[index].TryGetTarget(out var target))
+                {
+                    _firstAvailableIndex = Math.Min(_firstAvailableIndex, index);
+                    continue;
+                }
+
+                if (target.Equals(value))
+                {
+                    _store[index] = null;
+                    _firstAvailableIndex = Math.Min(_firstAvailableIndex, index);
+                    return;
+                }
+            }
+        }
+
+        public IEnumerator<T> GetEnumerator()
+        {
+            for (var index = 0; index < _store.Count; index++)
+            {
+                var reference = _store[index];
+                if (reference == null)
+                {
+                    _firstAvailableIndex = Math.Min(_firstAvailableIndex, index);
+                    continue;
+                }
+
+                if (!reference.TryGetTarget(out var target))
+                {
+                    _firstAvailableIndex = Math.Min(_firstAvailableIndex, index);
+                    continue;
+                }
+
+                yield return target;
+            }
+        }
+
+        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+    }
+}

source/Handlebars/Configuration/HandlebarsConfigurationAdapter.cs

@@ -17,6 +17,8 @@ namespace HandlebarsDotNet
 {
     internal class HandlebarsConfigurationAdapter : ICompiledHandlebarsConfiguration
     {
+        private readonly List<object> _observers = new List<object>();
+
         public HandlebarsConfigurationAdapter(HandlebarsConfiguration configuration)
         {
             UnderlingConfiguration = configuration;
@@ -85,11 +87,8 @@ private ObservableIndex<PathInfoLight, Ref<IHelperDescriptor<TOptions>>, IEquali
 
             var target = new ObservableIndex<PathInfoLight, Ref<IHelperDescriptor<TOptions>>, IEqualityComparer<PathInfoLight>>(equalityComparer, existingHelpers);
 
-            var helpersObserver = new ObserverBuilder<ObservableEvent<IHelperDescriptor<TOptions>>>()
-                .OnEvent<
-                    DictionaryAddedObservableEvent<string, IHelperDescriptor<TOptions>>,
-                    ObservableIndex<PathInfoLight, Ref<IHelperDescriptor<TOptions>>, IEqualityComparer<PathInfoLight>>
-                >(target,
+            var observer = ObserverBuilder<ObservableEvent<IHelperDescriptor<TOptions>>>.Create(target)
+                .OnEvent<DictionaryAddedObservableEvent<string, IHelperDescriptor<TOptions>>>(
                     (@event, state) =>
                     {
                         PathInfoLight key = $"[{@event.Key}]";
@@ -103,7 +102,9 @@ private ObservableIndex<PathInfoLight, Ref<IHelperDescriptor<TOptions>>, IEquali
                     })
                 .Build();
 
-            source.As<ObservableIndex<string, IHelperDescriptor<TOptions>, StringEqualityComparer>>()?.Subscribe(helpersObserver);
+            _observers.Add(observer);
+
+            source.As<ObservableIndex<string, IHelperDescriptor<TOptions>, StringEqualityComparer>>()?.Subscribe(observer);
 
             return target;
         }
@@ -125,13 +126,12 @@ private ObservableList<IObjectDescriptorProvider> CreateObjectDescriptorProvider
                 }
                 .AddMany(descriptorProviders);
 
-            var observer = new ObserverBuilder<ObservableEvent<IObjectDescriptorProvider>>()
-                .OnEvent<
-                    AddedObservableEvent<IObjectDescriptorProvider>,
-                    ObservableList<IObjectDescriptorProvider>
-                >(objectDescriptorProviders, (@event, state) => { state.Add(@event.Value); })
+            var observer = ObserverBuilder<ObservableEvent<IObjectDescriptorProvider>>.Create(objectDescriptorProviders)
+                .OnEvent<AddedObservableEvent<IObjectDescriptorProvider>>((@event, state) => { state.Add(@event.Value); })
                 .Build();
 
+            _observers.Add(observer);
+
             descriptorProviders.Subscribe(observer);
 
             return objectDescriptorProviders;

source/Handlebars/IO/Formatters/FormatterProvider.cs

@@ -34,19 +34,20 @@ public class FormatterProvider : IFormatterProvider
         private readonly LookupSlim _formatters = new LookupSlim(new ReferenceEqualityComparer<Type>());
 
         private readonly ObservableList<IFormatterProvider> _formatterProviders;
+        private readonly List<object> _observers = new List<object>();
 
         public FormatterProvider(ObservableList<IFormatterProvider> providers = null)
         {
             _formatterProviders = new ObservableList<IFormatterProvider>();
 
             if (providers != null) Append(providers);
 
-            var observer = new ObserverBuilder<ObservableEvent<IFormatterProvider>>()
-                .OnEvent<AddedObservableEvent<IFormatterProvider>, LookupSlim>(
-                    _formatters, (@event, state) => state.Clear()
-                )
+            var observer = ObserverBuilder<ObservableEvent<IFormatterProvider>>.Create(_formatters)
+                .OnEvent<AddedObservableEvent<IFormatterProvider>>((@event, state) => state.Clear())
                 .Build();
 
+            _observers.Add(observer);
+
             _formatterProviders.Subscribe(observer);
         }