-
Notifications
You must be signed in to change notification settings - Fork 97
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix Repeater memoization memory leak
If the ConditionalWeakTable references itself, it will get leaked by .NET Core GC: dotnet/runtime#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 <[email protected]>
- Loading branch information
Showing
4 changed files
with
175 additions
and
7 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)); | ||
| } | ||
| } | ||
| } | ||
| } |