Fix the intermittent Interop unit test failures - #369
Draft
damyanpetev wants to merge 3 commits into
Draft
Conversation
A component drains _messageQueue from two places: QueueUpdate defers the flush onto the renderer's dispatcher, while an API call flushes inline on the caller's thread so its own message stays ordered behind the queue. On the dispatcher those are serialised; off it the two run at once on an unsynchronised LinkedList, which throws out of RemoveFirst, tears nodes so a message vanishes, or lets both drains race to igSendMessage and hand the client two updates in the wrong order - and ref updates apply last-write-wins there, so an inverted pair silently keeps the older value. Reaching this needs no misuse: JsonDataSource.OnCollectionChanged runs on whichever thread mutated the bound collection and goes straight to SendMessage, so filling a bound ObservableCollection from a background load is enough. The added test does exactly that, with a single writer so the collection itself is never used concurrently, and crashed six times out of six before this change. One reentrant lock now covers the queue and the _updateQueued flag that decides when it is flushed, held across the whole drain rather than just the dequeue - two flushes that each took one message would still hand them over in whichever order they got there. _invokeId goes atomic while here: it is shared by every instance and incremented from whichever thread invokes, so a plain ++ can hand two calls the same id, and two calls on one component with the same id collide in _methodTasks. This also settles the intermittent SnackbarTests.Methods_FollowContract failure, whose "The LinkedList is empty" came from the contract test calling ShowAsync off the dispatcher. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bUnit records every JS call on an unsynchronised list: eight threads calling InvokeAsync on its JSInterop throw "An item with the same key has already been added" and "Destination array was not long enough", a List<T>.Add resize race. So two concurrent igSendMessage calls do not merely arrive out of order, they can drop a message outright - and no retry budget recovers one that was never recorded. That is the intermittent CheckboxTests.Binds_FollowContract failure in #329: "binding transmitted no Change event registration", reported after two seconds of looking for a message that had been sent. An API call flushes the component's queue inline on the calling thread, so a test that calls into a component off the dispatcher sends concurrently with the flush the renderer is running for that component. Everything that makes a component transmit now goes through InteropHarness.OnDispatcher: contract invocations, because application code calls component APIs from event handlers, and the harness's JS-to-.NET entries, because that is where Blazor delivers the real ones. The generic overload hands back a still-running task rather than awaiting on the dispatcher. Awaiting there holds the dispatcher until that task completes, which deadlocks anything whose completion needs the dispatcher in turn - a deferred return, delivered by a later JS message, is exactly that. Checked with a temporary assertion on Dispatcher.CheckAccess() in the send handler: no send in the suite leaves the dispatcher now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
damyanpetev
force-pushed
the
dpetev/interop-unit-flicker
branch
from
August 26, 2026 08:41
55274b6 to
b3b2df5
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Improves interop reliability by synchronizing renderer messages and aligning tests with Blazor dispatcher behavior.
Changes:
- Protects message queues and invocation IDs from concurrency.
- Routes test interop through the renderer dispatcher.
- Adds settled-traffic handling and a threading regression test.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/componentsBase/BaseRendererControl.cs |
Synchronizes queue access and invocation IDs. |
tests/IgniteUI.Blazor.Tests/BlazorComponentTestBase.cs |
Configures harness dispatchers. |
tests/IgniteUI.Blazor.Tests/ComponentWithContractTestBase.cs |
Dispatches contract invocations. |
tests/IgniteUI.Blazor.Tests/Interop/InteropHarness.cs |
Adds dispatcher helpers. |
tests/IgniteUI.Blazor.Tests/Interop/RendererMessageInteropHarness.cs |
Dispatches callbacks and settles traffic reads. |
tests/IgniteUI.Blazor.Tests/InteropReadinessTests.cs |
Dispatches readiness API calls. |
tests/IgniteUI.Blazor.Tests/InteropThreadingTests.cs |
Adds background collection regression coverage. |
tests/IgniteUI.Blazor.Tests/MethodInteropTests.cs |
Dispatches deferred method invocation. |
tests/IgniteUI.Blazor.Tests/ScriptPropTests.cs |
Dispatches script-property clearing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ScriptPropTests read back "Change" instead of "handleChangeScript" for IgbTabs, whose ChangeScript and Change registration share the ref name <containerId>/Change: the constructor installs its own handler through EnsureChangeHandled before the parameter is applied, so FindPropertyUpdate depends on newest-wins to tell them apart. Dumping the recorded traffic on failure showed the wire order was right every time - the scan had landed between the two sends of one flush, seen only the registration, and returned it without retrying, because it had found something. FindPropertyUpdate now waits for the instance's own traffic to stop moving before deciding, so no later message can supersede what it read. Only that instance's traffic can, so other components rendered by the same test do not hold it up. Counting needs no parsing, so only the settled record is parsed - which leaves the suite faster than before, the settle replacing the 25ms retry sleeps it used to spend waiting on a flush that had not started. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> x
damyanpetev
force-pushed
the
dpetev/interop-unit-flicker
branch
from
August 26, 2026 14:37
b3b2df5 to
fa6e1b8
Compare
Comment on lines
+38
to
+40
| // count shows neither. Only the renderer drains here, so this says nothing about two | ||
| // drains interleaving - that is what holding the lock across the whole drain is for. | ||
| Assert.Equal(Enumerable.Range(0, Rows), Interop.DataItemInsertions(Interop.ContainerIdOf(cut))); |
Comment on lines
+229
to
+230
| Thread.Sleep(1); | ||
| if (SendCountFor(containerId) == sends) |
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 #329
Three different intermittent failures in
IgniteUI.Blazor.Teststurned out to have three different causes, all of them variations on one theme: an interop message being sent from a thread other than the renderer's dispatcher. All three are reproduced locally, and each cause is fixed in its own commit.One of the three is a genuine product bug that reproduces from ordinary application code, with no test involvement at all.
The failures
SnackbarTests.Methods_FollowContract(run)InvalidOperationException: The LinkedList is emptyout ofBaseRendererControl.Update()ScriptPropTests.ScriptProps_TransmitScriptRefs(IgbTabs)(run, run)Expected: "handleChangeScript"/Actual: "Change"CheckboxTests.Binds_FollowContract(#329)binding transmitted no "Change" event registrationAll intermittent, all green on re-run.
How component messaging actually works
Every
BaseRendererControlowns aLinkedList<RendererMessage> _messageQueueand reaches it through two quite different paths.The deferred path. A property setter marks state dirty, which appends a message and asks for a flush:
QueueUpdatedeliberately does not flush inline — it defers:The deferral matters:
Updateserialises the component (ProcessMessage→Serialize()), and doing that inline from a property setter would serialise a half-applied component and emit one description per dirty property instead of one per batch. So the flush hops onto the thread pool and then posts back to the renderer's dispatcher viaInvokeAsync.Updatedrains FIFO:The immediate path. An API call cannot be deferred — the caller is awaiting a return value — so
InvokeMethod/InvokeMethodSyncflush synchronously, on the calling thread, to preserve ordering against the message they are about to send:Both paths are correct on their own. Together they mean the queue is drained from two places, and
Updateis reachable from any thread the component is driven from. On the dispatcher that is harmless, because the dispatcher serialises everything. Off it, two flushes run at once on aLinkedListwith no synchronisation, and the failure modes are:Count > 0thenRemoveFirst()on an emptied list →InvalidOperationException: The LinkedList is empty.AddLastinterleaved withRemoveFirst→ torn nodes:NullReferenceException, or a message silently dropped.igSendMessage→ the client receives two messages in the wrong order. Since the client applies ref updates last-write-wins, an inverted pair means the older value is the one that sticks — silently.The three causes
1. The queue is not thread-safe (product). Reachable from application code — see below. This is what killed
SnackbarTests: the contract test calledShowAsync()from the xUnit thread, so the inline drain ran concurrently with the renderer's.2. The harness read a partially flushed queue (test).
IgbTabs.ChangeScriptand theChangeevent registration ride the same ref name (<containerId>/Change), becauseIgbTabs' constructor installs its own handler viaEnsureChangeHandled()before the parameter is applied.FindPropertyUpdatetherefore relies on "newest wins". But a flush hands its messages to JS one at a time, and dumping the recorded traffic on failure showed the wire order was always correct — the scan had simply landed between the two sends, seen only the event registration, and returned it. It never retried, because it had found something.3. bUnit's invocation recorder is not thread-safe (test). This is the one that explains
CheckboxTests, where the message was never observed even after ~2s of retrying. A direct probe — eight threads callingInvokeAsyncon bUnit's JSInterop — throwsArgumentException: An item with the same key has already been addedandDestination array was not long enough, i.e. an unsynchronisedList<T>.Addresize. So a raced send is not merely late; it can be lost permanently, which no retry budget can recover. Any two concurrentigSendMessagecalls corrupt the record, and the immediate path made those concurrent whenever a test drove a component off the dispatcher.The product bug reproduces without tests
The library itself has exactly one off-dispatcher hop, and it immediately re-enters the dispatcher. But a component reaches its queue from wherever it is driven from, and
JsonDataSource.OnCollectionChangedruns synchronously on whichever thread mutated the bound collection, straight intoSendMessage→AddLast, with no dispatcher hop.So this is enough — bind a collection, fill it from a background load:
A single writer, so the
ObservableCollectionis never used concurrently; no component API called off the dispatcher; no test-only threading. The only thing seeing two threads is the component's own message queue: this task's change notifications, and the flush the renderer is running for the component.Without the fix this crashes 6 times out of 6 (
NullReferenceExceptioninsideLinkedList.AddLast); with it, clean. It is kept as the regression test.The fixes
1.
fix: guard the renderer message queue against concurrent accessOne reentrant lock over
_messageQueueand the_updateQueuedflag that decides when it is flushed, held across the whole drain rather than just the dequeue — two flushes that each took one message would still hand them to the client in whichever order they got there. Also makes the shared_invokeIdcounterInterlocked: a plain++can hand two calls the same id, and two calls on one component with the same id collide in_methodTasks.2.
test: drive components through the renderer's dispatcherAdds
InteropHarness.OnDispatcherand routes everything that makes a component transmit through it: contract method/getter invocations, the harness's JS-to-.NET entries (RaiseEvent,MakeReady,CompleteDeferred— Blazor delivers the real ones on the dispatcher), and the script-prop clear. The generic overload deliberately hands back a still-running task instead of awaiting on the dispatcher; awaiting there holds the dispatcher until the task completes, which deadlocks a deferred return whose completion needs the dispatcher in turn.Verified with temporary instrumentation asserting
Dispatcher.CheckAccess()on every send:zero off-dispatcher sends across all 985 tests, where previously the immediate path produced them routinely.
3.
test: read interop traffic only once it has settledFindPropertyUpdatenow waits for the instance's own traffic to stop moving before deciding, so it cannot read a value that the very next message supersedes. Counting needs no JSON parsing, so only the settled record is parsed — which made the suite faster, because the settle replaces the old 25ms retry sleeps.Verification
The LinkedList is emptyfrom a property-set + API-call loop (~400ms), andExpected: "handleChangeScript" / Actual: "Change"in 4 of 5 runs.DOTNET_PROCESSOR_COUNT=1with 24 CPU hogs, to approximate CI's three concurrent per-TFM processes.The product change is not required to fix the flakes. With it reverted the suite is 12/12 green — the dispatcher routing is what fixes them. It is included because it is a real bug in its own right, and can be judged separately.
Notes for review
ProcessMessage, so acrossSerialize()and a JS interop call. This should be safe: it is per-component and reentrant, and nothing inside waits on another thread (SendJsonfiresInvokeAsyncwithout awaiting; the sync path blocks on its own thread), so there is no cycle. It is the most reviewable claim here. The narrower alternative is locking only the enqueue/dequeue, which stops the crash but leaves send ordering unguarded.IgbTabs.ChangeScriptsharing a ref name with the internalChangeregistration looks like a real product wrinkle rather than a test artifact: whether a user'sChangeScriptwins depends on ordering against the handler the constructor installs. Left alone as out of scope, but it may deserve its own look.