Skip to content

Commit 1202a8f

Browse files
dariatiurinailonatommy
dariatiurina
and
ilonatommy
authored
Fixed race condition in the disposing of the QuickGrid (#62840)
* Fixed race condition in the disposing of the QuickGrid * Update tests to check for fixed and not fixed scenario. Co-authored-by: Ilona Tomkowicz <[email protected]> --------- Co-authored-by: Ilona Tomkowicz <[email protected]> Co-authored-by: Ilona Tomkowicz <[email protected]>
1 parent 2dfd73a commit 1202a8f

File tree

6 files changed

+344
-3
lines changed

6 files changed

+344
-3
lines changed

‎src/Components/Components/src/Microsoft.AspNetCore.Components.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@
8989
<InternalsVisibleTo Include="Microsoft.AspNetCore.Components.Tests" />
9090
<InternalsVisibleTo Include="Microsoft.AspNetCore.Components.Endpoints.Tests" />
9191
<InternalsVisibleTo Include="Microsoft.AspNetCore.Components.Web.Tests" />
92+
<InternalsVisibleTo Include="Microsoft.AspNetCore.Components.QuickGrid.Tests" />
9293
<InternalsVisibleTo Include="Microsoft.AspNetCore.Components.ProtectedBrowserStorage.Tests" />
9394
<InternalsVisibleTo Include="Components.TestServer" />
9495
<InternalsVisibleTo Include="DynamicProxyGenAssembly2" Key="$(MoqPublicKey)" />

‎src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/src/QuickGrid.razor.cs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,9 @@ public partial class QuickGrid<TGridItem> : IAsyncDisposable
152152
// If the PaginationState mutates, it raises this event. We use it to trigger a re-render.
153153
private readonly EventCallbackSubscriber<PaginationState> _currentPageItemsChanged;
154154

155+
// If the QuickGrid is disposed while the JS module is being loaded, we need to avoid calling JS methods
156+
private bool _wasDisposed;
157+
155158
/// <summary>
156159
/// Constructs an instance of <see cref="QuickGrid{TGridItem}"/>.
157160
/// </summary>
@@ -206,6 +209,12 @@ protected override async Task OnAfterRenderAsync(bool firstRender)
206209
if (firstRender)
207210
{
208211
_jsModule = await JS.InvokeAsync<IJSObjectReference>("import", "./_content/Microsoft.AspNetCore.Components.QuickGrid/QuickGrid.razor.js");
212+
if (_wasDisposed)
213+
{
214+
// If the component has been disposed while JS module was being loaded, we don't need to continue
215+
await _jsModule.DisposeAsync();
216+
return;
217+
}
209218
_jsEventDisposable = await _jsModule.InvokeAsync<IJSObjectReference>("init", _tableReference);
210219
}
211220

@@ -434,6 +443,7 @@ private string GridClass()
434443
/// <inheritdoc />
435444
public async ValueTask DisposeAsync()
436445
{
446+
_wasDisposed = true;
437447
_currentPageItemsChanged.Dispose();
438448

439449
try

‎src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/test/FailingQuickGrid.cs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using Microsoft.AspNetCore.Components;
5+
using Microsoft.AspNetCore.Components.QuickGrid;
6+
using Microsoft.JSInterop;
7+
using System.Reflection;
8+
9+
namespace Microsoft.AspNetCore.Components.QuickGrid.Tests;
10+
11+
/// <summary>
12+
/// A QuickGrid implementation that simulates the behavior before the race condition fix.
13+
/// This class intentionally does NOT set _disposeBool during disposal to simulate the race condition.
14+
/// </summary>
15+
/// <typeparam name="TGridItem">The type of data represented by each row in the grid.</typeparam>
16+
internal class FailingQuickGrid<TGridItem> : QuickGrid<TGridItem>, IAsyncDisposable
17+
{
18+
[Inject] private IJSRuntime JS { get; set; } = default!;
19+
20+
private readonly TaskCompletionSource _onAfterRenderCompleted = new();
21+
22+
public bool DisposeAsyncWasCalled { get; private set; }
23+
24+
/// <summary>
25+
/// Task that completes when OnAfterRenderAsync has finished executing.
26+
/// This allows tests to wait deterministically for the race condition to occur.
27+
/// </summary>
28+
public Task OnAfterRenderCompleted => _onAfterRenderCompleted.Task;
29+
30+
/// <summary>
31+
/// Intentionally does NOT call base.DisposeAsync() to prevent _disposeBool from being set.
32+
/// This simulates the behavior before the fix was implemented.
33+
/// </summary>
34+
public new async ValueTask DisposeAsync()
35+
{
36+
DisposeAsyncWasCalled = true;
37+
await Task.CompletedTask;
38+
}
39+
40+
/// <summary>
41+
/// Explicit interface implementation to ensure our disposal method is called.
42+
/// </summary>
43+
async ValueTask IAsyncDisposable.DisposeAsync()
44+
{
45+
await DisposeAsync();
46+
}
47+
48+
/// <summary>
49+
/// Check if _disposeBool is false, proving we didn't call base.DisposeAsync().
50+
/// This is used by tests to verify that our simulation is working correctly.
51+
/// </summary>
52+
public bool IsWasDisposedFalse()
53+
{
54+
var field = typeof(QuickGrid<TGridItem>).GetField("_wasDisposed", BindingFlags.NonPublic | BindingFlags.Instance);
55+
return field?.GetValue(this) is false;
56+
}
57+
58+
/// <summary>
59+
/// Override OnAfterRenderAsync to simulate the race condition by NOT checking _disposeBool.
60+
/// This exactly replicates the code path that existed before the race condition fix.
61+
/// </summary>
62+
protected override async Task OnAfterRenderAsync(bool firstRender)
63+
{
64+
try
65+
{
66+
if (firstRender)
67+
{
68+
if (JS != null)
69+
{
70+
// Import the JS module (this will trigger our TestJsRuntime's import logic)
71+
var jsModule = await JS.InvokeAsync<IJSObjectReference>("import",
72+
"./_content/Microsoft.AspNetCore.Components.QuickGrid/QuickGrid.razor.js");
73+
await jsModule.InvokeAsync<IJSObjectReference>("init", new object());
74+
}
75+
}
76+
}
77+
finally
78+
{
79+
if (firstRender)
80+
{
81+
_onAfterRenderCompleted.TrySetResult();
82+
}
83+
}
84+
}
85+
}

‎src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/test/GridRaceConditionTest.cs

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using Microsoft.AspNetCore.Components.Rendering;
5+
using Microsoft.AspNetCore.Components.QuickGrid;
6+
using Microsoft.AspNetCore.Components.Test.Helpers;
7+
using Microsoft.Extensions.DependencyInjection;
8+
using Microsoft.JSInterop;
9+
using Xunit.Sdk;
10+
11+
namespace Microsoft.AspNetCore.Components.QuickGrid.Tests;
12+
13+
public class GridRaceConditionTest
14+
{
15+
16+
[Fact]
17+
public async Task CanCorrectlyDisposeAsync()
18+
{
19+
var moduleLoadCompletion = new TaskCompletionSource();
20+
var moduleImportStarted = new TaskCompletionSource();
21+
var testJsRuntime = new TestJsRuntime(moduleLoadCompletion, moduleImportStarted);
22+
var serviceProvider = new ServiceCollection()
23+
.AddSingleton<IJSRuntime>(testJsRuntime)
24+
.BuildServiceProvider();
25+
var renderer = new TestRenderer(serviceProvider);
26+
27+
var testComponent = new SimpleTestComponent();
28+
29+
var componentId = renderer.AssignRootComponentId(testComponent);
30+
renderer.RenderRootComponent(componentId);
31+
32+
// Wait until JS import has started but not completed
33+
await moduleImportStarted.Task;
34+
35+
// Dispose component while JS module loading is pending
36+
testJsRuntime.MarkDisposed();
37+
await renderer.DisposeAsync();
38+
39+
// Complete the JS module loading
40+
moduleLoadCompletion.SetResult();
41+
42+
// Wait until after OnAfterRenderAsync has completed to test the disposal of the jsModule
43+
var notFailingGrid = testComponent.NotFailingGrid;
44+
await notFailingGrid.OnAfterRenderCompleted;
45+
46+
// Assert that init was not called after disposal and JsModule was disposed of
47+
Assert.False(testJsRuntime.InitWasCalledAfterDisposal,
48+
"Init should not be called on a disposed component.");
49+
Assert.True(testJsRuntime.JsModuleDisposed);
50+
}
51+
52+
[Fact]
53+
public async Task FailingQuickGridCallsInitAfterDisposal()
54+
{
55+
var moduleLoadCompletion = new TaskCompletionSource();
56+
var moduleImportStarted = new TaskCompletionSource();
57+
var testJsRuntime = new TestJsRuntime(moduleLoadCompletion, moduleImportStarted);
58+
var serviceProvider = new ServiceCollection()
59+
.AddSingleton<IJSRuntime>(testJsRuntime)
60+
.BuildServiceProvider();
61+
var renderer = new TestRenderer(serviceProvider);
62+
63+
var testComponent = new FailingGridTestComponent();
64+
65+
var componentId = renderer.AssignRootComponentId(testComponent);
66+
renderer.RenderRootComponent(componentId);
67+
68+
// Wait until JS import has started but not completed
69+
await moduleImportStarted.Task;
70+
71+
// Dispose component while JS module loading is pending
72+
testJsRuntime.MarkDisposed();
73+
await renderer.DisposeAsync();
74+
75+
// Complete the JS module loading - this allows the FailingQuickGrid's OnAfterRenderAsync to continue
76+
// and demonstrate the race condition by calling init after disposal
77+
moduleLoadCompletion.SetResult();
78+
79+
// Wait until after OnAfterRenderAsync has completed, to make sure jsmodule import started and the reported issue is reproduced
80+
var failingGrid = testComponent.FailingQuickGrid;
81+
await failingGrid.OnAfterRenderCompleted;
82+
83+
// Assert that init WAS called after disposal
84+
// The FailingQuickGrid's OnAfterRenderAsync should have called init despite being disposed
85+
// The FailingQuickGrid should not have disposed of JsModule
86+
Assert.True(testJsRuntime.InitWasCalledAfterDisposal,
87+
$"FailingQuickGrid should call init after disposal, demonstrating the race condition bug. " +
88+
$"InitWasCalledAfterDisposal: {testJsRuntime.InitWasCalledAfterDisposal}, " +
89+
$"DisposeAsyncWasCalled: {failingGrid.DisposeAsyncWasCalled}, " +
90+
$"_disposeBool is false: {failingGrid.IsWasDisposedFalse()}");
91+
Assert.False(testJsRuntime.JsModuleDisposed);
92+
}
93+
}
94+
95+
internal class Person
96+
{
97+
public int Id { get; set; }
98+
public string Name { get; set; } = string.Empty;
99+
}
100+
101+
internal abstract class BaseTestComponent<TGrid> : ComponentBase
102+
where TGrid : ComponentBase
103+
{
104+
[Inject] public IJSRuntime JSRuntime { get; set; } = default!;
105+
106+
protected TGrid _grid;
107+
public TGrid Grid => _grid;
108+
109+
private readonly List<Person> _people = [
110+
new() { Id = 1, Name = "John" },
111+
new() { Id = 2, Name = "Jane" }
112+
];
113+
114+
protected override void BuildRenderTree(RenderTreeBuilder builder)
115+
{
116+
builder.OpenComponent<TGrid>(0);
117+
builder.AddAttribute(1, "Items", _people.AsQueryable());
118+
builder.AddAttribute(2, "ChildContent", (RenderFragment)(b =>
119+
{
120+
b.OpenComponent<PropertyColumn<Person, int>>(0);
121+
b.AddAttribute(1, "Property", (System.Linq.Expressions.Expression<Func<Person, int>>)(p => p.Id));
122+
b.CloseComponent();
123+
}));
124+
builder.AddComponentReferenceCapture(3, component => _grid = (TGrid)component);
125+
builder.CloseComponent();
126+
}
127+
}
128+
129+
internal class SimpleTestComponent : BaseTestComponent<NotFailingGrid<Person>>
130+
{
131+
public NotFailingGrid<Person> NotFailingGrid => Grid;
132+
}
133+
134+
internal class FailingGridTestComponent : BaseTestComponent<FailingQuickGrid<Person>>
135+
{
136+
public FailingQuickGrid<Person> FailingQuickGrid => Grid;
137+
}
138+
139+
internal class TestJsRuntime(TaskCompletionSource moduleCompletion, TaskCompletionSource importStarted) : IJSRuntime
140+
{
141+
private readonly TaskCompletionSource _moduleCompletion = moduleCompletion;
142+
private readonly TaskCompletionSource _importStarted = importStarted;
143+
private bool _disposed;
144+
145+
public bool JsModuleDisposed { get; private set; }
146+
147+
public bool InitWasCalledAfterDisposal { get; private set; }
148+
149+
public async ValueTask<TValue> InvokeAsync<TValue>(string identifier, object[] args = null)
150+
{
151+
if (identifier == "import" && args?.Length > 0 && args[0] is string modulePath &&
152+
modulePath == "./_content/Microsoft.AspNetCore.Components.QuickGrid/QuickGrid.razor.js")
153+
{
154+
// Signal that import has started
155+
_importStarted.TrySetResult();
156+
157+
// Wait for test to control when import completes
158+
await _moduleCompletion.Task;
159+
return (TValue)(object)new TestJSObjectReference(this);
160+
}
161+
throw new InvalidOperationException($"Unexpected JS call: {identifier}");
162+
}
163+
164+
public void MarkDisposed() => _disposed = true;
165+
166+
public void MarkJsModuleDisposed() => JsModuleDisposed = true;
167+
168+
public void RecordInitCall()
169+
{
170+
if (_disposed)
171+
{
172+
InitWasCalledAfterDisposal = true;
173+
}
174+
}
175+
176+
public ValueTask<TValue> InvokeAsync<TValue>(string identifier, CancellationToken cancellationToken, object[] args) =>
177+
InvokeAsync<TValue>(identifier, args);
178+
}
179+
180+
internal class TestJSObjectReference(TestJsRuntime jsRuntime) : IJSObjectReference
181+
{
182+
private readonly TestJsRuntime _jsRuntime = jsRuntime;
183+
184+
public ValueTask<TValue> InvokeAsync<TValue>(string identifier, object[] args)
185+
{
186+
if (identifier == "init")
187+
{
188+
_jsRuntime.RecordInitCall();
189+
}
190+
return default!;
191+
}
192+
193+
public ValueTask<TValue> InvokeAsync<TValue>(string identifier, CancellationToken cancellationToken, object[] args) =>
194+
InvokeAsync<TValue>(identifier, args);
195+
196+
public ValueTask DisposeAsync() {
197+
_jsRuntime.MarkJsModuleDisposed();
198+
return default!;
199+
}
200+
}

‎src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/test/Microsoft.AspNetCore.Components.QuickGrid.Tests.csproj

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<Project Sdk="Microsoft.NET.Sdk">
1+
<Project Sdk="Microsoft.NET.Sdk">
22

33
<PropertyGroup>
44
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
@@ -8,8 +8,10 @@
88
<ItemGroup>
99
<Reference Include="Microsoft.AspNetCore.Components.QuickGrid" />
1010
</ItemGroup>
11-
12-
11+
12+
<ItemGroup>
13+
<Compile Include="$(ComponentsSharedSourceRoot)test\**\*.cs" LinkBase="Helpers" />
14+
</ItemGroup>
1315

1416
<ItemGroup>
1517
<Using Include="Xunit" />

‎src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/test/NotFailingQuickGrid.cs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Text;
7+
using Microsoft.JSInterop;
8+
9+
namespace Microsoft.AspNetCore.Components.QuickGrid.Tests;
10+
/// <summary>
11+
/// A QuickGrid implementation that uses the same implementation of the basic QuickGrid with the additions of the OnAfterRenderCompleted task.
12+
/// </summary>
13+
/// /// <typeparam name="TGridItem">The type of data represented by each row in the grid.</typeparam>
14+
internal class NotFailingGrid<TGridItem> : QuickGrid<TGridItem>
15+
{
16+
[Inject] private IJSRuntime JS { get; set; } = default!;
17+
18+
private readonly TaskCompletionSource _onAfterRenderCompleted = new();
19+
20+
/// <summary>
21+
/// Task that completes when OnAfterRenderAsync has finished executing.
22+
/// This allows tests to wait deterministically for the race condition to occur.
23+
/// </summary>
24+
public Task OnAfterRenderCompleted => _onAfterRenderCompleted.Task;
25+
26+
protected override async Task OnAfterRenderAsync(bool firstRender)
27+
{
28+
try
29+
{
30+
if (firstRender)
31+
{
32+
await base.OnAfterRenderAsync(firstRender);
33+
}
34+
}
35+
finally
36+
{
37+
if (firstRender)
38+
{
39+
_onAfterRenderCompleted.TrySetResult();
40+
}
41+
}
42+
}
43+
}

0 commit comments

Comments
 (0)