Unify the behavior of AsyncBatchingWorkQueue's Dispose() and clean up uses - #84712
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates AsyncBatchingWorkQueue so that shutting it down is consistent and reliable: Dispose() (and an optional external CancellationToken) now fully shuts the queue down by canceling work, clearing queued items, and preventing any future queueing. It then migrates a broad set of call sites to rely on disposal rather than “shutdown” cancellation tokens, and adds focused unit tests for the new shutdown semantics.
Changes:
- Update
AsyncBatchingWorkQueueto treatDispose()and constructor cancellation as equivalent shutdown mechanisms (including preventing post-shutdown queueing). - Migrate many queue usages to dispose queues (and related owners) instead of passing shutdown tokens purely for queue cancellation.
- Add new
AsyncBatchingWorkQueueunit tests covering dispose/cancellation shutdown behavior.
Show a summary per file
| File | Description |
|---|---|
| src/Workspaces/Remote/ServiceHub/Services/SemanticClassification/RemoteSemanticClassificationService.Caching.cs | Switches queue shutdown from CTS-cancel to disposing the queue. |
| src/Workspaces/Remote/ServiceHub/Services/ProcessTelemetry/RemoteProcessTelemetryService.PerformanceReporter.cs | Removes shutdown token usage; adds IDisposable to dispose the queue. |
| src/Workspaces/Remote/ServiceHub/Services/ProcessTelemetry/RemoteProcessTelemetryService.cs | Disposes the performance reporter during service disposal. |
| src/Workspaces/CoreTest/UtilityTest/AsyncBatchingWorkQueueTests.cs | Adds regression/unit tests for dispose/cancellation shutdown semantics. |
| src/Workspaces/Core/Portable/Workspace/Workspace.cs | Replaces CTS-cancel shutdown with disposing workspace queues. |
| src/Workspaces/Core/Portable/Workspace/Workspace_SourceGeneration.cs | Removes now-unneeded CTS field for the source generator update queue. |
| src/Workspaces/Core/Portable/Workspace/ProjectSystem/ReferenceFileChangeTracker.cs | Removes shutdown token parameter from queue usage. |
| src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProjectFactory.cs | Removes shutdown token threading through to reference trackers. |
| src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs | Disposes file-change queue when project is removed. |
| src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfoCacheService.cs | Uses queue disposal instead of CTS-cancel for shutdown. |
| src/VisualStudio/VisualBasic/Impl/LanguageService/VisualBasicCreateServicesOnTextViewConnection.vb | Removes threading-context dependency from base constructor call. |
| src/VisualStudio/CSharp/Impl/SemanticSearch/SemanticSearchToolWindowImpl.cs | Stops passing CancellationToken.None; ensures log queue is disposed. |
| src/VisualStudio/CSharp/Impl/LanguageService/CSharpCreateServicesOnTextViewConnection.cs | Removes threading-context dependency from base constructor call. |
| src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs | Uses using for checksum updater to ensure disposal. |
| src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs | Switches from Shutdown() to Dispose() on checksum updater. |
| src/VisualStudio/Core/Impl/SolutionExplorer/SymbolTree/RootSymbolTreeItemSourceProvider.cs | Removes disposal token usage; adds IDisposable to dispose update queue. |
| src/VisualStudio/Core/Impl/CodeModel/ProjectCodeModelFactory.cs | Removes disposal token usage; adds IDisposable to dispose batching queue. |
| src/VisualStudio/Core/Def/Workspace/VisualStudioSourceGeneratorTelemetryCollectorWorkspaceServiceFactory.cs | Removes threading-context token usage; adds IDisposable to dispose work queue. |
| src/VisualStudio/Core/Def/TaskList/ExternalErrorDiagnosticUpdateSource.cs | Removes disposal token usage; disposes batching queue on shutdown. |
| src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs | Switches checksum updater shutdown to disposal. |
| src/VisualStudio/Core/Def/ProjectSystem/VisualStudioWorkspaceImpl.cs | Removes passing disposal token into factory; explicitly disposes factory on workspace dispose. |
| src/VisualStudio/Core/Def/PdbSourceDocument/PdbSourceDocumentOutputWindowLogger.cs | Uses queue disposal instead of CTS-cancel for shutdown. |
| src/VisualStudio/Core/Def/LanguageService/AbstractCreateServicesOnTextViewConnection.cs | Removes disposal token threading; ensures queue is disposed when listener is disposed. |
| src/VisualStudio/Core/Def/ExternalAccess/UnitTesting/VisualStudioGlobalOperationNotificationService.cs | Moves disposal responsibility to base; ensures base queue is disposed before monitor. |
| src/VisualStudio/Core/Def/DocumentOutline/DocumentOutlineViewModel.cs | Removes disposal token usage; explicitly disposes work queues on view model dispose. |
| src/VisualStudio/Core/Def/DesignerAttribute/VisualStudioDesignerAttributeService.cs | Removes disposal token usage; adds IDisposable to dispose work queues. |
| src/LanguageServer/Protocol/Handler/AbstractRefreshQueue.cs | Removes CTS usage; disposes refresh queue during disposal. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerWorkspaceFactory.cs | Stops passing CancellationToken.None into factory constructors. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs | Removes CancellationToken.None plumbing to batching queues. |
| src/Features/Core/Portable/ExternalAccess/UnitTesting/Notification/AbstractGlobalOperationNotificationService.cs | Removes disposal token parameter; adds IDisposable to dispose event batching queue. |
| src/Features/Core/Portable/Completion/Providers/ImportCompletionProvider/DefaultTypeImportCompletionCacheServiceFactory.cs | Removes cancellation token threading into cache service factory base. |
| src/Features/Core/Portable/Completion/Providers/ImportCompletionProvider/DefaultExtensionMethodImportCompletionCacheServiceFactory.cs | Removes cancellation token threading into cache service factory base. |
| src/Features/Core/Portable/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionCacheServiceFactory.cs | Drops disposal token; makes created cache service disposable to dispose its queue. |
| src/EditorFeatures/Core/SolutionEvents/HostLegacySolutionEventsWorkspaceEventListener.cs | Removes disposal token usage; adds IDisposable to dispose event queue. |
| src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.WorkspaceChangedEventSource.cs | Stops passing CancellationToken.None; ensures internal delay queue is disposed on disconnect. |
| src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs | Converts shutdown pattern to IDisposable, disposing internal queues. |
| src/EditorFeatures/Core/IntelliSense/ImportCompletionCacheService/EditorTypeImportCompletionCacheServiceFactory.cs | Deletes editor-layer factory now that shutdown is handled via disposal. |
| src/EditorFeatures/Core/IntelliSense/ImportCompletionCacheService/EditorExtensionMethodImportCompletionCacheServiceFactory.cs | Deletes editor-layer factory now that shutdown is handled via disposal. |
| src/EditorFeatures/Core/Copilot/CopilotWpfTextCreationListener.cs | Removes disposal token usage; adds IDisposable to dispose completion queue. |
| src/EditorFeatures/Core/CodeDefinitionWindow/DefinitionContextTracker.cs | Removes disposal token usage; adds IDisposable to dispose work queue. |
| src/EditorFeatures/Core/Classification/Syntactic/SyntacticClassificationTaggerProvider.TagComputer.cs | Switches from CTS-cancel to disposing the internal work queue. |
| src/Dependencies/Threading/AsyncBatchingWorkQueue`2.cs | Core behavior change: unify shutdown semantics and prevent post-shutdown queueing. |
| src/Dependencies/Threading/AsyncBatchingWorkQueue`1.cs | Makes cancellation token optional to support new usage patterns. |
| src/Dependencies/Threading/AsyncBatchingWorkQueue`0.cs | Makes cancellation token optional to support new usage patterns. |
Copilot's findings
Comments suppressed due to low confidence (6)
src/Workspaces/CoreTest/UtilityTest/AsyncBatchingWorkQueueTests.cs:44
- This async lambda has no await and will trigger CS1998. Use a non-async lambda returning ValueTask.CompletedTask.
src/Workspaces/CoreTest/UtilityTest/AsyncBatchingWorkQueueTests.cs:62 - This async lambda has no await and will trigger CS1998. Use a non-async lambda returning ValueTask.CompletedTask.
src/Workspaces/CoreTest/UtilityTest/AsyncBatchingWorkQueueTests.cs:83 - This async lambda has no await and will trigger CS1998. Use a non-async lambda returning ValueTask.CompletedTask.
src/Workspaces/CoreTest/UtilityTest/AsyncBatchingWorkQueueTests.cs:99 - This async lambda has no await and will trigger CS1998. Use a non-async lambda returning ValueTask.CompletedTask.
src/Workspaces/CoreTest/UtilityTest/AsyncBatchingWorkQueueTests.cs:120 - This async lambda has no await and will trigger CS1998. Use a non-async lambda returning ValueTask.CompletedTask.
src/Workspaces/CoreTest/UtilityTest/AsyncBatchingWorkQueueTests.cs:135 - This async lambda has no await and will trigger CS1998. Use a non-async lambda returning ValueTask.CompletedTask.
- Files reviewed: 44/44 changed files
- Comments generated: 4
84f1f57 to
2ae71a9
Compare
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
src/Workspaces/Remote/ServiceHub/Services/ProcessTelemetry/RemoteProcessTelemetryService.PerformanceReporter.cs:45
- PerformanceReporter subscribes to IPerformanceTrackerService.SnapshotAdded but Dispose() only disposes the work queue. This keeps the reporter rooted via the event and will keep calling AddWork on a disposed queue. Unsubscribe in Dispose (and avoid an inline lambda so the handler can be removed).
src/Dependencies/Threading/AsyncBatchingWorkQueue`2.cs:154 - AsyncBatchingWorkQueue.Dispose() cancels the queue but leaves the CancellationSeries undisposed. CancellationSeries holds a CTS that should be canceled/disposed to avoid retaining registrations/handles after shutdown. Also, disposing the CancellationTokenRegistration can block if Dispose() is invoked by that registration's callback; doing it synchronously risks deadlock. Dispose the CancellationSeries and dispose the registration in a non-blocking way.
// This must be done outside of the lock: disposing a registration blocks if the registered callback is currently running.
// If we did this inside the lock, the callback might be blocked waiting for a call to Dispose() to release the lock, but the
// caller of Dispose() would be blocked on that registration. If we could drop netstandard support, we could just call Unregister() instead.
_externalCancellationTokenRegistration.Dispose();
}
- Files reviewed: 44/44 changed files
- Comments generated: 0 new
AsyncBatchingWorkQueue was disposable, and also took a CancellationToken which could be used to cancel the work in the queue and also prevent future work. Dispose however was a bit strange: it would cancel work, but not free it and wouldn't prevent queueing of new work. This unifies the behavior: Dispose() now will also clear work and prevent future work. The cancellation token that's accepted in the constructor is just a convenience for calling Dispose().
Also remove the passed CancellationToken.None when either we already were disposing the queue, or could trivially dispose the queue.
We can now just call Dispose() directly and simplify the code.
There's no reason to MEF import IThreadingContext to get a disposal token if we can just manage our own lifetime more directly.
We had a default and editor version of these services, the only difference being whether we get a disposal token or not. But the disposal token just came from IThreadingContext which is just set when the MEF container is disposed. We can take advantage of workspace service disposal to achieve the same effect.
We already had Dispose() calls in most of right places, so the cancellation token was redundant.
Simplifies some logic and ensures we're not queueing more work when we might be shutting down.
The token was never actually cancelled anywhere.
2ae71a9 to
1fda624
Compare
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
src/Dependencies/Threading/AsyncBatchingWorkQueue`2.cs:154
- AsyncBatchingWorkQueue.Dispose() no longer disposes the internal CancellationSeries, which leaves the current CTS held by CancellationSeries undisposed (and potentially retains registrations/handles) after the queue is shut down. If we dispose CancellationSeries, CancelExistingWork() should become a no-op after disposal so tests like CancelExistingWorkAfterDisposeDoesNotThrow remain valid.
public void Dispose()
{
lock (_gate)
{
// If we've previously disposed, we don't need to do anything further
if (_entireQueueCancellationTokenSource.IsCancellationRequested)
return;
// Cancel all work in the queue; this .Cancel() should stop the work, but we'll call CancelExistingWork() too to ensure
// we've cleared out all items that haven't ran
CancelExistingWork();
_entireQueueCancellationTokenSource.Cancel();
}
// This must be done outside of the lock: disposing a registration blocks if the registered callback is currently running.
// If we did this inside the lock, the callback might be blocked waiting for a call to Dispose() to release the lock, but the
// caller of Dispose() would be blocked on that registration. If we could drop netstandard support, we could just call Unregister() instead.
_externalCancellationTokenRegistration.Dispose();
}
src/Workspaces/Remote/ServiceHub/Services/ProcessTelemetry/RemoteProcessTelemetryService.PerformanceReporter.cs:45
- PerformanceReporter subscribes to IPerformanceTrackerService.SnapshotAdded with an inline lambda, but Dispose() only disposes the queue. This leaves the reporter rooted by the event subscription and continues invoking AddWork() after disposal. Store the handler in a field (or use a named method) and unsubscribe in Dispose().
- Files reviewed: 44/44 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
src/Workspaces/Remote/ServiceHub/Services/ProcessTelemetry/RemoteProcessTelemetryService.PerformanceReporter.cs:44
- PerformanceReporter subscribes to IPerformanceTrackerService.SnapshotAdded with an anonymous lambda, but Dispose() only disposes the work queue. That leaves the reporter rooted by the event subscription and will keep invoking AddWork on a disposed queue. Store the delegate and unsubscribe in Dispose().
src/EditorFeatures/Core/SolutionEvents/HostLegacySolutionEventsWorkspaceEventListener.cs:50 - Dispose() only disposes the batching queue. If StartListening() was called, the workspace change registration in _workspaceChangedDisposer remains active, which can leak this listener and enqueue work after disposal. Dispose() should also dispose and clear _workspaceChangedDisposer.
public void Dispose() => _eventQueue.Dispose();
- Files reviewed: 44/44 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (4)
src/VisualStudio/Core/Impl/CodeModel/ProjectCodeModelFactory.cs:66
- The workspace-changed handler registration is discarded, so it can't be disposed when this factory is disposed. That can keep ProjectCodeModelFactory rooted by the workspace and continue invoking OnWorkspaceChanged after _workspaceChangeEventsToFireEventsFor has been disposed (it will no-op, but still leaks/overhead). Store the returned WorkspaceEventRegistration and dispose it in Dispose().
_ = _visualStudioWorkspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged);
}
public void Dispose() => _workspaceChangeEventsToFireEventsFor.Dispose();
src/VisualStudio/Core/Impl/SolutionExplorer/SymbolTree/RootSymbolTreeItemSourceProvider.cs:151
- Dispose() only disposes _updateSourcesQueue, but this type also registers workspace/document-open handlers (via RegisterWorkspaceChangedHandler/RegisterDocumentOpenedHandler) using lambdas that are never unregistered. If this provider is disposed (e.g., MEF shutdown), those event handlers can keep the provider alive and keep enqueueing work on a disposed queue. Consider storing the WorkspaceEventRegistration values and disposing them in Dispose() (and then disposing the queue).
src/EditorFeatures/Core/SolutionEvents/HostLegacySolutionEventsWorkspaceEventListener.cs:50 - Dispose() disposes the batching queue but doesn't dispose the workspace event subscription (_workspaceChangedDisposer). If MEF disposes this part without StopListening being called first, the handler can keep this instance rooted and keep calling into _eventQueue after it's disposed. Dispose() should defensively dispose the registration as well.
public void Dispose() => _eventQueue.Dispose();
src/VisualStudio/Core/Def/DesignerAttribute/VisualStudioDesignerAttributeService.cs:94
- Dispose() doesn't dispose the workspace-changed registration (_workspaceChangedDisposer). If this MEF part is disposed without StopListening being called first, the handler can keep this instance rooted and continue enqueueing work on disposed queues. Dispose() should defensively dispose the registration as well.
public void Dispose()
{
_workQueue.Dispose();
_projectSystemNotificationQueue.Dispose();
}
- Files reviewed: 44/44 changed files
- Comments generated: 0 new
This shouldn't be strictly necessary, but ensures if anything changes the pattern is still clear.
68d28a1 to
47202fa
Compare
AsyncBatchingWorkQueue was disposable, and also took a CancellationToken which could be used to cancel the work in the queue and also prevent future work. Dispose however was a bit strange: it would cancel work, but not free it and wouldn't prevent queueing of new work. This unifies the behavior: Dispose() now will also clear work and prevent future work. The cancellation token that's accepted in the constructor is just a convenience for calling Dispose().
This behavior was what lead to #84622 -- there we realized we had a queue cancelling we didn't really expect, but since it didn't prevent future work being queued you could still get more work running.
The first commit in this PR fixes AsyncBatchingWorkQueue, and then the follow up commits migrate all the easy uses of AsyncBatchingWorkQueue to use Disposal when possible. We had cases where we just forgot to shut down the queue but could do so, and cases where we had a token just to shut this down that can be written more easily now.
Microsoft Reviewers: Open in CodeFlow