Merge pull request #1886 from riganti/fix-Repeater-memory-leak

Fix Repeater memoization memory leak

src/Framework/Framework/Compilation/ErrorCheckingVisitor.cs

@@ -71,7 +71,7 @@ private void AddNodeErrors(DothtmlNode node, int priority)
         /// * the locations are processed using MapBindingLocation to make them useful in the context of a dothtml file </summary>
         Dictionary<Exception, DotvvmCompilationSourceLocation?> AnnotateBindingExceptionWithLocation(ResolvedBinding binding, DotvvmProperty? relatedProperty, IEnumerable<Exception> errors)
         {
-            var result = new Dictionary<Exception, DotvvmCompilationSourceLocation?>(new ReferenceEqualityComparer<Exception>());
+            var result = new Dictionary<Exception, DotvvmCompilationSourceLocation?>(ReferenceEqualityComparer<Exception>.Instance);
             void recurse(Exception exception, DotvvmCompilationSourceLocation? location)
             {
                 if (result.ContainsKey(exception))

src/Framework/Framework/Controls/Repeater.cs

@@ -10,6 +10,7 @@
 using System.Linq;
 using System.Runtime.CompilerServices;
 using System.Collections.Generic;
+using DotVVM.Framework.Utils;
 
 namespace DotVVM.Framework.Controls
 {
@@ -139,6 +140,7 @@ protected internal override void OnLoad(IDotvvmRequestContext context)
         protected internal override void OnPreRender(IDotvvmRequestContext context)
         {
             SetChildren(context, renderClientTemplate: !RenderOnServer, memoizeReferences: false);
+            childrenCache.Clear(); // not going to need the unreferenced children anymore
             base.OnPreRender(context);
         }
 
@@ -248,7 +250,7 @@ private DotvvmControl CreateEmptyItem(IDotvvmRequestContext context)
             return emptyDataContainer;
         }
 
-        private ConditionalWeakTable<object, DataItemContainer> childrenCache = new ConditionalWeakTable<object, DataItemContainer>();
+        private readonly Dictionary<object, DataItemContainer> childrenCache = new(ReferenceEqualityComparer<object>.Instance);
         private DotvvmControl AddItem(IList<DotvvmControl> c, IDotvvmRequestContext context, object? item = null, int? index = null, bool allowMemoizationRetrieve = false, bool allowMemoizationStore = false)
         {
             if (allowMemoizationRetrieve && item != null && childrenCache.TryGetValue(item, out var container2) && container2.Parent == null)
@@ -276,8 +278,12 @@ private DotvvmControl AddItem(IList<DotvvmControl> c, IDotvvmRequestContext cont
             // write it to the cache after the content is build. If it would be before that, exception could be suppressed
             if (allowMemoizationStore && item != null)
             {
-                // this GetValue call adds the value without raising exception when the value is already added...
-                childrenCache.GetValue(item, _ => container);
+#if DotNetCore
+                childrenCache.TryAdd(item, container);
+#else
+                if (!childrenCache.ContainsKey(item))
+                    childrenCache.Add(item, container);
+#endif
             }
 
             return container;
@@ -306,14 +312,15 @@ private void SetChildren(IDotvvmRequestContext context, bool renderClientTemplat
             if (DataSource != null)
             {
                 var index = 0;
+                var isCommand = context.RequestType == DotvvmRequestType.Command; // on GET request we are not initializing the Repeater twice
                 foreach (var item in GetIEnumerableFromDataSource()!)
                 {
                     if (SeparatorTemplate != null && index > 0)
                     {
                         AddSeparator(Children, context);
                     }
                     AddItem(Children, context, item, index,
-                        allowMemoizationRetrieve: context.RequestType == DotvvmRequestType.Command && !memoizeReferences, // on GET request we are not initializing the Repeater twice
+                        allowMemoizationRetrieve: isCommand && !memoizeReferences,
                         allowMemoizationStore: memoizeReferences
                     );
                     index++;

src/Framework/Framework/Utils/ReferenceEqualityComparer.cs

@@ -1,12 +1,16 @@
 using System.Collections.Generic;
+using System.Runtime.CompilerServices;
 
 namespace DotVVM.Framework.Utils
 {
+    // TODO next version: Replace with System.Collections.Generic.ReferenceEqualityComparer
     internal class ReferenceEqualityComparer<T> : IEqualityComparer<T>
         where T : class
     {
-        public bool Equals(T? x, T? y) => ReferenceEquals(x, y);
+        public static ReferenceEqualityComparer<T> Instance { get; } = new ReferenceEqualityComparer<T>();
+
 
-        public int GetHashCode(T obj) => obj?.GetHashCode() ?? 0;
+        public bool Equals(T? x, T? y) => ReferenceEquals(x, y);
+        public int GetHashCode(T obj) => obj == null ? 0 : RuntimeHelpers.GetHashCode(obj);
     }
 }

src/Tests/Runtime/RepeaterMemoizationTest.cs

@@ -0,0 +1,157 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
+using AngleSharp.Io;
+using DotVVM.Framework.Binding.Expressions;
+using DotVVM.Framework.Compilation.ControlTree;
+using DotVVM.Framework.Controls;
+using DotVVM.Framework.Controls.Infrastructure;
+using DotVVM.Framework.Hosting;
+using DotVVM.Framework.Testing;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace DotVVM.Framework.Tests.Runtime
+{
+    [TestClass]
+    public class RepeaterMemoizationTest: DotvvmControlTestBase
+    {
+        (Repeater, TestDotvvmRequestContext) Init<T>(T viewModel, ITemplate template)
+            where T: IEnumerable
+        {
+            var dataContextStack = DataContextStack.Create(viewModel.GetType());
+            var repeater = new Repeater() {
+                DataSource = ValueBindingExpression.CreateBinding(BindingService, h => (T)h[0], dataContextStack),
+                ItemTemplate = template
+            };
+            repeater.SetValue(RenderSettings.ModeProperty, RenderMode.Client);
+
+            var context = CreateContext(viewModel);
+
+            return (repeater, context);
+        }
+
+        [DataTestMethod]
+        [DataRow(DotvvmRequestType.Navigate, RenderMode.Client)]
+        [DataRow(DotvvmRequestType.Navigate, RenderMode.Server)]
+        [DataRow(DotvvmRequestType.SpaNavigate, RenderMode.Client)]
+        [DataRow(DotvvmRequestType.SpaNavigate, RenderMode.Server)]
+        [DataRow(DotvvmRequestType.Command, RenderMode.Client)]
+        [DataRow(DotvvmRequestType.Command, RenderMode.Server)]
+        public void Repeater_TemplateInitializedOnce(DotvvmRequestType requestType, RenderMode renderMode)
+        {
+            var templateInits = new List<string>();
+            var (repeater, context) = Init(new[] { "ROW 1", "ROW 2", "ROW 3" }, new DelegateTemplate((sp, body) => {
+
+                body.AppendChildren(new Literal("test"));
+                templateInits.Add((string)body.DataContext);
+            }));
+            repeater.SetValue(RenderSettings.ModeProperty, renderMode);
+            context.RequestType = requestType;
+            var html = InvokeLifecycleAndRender(repeater, context);
+
+            Console.WriteLine("Template inits: " + string.Join(", ", templateInits.Select(i => i ?? "<null>")));
+
+            if (renderMode == RenderMode.Client)
+            {
+                XAssert.Equal(["ROW 1", "ROW 2", "ROW 3", null], templateInits);
+            }
+            else
+            {
+                XAssert.Equal(["ROW 1", "ROW 2", "ROW 3"], templateInits);
+            }
+        }
+
+        [DataTestMethod]
+        [DataRow(DotvvmRequestType.Navigate, RenderMode.Client)]
+        [DataRow(DotvvmRequestType.Navigate, RenderMode.Server)]
+        [DataRow(DotvvmRequestType.SpaNavigate, RenderMode.Client)]
+        [DataRow(DotvvmRequestType.SpaNavigate, RenderMode.Server)]
+        [DataRow(DotvvmRequestType.Command, RenderMode.Client)]
+        [DataRow(DotvvmRequestType.Command, RenderMode.Server)]
+        public void Repeater_ViewModelChange(DotvvmRequestType requestType, RenderMode renderMode)
+        {
+            var templateInits = new List<string>();
+            var viewModel = new[] { "ROW 1", "ROW 2", "ROW 3" };
+            var (repeater, context) = Init(viewModel, new DelegateTemplate((sp, body) => {
+
+                body.AppendChildren(new Literal("test"));
+                templateInits.Add((string)body.DataContext);
+            }));
+            context.RequestType = requestType;
+            repeater.SetValue(RenderSettings.ModeProperty, renderMode);
+
+            var view = new DotvvmView();
+            view.Children.Add(repeater);
+            view.SetValue(Internal.RequestContextProperty, context);
+            view.DataContext = context.ViewModel;
+
+            // Run Load event
+
+            DotvvmControlCollection.InvokePageLifeCycleEventRecursive(view, LifeCycleEventType.Load, context);
+            Console.WriteLine("Template inits (Load): " + string.Join(", ", templateInits.Select(i => i ?? "<null>")));
+
+            if (requestType == DotvvmRequestType.Command)
+                XAssert.Equal(["ROW 1", "ROW 2", "ROW 3"], templateInits);
+            else
+                XAssert.Equal([], templateInits);
+
+            // Continue with PreRender and Render after shuffling the view model
+
+            view.DataContext = context.ViewModel = viewModel = [ viewModel[1], viewModel[2], "ROW 4", viewModel[1] ];
+
+            var html = InvokeLifecycleAndRender(view, context);
+
+            Console.WriteLine("Template inits (Render): " + string.Join(", ", templateInits.Select(i => i ?? "<null>")));
+
+            string[] nullIfClient = renderMode == RenderMode.Client ? [ null ] : [];
+
+            if (requestType == DotvvmRequestType.Command)
+            {
+                XAssert.Equal(["ROW 1", "ROW 2", "ROW 3", /* Load end */ "ROW 4", "ROW 2", ..nullIfClient], templateInits);
+            }
+            else
+            {
+                XAssert.Equal(["ROW 2", "ROW 3", "ROW 4", "ROW 2", ..nullIfClient], templateInits);
+            }
+        }
+
+        [DataTestMethod]
+        [DataRow(DotvvmRequestType.Navigate, RenderMode.Client)]
+        [DataRow(DotvvmRequestType.Navigate, RenderMode.Server)]
+        [DataRow(DotvvmRequestType.SpaNavigate, RenderMode.Client)]
+        [DataRow(DotvvmRequestType.SpaNavigate, RenderMode.Server)]
+        [DataRow(DotvvmRequestType.Command, RenderMode.Client)]
+        [DataRow(DotvvmRequestType.Command, RenderMode.Server)]
+        public void Repeater_IsCollectible(DotvvmRequestType requestType, RenderMode renderMode)
+        {
+            var (repeaterRef, contextRef, viewModelRef) = RunLifecycle(requestType, renderMode);
+
+            for (int i = 0; i < 100; i++) // single collection is enough normally, but just to be sure it's not flaky on CI...
+            {
+                GC.Collect(2, GCCollectionMode.Forced, blocking: true);
+
+                Console.WriteLine($"GC Collect #{i} ({repeaterRef.IsAlive}, {contextRef.IsAlive}, {viewModelRef.IsAlive})");
+
+                if (!repeaterRef.IsAlive || !contextRef.IsAlive || !viewModelRef.IsAlive)
+                {
+                    break;
+                }
+            }
+
+            XAssert.Equal((false, false, false), (repeaterRef.IsAlive, contextRef.IsAlive, viewModelRef.IsAlive));
+
+            (WeakReference, WeakReference, WeakReference) RunLifecycle(DotvvmRequestType requestType, RenderMode renderMode)
+            {
+                var (repeater, context) = Init(new[] { "ROW 1", "ROW 2", "ROW 3" }, new DelegateTemplate((sp, body) => {
+                    body.AppendChildren(new Literal("test"));
+                }));
+                repeater.SetValue(RenderSettings.ModeProperty, renderMode);
+                context.RequestType = requestType;
+                var html = InvokeLifecycleAndRender(repeater, context);
+
+                return (new WeakReference(repeater), new WeakReference(context), new WeakReference(context.ViewModel));
+            }
+        }
+    }
+}