Answer the viewer's port while its render thread is busy - #873
Merged
Merged
Conversation
ViewerServer.Listen awaited its accept without ConfigureAwait(false), and the Windows viewer calls it from its UI thread once the form exists, when that thread already carries a WinForms synchronization context. So every accept resumed on the UI thread, which is the render loop and only pumps between frames. While the render thread was busy - an owning viewer applying an accept waits up to ten seconds on InlineApplier's cross process mutex - no new connection was handled at all: the tray's listings, an attached viewer's polls and the next failing snapshot all waited for the render loop to come round. Reproduced against the real viewer: with one source file's patch mutex held and Accept clicked on that entry, a listfull sent to the viewer did not answer within a second for the whole five seconds the mutex was held, and answered at once after it was let go. With this change it answers throughout. Both awaits in the accept chain take ConfigureAwait(false). The first Accept runs on the caller's thread, so fixing Listen's await alone still parks Accept's continuation on the UI thread. Each connection's handler already ran on the pool. The Mac and Linux heads install no context and were never affected. AnOwnerAnswersWhileTheThreadThatStartedItIsBusy starts the listener under a single threaded context that is never pumped, and asserts a client is answered. It times out on the old code, and on the new code with either await left as it was.
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.
ViewerServer.Listenawaited its accept withoutConfigureAwait(false), and the Windows viewer calls it from its UI thread once the form exists, when that thread already carries a WinForms synchronization context. So every accept resumed on the UI thread, which is the render loop and only pumps between frames. While the render thread was busy - an owning viewer applying an accept waits up to ten seconds onInlineApplier's cross process mutex - no new connection was handled at all: the tray's listings, an attached viewer's polls and the next failing snapshot all waited for the render loop to come round.Reproduced against the real viewer: with one source file's patch mutex held and Accept clicked on that entry, a
listfullsent to the viewer did not answer within a second for the whole five seconds the mutex was held, and answered at once after it was let go. With this change it answers throughout.Both awaits in the accept chain take
ConfigureAwait(false). The firstAcceptruns on the caller's thread, so fixingListen's await alone still parksAccept's continuation on the UI thread. Each connection's handler already ran on the pool. The Mac and Linux heads install no context and were never affected.AnOwnerAnswersWhileTheThreadThatStartedItIsBusystarts the listener under a single threaded context that is never pumped, and asserts a client is answered. It times out on the old code, and on the new code with either await left as it was. The repo otherwise leavesConfigureAwaitoff, so CLAUDE.md says why this one stays.