Skip to content

Dpetev/queue lock fixes followup - #394

Draft
damyanpetev wants to merge 2 commits into
dpetev/interop-unit-flickerfrom
dpetev/queue-lock-fixes-followup
Draft

Dpetev/queue lock fixes followup#394
damyanpetev wants to merge 2 commits into
dpetev/interop-unit-flickerfrom
dpetev/queue-lock-fixes-followup

Conversation

@damyanpetev

Copy link
Copy Markdown
Member

Two product races in BaseRendererControl, both of the same class as the message-queue lock in #369: state reached from off the Blazor dispatcher, where a timer, a background task or a bound collection filled by one drives a component outside the renderer's thread. Neither can produce a test flake, which is why they are not in that PR.

The invocation bookkeeping

_methodTasks and _methodReturns pair a call with its return. The caller registers its TaskCompletionSource after awaiting the send, so on the dispatcher that lands on the same thread as OnInvokeReturn, and off it on a pool thread — two threads writing plain Dictionary instances.

Atomic collections would not have been enough, because the two maps are a handshake rather than storage: OnInvokeReturn completes the task if it finds one and otherwise stores the value, while the caller registers its task and then looks for a stored value. With per-operation atomicity and no lock, a return arriving between those two steps is stored by nobody's reader and the call awaits forever.

Both maps now go through the _semLock that was already declared for them and referenced only from commented-out code. The task is completed outside the lock, since the continuation runs inline on the completing thread.

ConcurrentApiCalls_KeepInvocationBookkeepingIntact drives eight threads through one component and fails 4 runs out of 5 without the lock, with Operations that change non-concurrent collections must have exclusive access. It also asserts every call got its own id, which is what fails if _invokeId goes back to a plain ++.

Teardown

The disposed check sat outside the queue's lock, so a producer or an API call could pass it and still send after disposal — including after the component's object reference is disposed, which hands a disposed handle to JS. The check now sits inside that lock at the enqueue, both immediate sends and the drain, and DisposeAsync publishes the flag through the same lock, which also makes its own double-dispose check atomic.

This surfaced a separate, pre-existing bug. DisposeAsync sets disposedValue before calling TrySendCleanupAsync, and SendMessageImmediate refuses to send on that flag — so the cleanup message is never transmitted. Disposing a component records zero cleanup sends. It predates #369 and is untouched here.

Two existing tests in BaseRendererControlDisposalTests stub igSendMessage to throw during cleanup and assert DisposeAsync does not throw; since no cleanup send happens, that handler never fires and both pass vacuously.

DisposeAsync_StopsAFlushScheduledBeforeIt holds the thread pool so a flush is scheduled but cannot run, disposes, then releases the pool: the pending flush must find nothing to send. It passes with the fix and fails 3 runs out of 3 with both the drain's check and teardown's queue clear removed — the drain check alone is redundant while the clear stands, so the test pins the pair rather than either line.

DisposeAsync_SendsNothingAfterCleanup is also added but skipped, asserting cleanup is the last thing an instance sends. Its skip reason names the blocker, so it un-skips once disposal transmits one.

Notes for review

  • The lock spans ProcessMessage, so Serialize() and a JS interop call. Per-component, reentrant, and nothing inside waits on another thread, so there is no cycle — but it is the most reviewable claim here.
  • Adjacent and not addressed: when SendMessageImmediate refuses a send — disposed, or the runtime invalid — the reply is not a JsonElement, so the task is never completed and the call awaits forever, leaking its map entry. Fixing it is a behaviour choice between completing with null and throwing ObjectDisposedException.
  • Also adjacent: _isDirty and _isContentDirty are written by MarkPropDirty on the setter's thread and read by Serialize() under the queue lock on another. Same off-dispatcher condition, reached through property setters rather than bound data.

damyanpetev and others added 2 commits September 4, 2026 19:46
Off the Blazor dispatcher, concurrent calls raced the two maps that pair an invocation with
its return, and a return arriving before its caller registered could leave the call awaiting
forever.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A producer could pass the disposed check and enqueue while teardown was clearing the queue.
The check now sits under the queue's lock, and disposal publishes the flag through it.

The accompanying test is skipped: it orders traffic against the cleanup message, which
disposal never transmits, having set disposedValue before the send that would carry it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@damyanpetev
damyanpetev force-pushed the dpetev/queue-lock-fixes-followup branch from 1e47f27 to 0d497cc Compare September 4, 2026 16:46
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.

1 participant