fix: Race condition between HTML parser disposal and concurrent parsing - #1909
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new test and changelog currently claim “waiting” semantics that are not actually asserted/implemented, so they should be corrected for accuracy (and ideally strengthened with a deterministic regression test for the race).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses an intermittent teardown race where BunitHtmlParser.Dispose() could enumerate its internal document list while another thread was still parsing and adding documents, causing InvalidOperationException: Collection was modified.
Changes:
- Serialize
BunitHtmlParser.Parse()andDispose()via a shared lock to prevent concurrent mutation/enumeration of tracked documents. - Add a new test intended to cover disposal during an active markup assertion.
- Document the fix in the changelog.
File summaries
| File | Description |
|---|---|
| src/bunit/Rendering/BunitHtmlParser.cs | Adds a lock to serialize parsing and disposal to prevent concurrent access to the internal documents list. |
| tests/bunit.tests/Extensions/WaitForHelpers/RenderedComponentWaitForHelperExtensions.cs | Adds a regression-style test around disposal while markup assertions run on the renderer dispatcher. |
| CHANGELOG.md | Adds an Unreleased “Fixed” entry describing the change. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
69
to
+82
| public INodeList Parse([StringSyntax("Html")] string markup) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(markup); | ||
|
|
||
| var document = GetNewDocumentAsync().GetAwaiter().GetResult(); | ||
| lock (parserLock) | ||
| { | ||
| var document = GetNewDocumentAsync().GetAwaiter().GetResult(); | ||
|
|
||
| var (ctx, matchedElement) = GetParseContext(markup, document); | ||
| var (ctx, matchedElement) = GetParseContext(markup, document); | ||
|
|
||
| return ctx is null && matchedElement is not null | ||
| ? ParseSpecial(markup, matchedElement) | ||
| : htmlParser.ParseFragment(markup, ctx!); | ||
| return ctx is null && matchedElement is not null | ||
| ? ParseSpecial(markup, matchedElement) | ||
| : htmlParser.ParseFragment(markup, ctx!); | ||
| } |
|
|
||
| ### Fixed | ||
|
|
||
| - Synchronous context disposal now waits for queued renderer work, and HTML parsing is serialized with parser disposal, preventing teardown races. Reported by [@thimobuchheister](https://github.com/thimobuchheister) in #1892. Fixed by [@linkdotnet](https://github.com/linkdotnet). |
Comment on lines
+243
to
+244
| [Fact(DisplayName = "Disposing a context waits for an active markup assertion on the renderer dispatcher")] | ||
| public async Task Context_disposal_waits_for_active_markup_assertion() |
linkdotnet
force-pushed
the
fix/#1892
branch
from
September 4, 2026 11:08
aa5f3a4 to
603ea9a
Compare
egil
reviewed
Sep 4, 2026
| private readonly IBrowsingContext context; | ||
| private readonly HtmlParser htmlParser; | ||
| private readonly List<IDocument> documents = new(); | ||
| private readonly object parserLock = new(); |
Member
There was a problem hiding this comment.
Can we use the Lock type instead of object?
Collaborator
Author
There was a problem hiding this comment.
We have to #if NET9_0_OR_GREATER - we never use System.Lock until now but that can be done in bunit v3
linkdotnet
force-pushed
the
fix/#1892
branch
from
September 4, 2026 13:57
603ea9a to
0a54860
Compare
Collaborator
Author
Collaborator
Author
|
I would merge if there are no reasons against it |
9 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1892