From 5451e96e843c280ac96dcd44a6c73f3fde016465 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Mon, 14 Jul 2025 15:20:09 -0700 Subject: [PATCH] *** DO NOT MERGE: Revert "Remove most remaining uses of WorkspaceChanged off the UI thread (#78778)" (#79366) This reverts commit 67cce506abea8b9868c7da8067f8557753246d92, reversing changes made to aafd6ebe5e8d2ced7ed7b8c59158ee0103d4a93a. Going to run a test insertion to see if this is the cause of regressions flagged in https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/650773 --- ...rces.DocumentActiveContextChangedEventSource.cs | 3 ++- .../Core/Tagging/ITaggerEventSource.cs | 2 +- .../CodeAnalysisDiagnosticAnalyzerService.cs | 4 +++- .../Diagnostics/IDiagnosticAnalyzerService.cs | 3 --- .../Workspaces/LspWorkspaceRegistrationService.cs | 8 ++++++-- .../StackTraceExplorerViewModel.cs | 12 ++++++++++++ .../DiagnosticItem/CpsDiagnosticItemSource.cs | 14 ++++++++------ 7 files changed, 32 insertions(+), 14 deletions(-) diff --git a/src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.DocumentActiveContextChangedEventSource.cs b/src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.DocumentActiveContextChangedEventSource.cs index ce3dc9480a18e..ee02ce7ef6cf7 100644 --- a/src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.DocumentActiveContextChangedEventSource.cs +++ b/src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.DocumentActiveContextChangedEventSource.cs @@ -13,8 +13,9 @@ private sealed class DocumentActiveContextChangedEventSource(ITextBuffer subject { private WorkspaceEventRegistration? _documentActiveContextChangedDisposer; + // Require main thread on the callback as RaiseChanged implementors may have main thread dependencies. protected override void ConnectToWorkspace(Workspace workspace) - => _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged); + => _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged, WorkspaceEventOptions.RequiresMainThreadOptions); protected override void DisconnectFromWorkspace(Workspace workspace) => _documentActiveContextChangedDisposer?.Dispose(); diff --git a/src/EditorFeatures/Core/Tagging/ITaggerEventSource.cs b/src/EditorFeatures/Core/Tagging/ITaggerEventSource.cs index 5bc6e17c726c7..1a8e1f613febd 100644 --- a/src/EditorFeatures/Core/Tagging/ITaggerEventSource.cs +++ b/src/EditorFeatures/Core/Tagging/ITaggerEventSource.cs @@ -41,7 +41,7 @@ internal interface ITaggerEventSource /// /// An event has happened on the thing the tagger is attached to. The tagger should - /// recompute tags. May be raised on any thread. + /// recompute tags. /// event EventHandler Changed; } diff --git a/src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs b/src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs index 9fbca617af846..62ccb84a89d9c 100644 --- a/src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs +++ b/src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs @@ -49,7 +49,9 @@ public CodeAnalysisDiagnosticAnalyzerService( _workspace = workspace; _diagnosticAnalyzerService = _workspace.Services.GetRequiredService(); - _ = workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged); + // Main thread as OnWorkspaceChanged's call to IDiagnosticAnalyzerService.RequestDiagnosticRefresh isn't clear on + // threading requirements + _ = workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions); } private void OnWorkspaceChanged(WorkspaceChangeEventArgs e) diff --git a/src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs b/src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs index 52e6bb235e339..77381fe91a7d8 100644 --- a/src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs +++ b/src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs @@ -22,9 +22,6 @@ internal interface IDiagnosticAnalyzerService : IWorkspaceService /// /// Re-analyze all projects and documents. This will cause an LSP diagnostic refresh request to be sent. /// - /// - /// This implementation must be safe to call on any thread. - /// void RequestDiagnosticRefresh(); /// diff --git a/src/LanguageServer/Protocol/Workspaces/LspWorkspaceRegistrationService.cs b/src/LanguageServer/Protocol/Workspaces/LspWorkspaceRegistrationService.cs index 35f3406c1bbf1..1b7a230ddb6b6 100644 --- a/src/LanguageServer/Protocol/Workspaces/LspWorkspaceRegistrationService.cs +++ b/src/LanguageServer/Protocol/Workspaces/LspWorkspaceRegistrationService.cs @@ -39,7 +39,9 @@ public virtual void Register(Workspace? workspace) m["WorkspacePartialSemanticsEnabled"] = workspace.PartialSemanticsEnabled; }, workspace)); - var workspaceChangedDisposer = workspace.RegisterWorkspaceChangedHandler(OnLspWorkspaceChanged); + // Forward workspace change events for all registered LSP workspaces. Requires main thread as it + // fires LspSolutionChanged which hasn't been guaranteed to be thread safe. + var workspaceChangedDisposer = workspace.RegisterWorkspaceChangedHandler(OnLspWorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions); lock (_gate) { @@ -92,7 +94,9 @@ public void Dispose() } /// - /// Indicates whether the LSP solution has changed in a non-tracked document context. May be raised on any thread. + /// Indicates whether the LSP solution has changed in a non-tracked document context. + /// + /// IMPORTANT: Implementations of this event handler should do as little synchronous work as possible since this will block. /// public EventHandler? LspSolutionChanged; } diff --git a/src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerViewModel.cs b/src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerViewModel.cs index 1c4c8e6166489..3814232331dc5 100644 --- a/src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerViewModel.cs +++ b/src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerViewModel.cs @@ -50,6 +50,9 @@ public StackTraceExplorerViewModel(IThreadingContext threadingContext, Workspace _threadingContext = threadingContext; _workspace = workspace; + // Main thread dependency as Workspace_WorkspaceChanged modifies an ObservableCollection + _ = workspace.RegisterWorkspaceChangedHandler(Workspace_WorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions); + _classificationTypeMap = classificationTypeMap; _formatMap = formatMap; @@ -100,6 +103,15 @@ private void CallstackLines_CollectionChanged(object sender, System.Collections. NotifyPropertyChanged(nameof(IsInstructionTextVisible)); } + private void Workspace_WorkspaceChanged(WorkspaceChangeEventArgs e) + { + if (e.Kind == WorkspaceChangeKind.SolutionChanged) + { + Selection = null; + Frames.Clear(); + } + } + private FrameViewModel GetViewModel(ParsedFrame frame) => frame switch { diff --git a/src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/CpsDiagnosticItemSource.cs b/src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/CpsDiagnosticItemSource.cs index f5d797ba1dfb4..0525b9c7cc116 100644 --- a/src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/CpsDiagnosticItemSource.cs +++ b/src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/CpsDiagnosticItemSource.cs @@ -19,7 +19,6 @@ internal sealed partial class CpsDiagnosticItemSource : BaseDiagnosticAndGenerat { private readonly IVsHierarchyItem _item; private readonly string _projectDirectoryPath; - private readonly string? _analyzerFilePath; private WorkspaceEventRegistration? _workspaceChangedDisposer; @@ -38,8 +37,6 @@ public CpsDiagnosticItemSource( _item = item; _projectDirectoryPath = Path.GetDirectoryName(projectPath); - _analyzerFilePath = CpsUtilities.ExtractAnalyzerFilePath(_projectDirectoryPath, _item.CanonicalName); - this.AnalyzerReference = TryGetAnalyzerReference(Workspace.CurrentSolution); if (this.AnalyzerReference == null) { @@ -50,7 +47,9 @@ public CpsDiagnosticItemSource( // then connect to it. if (workspace.CurrentSolution.ContainsProject(projectId)) { - _workspaceChangedDisposer = Workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChangedLookForAnalyzer); + // Main thread dependency as OnWorkspaceChangedLookForAnalyzer accesses the IVsHierarchy + // and fires the PropertyChanged event + _workspaceChangedDisposer = Workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChangedLookForAnalyzer, WorkspaceEventOptions.RequiresMainThreadOptions); item.PropertyChanged += IVsHierarchyItem_PropertyChanged; // Now that we've subscribed, check once more in case we missed the event @@ -119,11 +118,14 @@ private void OnWorkspaceChangedLookForAnalyzer(WorkspaceChangeEventArgs e) return null; } - if (string.IsNullOrEmpty(_analyzerFilePath)) + var canonicalName = _item.CanonicalName; + var analyzerFilePath = CpsUtilities.ExtractAnalyzerFilePath(_projectDirectoryPath, canonicalName); + + if (string.IsNullOrEmpty(analyzerFilePath)) { return null; } - return project.AnalyzerReferences.FirstOrDefault(r => string.Equals(r.FullPath, _analyzerFilePath, StringComparison.OrdinalIgnoreCase)); + return project.AnalyzerReferences.FirstOrDefault(r => string.Equals(r.FullPath, analyzerFilePath, StringComparison.OrdinalIgnoreCase)); } }