Skip to content

Commit b93370f

Browse files
Xavier Klausenerclaude
andcommitted
fix: stop the WaitFor timer when the first check passes before it is started
When a WaitFor* check passes synchronously, the helper disposes itself from inside the constructor, while the timer field is still null. The constructor then looks at WaitTask.IsCompleted to decide whether to start the timer, but WaitTask is a WhenAny(...).Unwrap() that completes on the thread pool, so it usually still reads as not completed at that point and the timer is started anyway. The later Dispose() from the using block returns early because the helper is already disposed, and nothing ever stops the timer. The timer's state is the helper, so until the timeout elapses it roots the helper, the rendered component, the renderer and everything the renderer holds. With bUnit's default timeout of one second that goes unnoticed. With a long DefaultWaitTimeout, a test suite keeps every page it rendered in memory for its whole run. The constructor now also checks isDisposed before starting the timer, and disposes the timer right after creating it if the check disposed the helper in between. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 31d1b96 commit b93370f

4 files changed

Lines changed: 75 additions & 1 deletion

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ All notable changes to **bUnit** will be documented in this file. The project ad
66

77
## [Unreleased]
88

9+
### Fixed
10+
11+
- A `WaitFor*` call whose first check passed left its timeout timer running, which kept the helper, the renderer and everything the renderer holds alive until the timeout elapsed. With a long `DefaultWaitTimeout`, a test suite could hold every rendered component in memory for its whole run. Fixed by [@xaviergxf](https://github.com/xaviergxf).
12+
913
## [2.12.5] - 2026-10-19
1014

1115
### Fixed
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<!-- https://learn.microsoft.com/dotnet/fundamentals/package-validation/diagnostic-ids -->
3+
<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
4+
<Suppression>
5+
<DiagnosticId>CP0001</DiagnosticId>
6+
<Target>T:Bunit.TestDoubles.BunitSignOutSessionStateManager</Target>
7+
<Left>lib/net10.0/bunit.dll</Left>
8+
<Right>lib/net11.0/bunit.dll</Right>
9+
</Suppression>
10+
</Suppressions>

‎src/bunit/Extensions/WaitForHelpers/WaitForHelper.cs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,11 @@ protected WaitForHelper(
9494

9595
// If the initial check did not complete successfully,
9696
// start the timer and recheck after every render until the timer expires.
97-
if (!WaitTask.IsCompleted)
97+
// A check that completed synchronously has already disposed this helper
98+
// before WaitTask, which completes on the thread pool, reflects it. A timer
99+
// started after that would never be disposed, and would keep this helper,
100+
// and through it the renderer, alive until the timeout elapsed.
101+
if (!isDisposed && !WaitTask.IsCompleted)
98102
{
99103
timer = new Timer(
100104
static (state) =>
@@ -112,6 +116,12 @@ protected WaitForHelper(
112116
this,
113117
GetRuntimeTimeout(timeout),
114118
Timeout.InfiniteTimeSpan);
119+
120+
// The check may have completed on the renderer's dispatcher, and disposed
121+
// this helper while the timer field was still null, between the test above
122+
// and the assignment. Dispose(bool) found no timer to stop in that case.
123+
if (isDisposed)
124+
timer.Dispose();
115125
}
116126
}
117127

‎tests/bunit.tests/Extensions/WaitForHelpers/RenderedComponentWaitForHelperExtensions.cs‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.Runtime.CompilerServices;
12
using AngleSharp.Dom;
23

34
namespace Bunit.Extensions.WaitForHelpers;
@@ -240,6 +241,55 @@ await cut.WaitForAssertionAsync(async () =>
240241
expected.Message.ShouldStartWith(WaitForAssertionHelper<Simple1>.TimeoutMessage);
241242
}
242243

244+
[Fact(DisplayName = "WaitForAssertion does not keep the renderer alive after the first check passes")]
245+
public async Task Test500()
246+
{
247+
// The first call is slow while it is being JIT compiled, which is enough for the wait task
248+
// to complete before the helper decides whether to start its timer, so it is not measured.
249+
RenderAndWaitForAssertionThatPassesAtOnce();
250+
251+
var renderers = Enumerable.Range(0, 10)
252+
.Select(_ => RenderAndWaitForAssertionThatPassesAtOnce())
253+
.ToList();
254+
255+
// Disposing a context queues work that still references the renderer: cancelling its
256+
// unhandled exception task wakes the helper's failure continuation on the thread pool, and
257+
// the renderer's own base disposal goes through the dispatcher. On a busy test host that
258+
// can still be queued when the first collection runs, so the collection is retried for a
259+
// short while. A timer left running roots the renderer for ten minutes, so no number of
260+
// retries makes the leaking case pass.
261+
var alive = renderers.Count;
262+
for (var attempt = 0; attempt < 20 && alive > 0; attempt++)
263+
{
264+
if (attempt > 0)
265+
await Task.Delay(50, Xunit.TestContext.Current.CancellationToken);
266+
267+
#pragma warning disable S1215 // A full collection is what proves the renderers are no longer rooted.
268+
GC.Collect();
269+
GC.WaitForPendingFinalizers();
270+
GC.Collect();
271+
#pragma warning restore S1215
272+
273+
alive = renderers.Count(r => r.IsAlive);
274+
}
275+
276+
alive.ShouldBe(0, "a wait whose first check passed should leave no timer running");
277+
}
278+
279+
// Kept out of the test method so that nothing on its stack frame keeps the context alive.
280+
[MethodImpl(MethodImplOptions.NoInlining)]
281+
private static WeakReference RenderAndWaitForAssertionThatPassesAtOnce()
282+
{
283+
using var context = new BunitContext();
284+
var cut = context.Render<Simple1>();
285+
286+
// A timeout far longer than the test: a timer left running roots the helper,
287+
// and through it the renderer, until it fires.
288+
cut.WaitForAssertion(() => { }, TimeSpan.FromMinutes(10));
289+
290+
return new WeakReference(context.Renderer);
291+
}
292+
243293
private sealed class ThrowsAfterAsyncOperation : ComponentBase
244294
{
245295
protected override async Task OnInitializedAsync()

0 commit comments

Comments
 (0)