Fix discarded fire-and-forget Publish calls that ran inline and blocked their caller - #487
Merged
Merged
Conversation
…blocked their caller (#477) Discarding a Task reference (_ = mediator.Publish(...)) does not defer execution: an async method's body runs synchronously up to its first genuine await, so the entire reparse-every -open-feature-file cascade ran inline on the calling didChange/didChangeWatchedFiles handler's thread, blocking every other request queued behind it (confirmed during the #471 investigation). Adds FireAndForgetExtensions (Reqnroll.IdeSupport.Common) with a Func<Task> overload that genuinely backgrounds work via Task.Run and a Task overload that just observes an already-independent task, both logging any fault instead of leaving it unobserved. Replaces the four `_ = _mediator.Publish(...)` sites (BindingRegistryProviderRouter, MembershipIndex x2, LspWorkspaceScopeManager) and the LspServerConnectionService shutdown Task.Run, which lacked an outer catch around one non-critical-path await. The three genuinely-fire-and-forget-safe Task.Run sites already carrying their own full try/catch (ConnectorBindingRegistryProvider.RunDiscoveryAsync, CommentToggleCommandFilter) are left as-is -- they never actually leave a fault unobserved. Also fixes a real bug the change surfaced: the deferred Publish calls must use CancellationToken.None rather than the notification's own request-scoped token, since a background continuation that runs after the request completes would otherwise find that token already cancelled and silently drop the notification (caught by the existing ProjectFilesDelta.feature spec). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…currencyProbeTests Fresh-eyes finding: FireAndForgetExtensions logged only Flatten().InnerException (the first of possibly several aggregated exceptions) on a fault, silently dropping any siblings. Log the flattened AggregateException itself instead, so nothing is lost. CI fix: ConcurrencyProbeTests.Cheap_request_latency_under_concurrent_codeLens_load (a #471 regression gate asserting the *bad* dispatch-pipeline-stall behavior is still present) started failing on this branch once #477's fix landed. Investigation: - #477's fix is a plausible direct cause: it makes BindingRegistryProviderRouter's Publish call genuinely background instead of running its reparse cascade inline on the [Serial]-tagged didChange/didOpen handler that #471 describes as the stall mechanism. - But the improvement is partial, not complete, and the residual magnitude proved highly sensitive to what else was running on the machine: isolated runs measured a consistent ~15x-20x (down from the documented 45x-56x); running the full ~831-test assembly measured ~40x+ in two separate runs, and CI's one failing run measured ~1.3x. Naively flipping the assertion to require good (<5x) behavior would have just moved the flakiness to a different threshold, not removed it. - Root cause of the swing: xUnit's default cross-class parallelization ran this wall-clock latency benchmark concurrently with ~830 unrelated tests, contaminating the measurement with CPU contention that has nothing to do with the LSP dispatch pipeline under test. Disabling parallelization for the assembly (AssemblyInfo.cs) removes that noise source; the benchmark now lands consistently in the ~15x-20x range across repeated full-assembly runs, matching the isolated numbers. - Converted the assertion from a "must reproduce the fully-broken behavior" floor to a regression ceiling (<30x, comfortably above the now-stable ~15x-20x, comfortably below the original 45x-56x) so it still catches a real regression without asserting a specific ratio that isn't yet reliably near 1x. Full elimination of the residual stall is left as a #471 follow-up, not claimed here. Verified: dotnet test on the full LSP.Server.Tests assembly (Release, matching CI) passed 831/831 across three consecutive runs; full solution build + test (Release) all green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Taskreference (_ = _mediator.Publish(...)) is not fire-and-forget: anasyncmethod's body runs synchronously up to its first genuineawait, so a caller that never observes the returnedTaskcan still be blocked for that entire synchronous prefix, and any exception the task throws goes unobserved..csedit could maketextDocument/didChangeitself take several-to-tens of seconds to "complete" from the protocol's perspective — the entire reparse-every-open-feature-file cascade ran inline on the calling thread before the handler returned — blocking every other request queued behind it.FireAndForgetExtensions(Reqnroll.IdeSupport.Common) with aFunc<Task>overload that genuinely backgrounds work viaTask.Runand aTaskoverload for work that's already independently running, both logging any fault instead of leaving it unobserved (anIIdeSupportLoggeroverload and a Microsoft.Extensions.LoggingILoggeroverload, for the two logging abstractions in use across the codebase)._ = _mediator.Publish(...)sites (BindingRegistryProviderRouter,MembershipIndexx2,LspWorkspaceScopeManager) and theLspServerConnectionServiceshutdownTask.Run, which lacked an outer catch around one non-critical-pathawait.Task.Runsites that already carry their own full try/catch (ConnectorBindingRegistryProvider.RunDiscoveryAsync,CommentToggleCommandFilter) as-is — they never actually leave a fault unobserved, so wrapping them would add nothing.Publishcalls must useCancellationToken.Nonerather than the notification's own request-scoped token, since a background continuation that runs after the request completes would otherwise find that token already cancelled and silently drop the notification. Caught by the existingProjectFilesDelta.featurespec, which failed until this was corrected.Fixes #477
Test plan
FireAndForgetExtensionsTestscovering: theFunc<Task>overload genuinely runs off the calling thread, a faultingTask/Func<Task>logs its exception instead of losing it, and a successful task logs nothing.MembershipIndexTests/LspWorkspaceScopeManagerMembershipTeststo poll for the now-genuinely-asynchronousPublishcall instead of asserting immediately.dotnet testacrossReqnroll.IdeSupport.Common.Tests,Reqnroll.IdeSupport.LSP.Server.Tests, andReqnroll.IdeSupport.LSP.Server.Specs— all green,ProjectFilesDelta.feature's bound→unbound scenario in particular confirmed passing (it caught the CancellationToken bug above during development).dotnet test— all green.🤖 Generated with Claude Code