From d3b55c7955900a88d85faba4e1b75d3ba7c697d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Tue, 19 Nov 2024 20:43:50 +0100 Subject: [PATCH] Fix Repeater memoization memory leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the ConditionalWeakTable references itself, it will get leaked by .NET Core GC: https://github.com/dotnet/runtime/issues/12255 Fortunately, we don't really need the ConditinalWeakTable here: * Repeater should generally be a short-lived object, so strong references will also be collected reasonably soon * Between Load and PreRender, the viewmodels were pinned in the DataContext of all children and the children are in Children collection. Only after PreRender could those references be useful, but we can as well Clear the dictionary at that point. Co-authored-by: Adam Štěpánek <13067679+cafour@users.noreply.github.com> --- .../Compilation/ErrorCheckingVisitor.cs | 2 +- src/Framework/Framework/Controls/Repeater.cs | 15 +- .../Utils/ReferenceEqualityComparer.cs | 8 +- src/Tests/Runtime/RepeaterMemoizationTest.cs | 157 ++++++++++++++++++ 4 files changed, 175 insertions(+), 7 deletions(-) create mode 100644 src/Tests/Runtime/RepeaterMemoizationTest.cs diff --git a/src/Framework/Framework/Compilation/ErrorCheckingVisitor.cs b/src/Framework/Framework/Compilation/ErrorCheckingVisitor.cs index 5f6a637529..86413cde75 100644 --- a/src/Framework/Framework/Compilation/ErrorCheckingVisitor.cs +++ b/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 Dictionary AnnotateBindingExceptionWithLocation(ResolvedBinding binding, DotvvmProperty? relatedProperty, IEnumerable errors) { - var result = new Dictionary(new ReferenceEqualityComparer()); + var result = new Dictionary(ReferenceEqualityComparer.Instance); void recurse(Exception exception, DotvvmCompilationSourceLocation? location) { if (result.ContainsKey(exception)) diff --git a/src/Framework/Framework/Controls/Repeater.cs b/src/Framework/Framework/Controls/Repeater.cs index c6a3073be1..ccc4cf5f87 100644 --- a/src/Framework/Framework/Controls/Repeater.cs +++ b/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 childrenCache = new ConditionalWeakTable(); + private readonly Dictionary childrenCache = new(ReferenceEqualityComparer.Instance); private DotvvmControl AddItem(IList 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 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,6 +312,7 @@ 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) @@ -313,7 +320,7 @@ private void SetChildren(IDotvvmRequestContext context, bool renderClientTemplat 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++; diff --git a/src/Framework/Framework/Utils/ReferenceEqualityComparer.cs b/src/Framework/Framework/Utils/ReferenceEqualityComparer.cs index 8d83b2f6fe..4c34d41263 100644 --- a/src/Framework/Framework/Utils/ReferenceEqualityComparer.cs +++ b/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 : IEqualityComparer where T : class { - public bool Equals(T? x, T? y) => ReferenceEquals(x, y); + public static ReferenceEqualityComparer Instance { get; } = new ReferenceEqualityComparer(); + - 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); } } diff --git a/src/Tests/Runtime/RepeaterMemoizationTest.cs b/src/Tests/Runtime/RepeaterMemoizationTest.cs new file mode 100644 index 0000000000..9c447993c4 --- /dev/null +++ b/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 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(); + 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 ?? ""))); + + 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(); + 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 ?? ""))); + + 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 ?? ""))); + + 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)); + } + } + } +}