Skip to content

fix: guard BunitHtmlParser.documents against concurrent access - #1893

Open
thimobuchheister wants to merge 2 commits into
bUnit-dev:mainfrom
thimobuchheister:fix/htmlparser-dispose-race
Open

fix: guard BunitHtmlParser.documents against concurrent access#1893
thimobuchheister wants to merge 2 commits into
bUnit-dev:mainfrom
thimobuchheister:fix/htmlparser-dispose-race

Conversation

@thimobuchheister

Copy link
Copy Markdown

Pull request description

Fixes #1892.

BunitHtmlParser keeps its parsed documents in a plain List<IDocument>. Parse() adds to it and Dispose() enumerates it, and the two can run on different threads, producing an intermittent teardown failure that names an innocent test:

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 overlap: WaitForHelper evaluates its condition on the renderer's dispatcher (OnAfterRender calls completeChecker() directly), so cut.Find(…) inside a WaitForAssertion parses on the dispatcher thread. BunitContext.Dispose(bool) meanwhile runs bunitRenderer?.Dispose() then Services.Dispose() on the test thread — and its "ensure the renderer is disposed before all others" comment does not hold in practice, because BunitRenderer.Dispose(bool) ends with Dispatcher.InvokeAsync(() => base.Dispose(disposing)); and drops the returned task. Renderer disposal is asynchronous and unawaited, so the parser can be disposed mid-parse. Full trace in the issue.

The change is deliberately minimal — a dedicated lock object guarding documents, and Dispose() disposing a snapshot rather than the live list:

IDocument[] documentsToDispose;
lock (documentsLock)
{
    documentsToDispose = documents.ToArray();
    documents.Clear();
}

foreach (var doc in documentsToDispose)
{
    doc.Dispose();
}

I deliberately did not try to fix the unawaited Dispatcher.InvokeAsync in BunitRenderer.Dispose(), even though it is the reason the two sides overlap at all. That is a behavioural change to teardown ordering with a much wider blast radius, and making the parser thread-safe fixes the reported crash on its own. Happy to look at it separately if you would like.

One residual worth naming: a document whose parse completes after Dispose() has taken its snapshot will not be disposed by the parser. That was already true before this change (it would have been added to a list nobody reads again), and closing it properly means deciding what Parse() should do on a disposed parser — out of scope here, but I can follow up.

Verification

New test Dispose_does_not_throw_while_another_thread_is_parsing in BunitHtmlParserTest. It reaches the race through the parser directly, so it needs no renderer. Measured on net10.0, 100 iterations per run:

Result
Without the fix 0/5 runs passed
With the fix 5/5 runs passed (and 10/10 on an earlier shape of the test)

dotnet test tests/bunit.tests -c Release2266/2266 passing, no new warnings (checked in Release, where TreatWarningsAsErrors is on).

Caveat on my local verification: my SDK is 10.0.400-preview, which cannot target net11.0, so I built and ran against net10.0 only. The change uses nothing version-specific (lock + List<T>.ToArray()), so I would expect the other TFMs to be unaffected, but CI will be the real check.

PR meta checklist

  • Pull request is targeted at main branch for code
    or targeted at stable branch for documentation that is live on bunit.dev.
  • Pull request is linked to all related issues, if any.
  • I have read the CONTRIBUTING.md document.

Code PR specific checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

BunitHtmlParser keeps parsed documents in a plain List<IDocument>.
Parse() adds to it, Dispose() enumerates it, and the two can run on
different threads: WaitForHelper evaluates its condition on the
renderer's dispatcher, while BunitContext.Dispose() runs Services
disposal on the test thread. BunitRenderer.Dispose() drops the task
returned by Dispatcher.InvokeAsync(() => base.Dispose(disposing)), so
renderer disposal is asynchronous and unawaited and the parser can be
disposed while a parse is still in flight.

The result is an intermittent

  System.InvalidOperationException: Collection was modified;
  enumeration operation may not execute.
     at Bunit.Rendering.BunitHtmlParser.Dispose()

thrown during teardown, naming whichever test happened to lose the race.

Guard the list with a lock and dispose a snapshot rather than
enumerating the live list.

Fixes bUnit-dev#1892
// live list, which would otherwise throw "Collection was modified" if a
// parse completed mid-loop.
IDocument[] documentsToDispose;
lock (documentsLock)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lock can go through the while Dispose method. Context should be safe here as well. That would also remove the need for re-enumerating.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also Parse should for sure have the lock - dispose and parse can run against each other

var result = await context.OpenNewAsync().ConfigureAwait(false);
documents.Add(result);

lock (documentsLock)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the lock here is not needed - the only usage does a synchronous wait for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants