BunitHtmlParser.Dispose() races Parse() on another thread: "Collection was modified" during test teardown

[thimobuchheister]

Description of the bug

BunitHtmlParser keeps its parsed documents in a plain, non-thread-safe List<IDocument>, and Dispose() enumerates that list directly while Parse() can still be adding to it from another thread. The result is an intermittent teardown failure that names an innocent test and carries no assertion from the test itself:

System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.List`1.Enumerator.MoveNext()
   at Bunit.Rendering.BunitHtmlParser.Dispose() in /_/src/bunit/Rendering/BunitHtmlParser.cs:line 162
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.Dispose()

Why the two sides run concurrently

The relevant code (line numbers from v2.7.2; identical on main):

• private readonly List<IDocument> documents = new(); — line 25.
• Parse() (68) → GetNewDocumentAsync() (151) → documents.Add(result) (154).
• Dispose() (159) → context.Dispose() then foreach (var doc in documents) — line 162.

IRenderedComponent.Nodes (RenderedComponent.cs line 71) is the only caller of Parse(), and WaitForHelper evaluates its condition on the renderer's dispatcher, not on the test thread — OnAfterRender calls completeChecker() directly, and CheckAndInitializeWaiting queues that through an InvokeAsync whose task is not awaited. So a cut.Find(…) inside a WaitForAssertion parses on the dispatcher thread.

Meanwhile BunitContext.Dispose(bool) runs on the test thread:

// Ensure the renderer is disposed before all others,
// otherwise a render cycle may be ongoing and try to access
// the service provider to perform operations.
bunitRenderer?.Dispose();
Services.Dispose();

That ordering does not actually hold, because BunitRenderer.Dispose(bool) (line 542) ends with:

Dispatcher.InvokeAsync(() => base.Dispose(disposing));

and drops the returned task. Renderer disposal is therefore asynchronous and unawaited, so Services.Dispose() can reach BunitHtmlParser.Dispose() while the dispatcher is still parsing. Two threads, one unsynchronised List<T>, and the enumerating side is teardown.

Steps to reproduce

The race is reachable directly through the parser, without a renderer. This fails reliably on main:

using var cts = new CancellationTokenSource();
using var parser = new BunitHtmlParser();

var parsing = Task.Run(() =>
{
    while (!cts.IsCancellationRequested)
    {
        try { parser.Parse("<p>Hello world</p>"); } catch { }
    }
});

await Task.Delay(1);
parser.Dispose();   // throws InvalidOperationException

Measured over 100 iterations per run: 0/5 runs passed unpatched, 5/5 passed with the fix below.

Expected behaviour

Dispose() should not throw when a parse is in flight on another thread.

Version info

• bUnit: 2.7.2, and main (Dispose() is byte-identical at v2.7.2, v2.9.0 and main; nothing in the changelog between them touches it)
• .NET: net10.0
• Test framework: xunit.v3

Suggested fix

Guard documents with a lock and have Dispose() enumerate a snapshot rather than the live list. I have this working with a regression test and will open a PR referencing this issue.

Downstream context, in case it is useful: this surfaced as a random red test in a ~5,300-test bUnit suite, roughly half of full-suite CI runs while it was being observed. It reads as "that test is broken" when the named test is innocent of the assertion, which is what makes it expensive to diagnose.

[linkdotnet]

Hey @thimobuchheister - thanks for filing the issue and PR.

A few things that may be a bit misleading in the description here and I do think need proper analysis:
Dispatcher.InvokeAsync(() => base.Dispose(disposing)) runs inline on the test thread whenever there is nothing in the task queue of Blazor. That's the normal case, and it means the renderer is fully disposed before Services.Dispose() reaches the parser. That is not a general source of the issue in itself.

Dispatcher.InvokeAsync in general is quite synchronous in our use cases. Otherwise our build-pipeline should be constantly red I assume. Of course forcing it via Task.Run will break it - but that isn't how it is used throughout the code base.

IRenderedComponent.Nodes (RenderedComponent.cs line 71) is the only caller of Parse()

No there are other usages - like semantically comparing Markup. But that doesn't change much here to be honest.

But as said - that is the "general" behaviour so for sure we have an issue here. Furthermore WaitForXXXruns a timer that for sure is not funneled through the dispatcher right now, so those two can race against each other (dispose for example).