Skip to content

fix: latestRenderNodes was not thread-safe (fixes #1938) - #1948

Merged
linkdotnet merged 2 commits into
mainfrom
fix/#1938
Oct 18, 2026
Merged

linkdotnet merged 2 commits into
mainfrom
fix/#1938

Conversation

@linkdotnet

@linkdotnet linkdotnet commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Nodes cached a parse of the old markup when a render (e.g. timer) finished mid-parse; Find then served stale nodes with removed event handler IDs → UnknownEventHandlerIdException.

test thread                          dispatcher (UpdateMarkup)
latestRenderNodes is null
read Markup (old)                    latestRenderNodes = null
parse old markup...                  write new Markup
latestRenderNodes = old nodes  ← stale until next render

Nodes are now cached with the markup string they were parsed from and re-parsed when it no longer matches:

 Nodes
-  return latestRenderNodes ??= Parse(Markup)
+  current = Markup
+  if cached is null or cached.Markup is not current
+    cached = (current, Parse(current))
+  return cached.Nodes

Not needed in v3: #1910 (shared render tree) replaces this with immutable markup snapshots — take its side on conflict.

Evidence

Test030 (deterministic): render <p>1</p> without reading Nodes; hold BunitHtmlParser's parse lock so a reader thread pauses after capturing the old markup; render <p>2</p>; release; cut.Find("p").TextContent.ShouldBe("2").

  • Before (original getter): failed 5/5 — should be "2" but was "1"
    After: passed 5/5; full net10.0 suite 2274/2274 passed

Merge Danger

Door: two-way

Blast Radius: small

Only RenderedComponent.Nodes changes; same instance/new-instance semantics as before (Test006–008 unchanged).

Copilot AI balanced review requested due to automatic review settings October 18, 2026 16:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The regression test needs controlled synchronization to reliably detect the original race.

1 open issue
What changed in this PR

Fixes stale DOM-node caching in bUnit when rendering overlaps HTML parsing.

Changes:

  • Associates cached nodes with their markup snapshot.
  • Adds a cross-thread rendering regression test.
  • Documents the fix in the changelog.
File Description
tests/​bunit.tests/​Rendering/​RenderedComponentTest.cs Adds concurrent rendering coverage.
src/​bunit/​Rendering/​RenderedComponent.cs Validates cached nodes against current markup.
CHANGELOG.md Records the stale-node fix.

Review effort: Balanced

Comment on lines +308 to +311
var render = Task.Run(() => cut.Render(ps => ps.AddChildContent(content)), Xunit.TestContext.Current.CancellationToken);
while (!render.IsCompleted)
_ = cut.Nodes;
await render;
@linkdotnet
linkdotnet merged commit 69ad211 into main Oct 18, 2026
10 checks passed
@linkdotnet
linkdotnet deleted the fix/#1938 branch October 18, 2026 16:56
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.

2 participants