Dpetev/queue lock fixes followup - #394
Draft
damyanpetev wants to merge 2 commits into
Draft
Conversation
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
force-pushed
the
dpetev/queue-lock-fixes-followup
branch
from
September 4, 2026 16:46
1e47f27 to
0d497cc
Compare
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.
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
_methodTasksand_methodReturnspair a call with its return. The caller registers itsTaskCompletionSourceafter awaiting the send, so on the dispatcher that lands on the same thread asOnInvokeReturn, and off it on a pool thread — two threads writing plainDictionaryinstances.Atomic collections would not have been enough, because the two maps are a handshake rather than storage:
OnInvokeReturncompletes 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
_semLockthat 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_KeepInvocationBookkeepingIntactdrives eight threads through one component and fails 4 runs out of 5 without the lock, withOperations that change non-concurrent collections must have exclusive access. It also asserts every call got its own id, which is what fails if_invokeIdgoes 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
DisposeAsyncpublishes the flag through the same lock, which also makes its own double-dispose check atomic.This surfaced a separate, pre-existing bug.
DisposeAsyncsetsdisposedValuebefore callingTrySendCleanupAsync, andSendMessageImmediaterefuses to send on that flag — so the cleanup message is never transmitted. Disposing a component records zerocleanupsends. It predates #369 and is untouched here.Two existing tests in
BaseRendererControlDisposalTestsstubigSendMessageto throw during cleanup and assertDisposeAsyncdoes not throw; since no cleanup send happens, that handler never fires and both pass vacuously.DisposeAsync_StopsAFlushScheduledBeforeItholds 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_SendsNothingAfterCleanupis also added but skipped, assertingcleanupis the last thing an instance sends. Its skip reason names the blocker, so it un-skips once disposal transmits one.Notes for review
ProcessMessage, soSerialize()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.SendMessageImmediaterefuses a send — disposed, or the runtime invalid — the reply is not aJsonElement, so the task is never completed and the call awaits forever, leaking its map entry. Fixing it is a behaviour choice between completing withnulland throwingObjectDisposedException._isDirtyand_isContentDirtyare written byMarkPropDirtyon the setter's thread and read bySerialize()under the queue lock on another. Same off-dispatcher condition, reached through property setters rather than bound data.