From eade18f547751f65f26a1a99fa6b543b6d909e00 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 5b70959243..988f8bbf2a 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)); + } + } + } +}