Skip to content

Commit 123f4d5

Browse files
authored
*** DO NOT MERGE: Revert "Remove most remaining uses of WorkspaceChanged off the UI thread (dotnet#78778)" (dotnet#79366)
This reverts commit 67cce50, reversing changes made to aafd6eb. 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
2 parents a546c4b + 56e5de5 commit 123f4d5

File tree

7 files changed

+32
-14
lines changed

7 files changed

+32
-14
lines changed

src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.DocumentActiveContextChangedEventSource.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@ private sealed class DocumentActiveContextChangedEventSource(ITextBuffer subject
1313
{
1414
private WorkspaceEventRegistration? _documentActiveContextChangedDisposer;
1515

16+
// Require main thread on the callback as RaiseChanged implementors may have main thread dependencies.
1617
protected override void ConnectToWorkspace(Workspace workspace)
17-
=> _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged);
18+
=> _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
1819

1920
protected override void DisconnectFromWorkspace(Workspace workspace)
2021
=> _documentActiveContextChangedDisposer?.Dispose();

src/EditorFeatures/Core/Tagging/ITaggerEventSource.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ internal interface ITaggerEventSource
4141

4242
/// <summary>
4343
/// An event has happened on the thing the tagger is attached to. The tagger should
44-
/// recompute tags. May be raised on any thread.
44+
/// recompute tags.
4545
/// </summary>
4646
event EventHandler<TaggerEventArgs> Changed;
4747
}

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,9 @@ public CodeAnalysisDiagnosticAnalyzerService(
4949
_workspace = workspace;
5050
_diagnosticAnalyzerService = _workspace.Services.GetRequiredService<IDiagnosticAnalyzerService>();
5151

52-
_ = workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged);
52+
// Main thread as OnWorkspaceChanged's call to IDiagnosticAnalyzerService.RequestDiagnosticRefresh isn't clear on
53+
// threading requirements
54+
_ = workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
5355
}
5456

5557
private void OnWorkspaceChanged(WorkspaceChangeEventArgs e)

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,6 @@ internal interface IDiagnosticAnalyzerService : IWorkspaceService
2222
/// <summary>
2323
/// Re-analyze all projects and documents. This will cause an LSP diagnostic refresh request to be sent.
2424
/// </summary>
25-
/// <remarks>
26-
/// This implementation must be safe to call on any thread.
27-
/// </remarks>
2825
void RequestDiagnosticRefresh();
2926

3027
/// <summary>

src/LanguageServer/Protocol/Workspaces/LspWorkspaceRegistrationService.cs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ public virtual void Register(Workspace? workspace)
3939
m["WorkspacePartialSemanticsEnabled"] = workspace.PartialSemanticsEnabled;
4040
}, workspace));
4141

42-
var workspaceChangedDisposer = workspace.RegisterWorkspaceChangedHandler(OnLspWorkspaceChanged);
42+
// Forward workspace change events for all registered LSP workspaces. Requires main thread as it
43+
// fires LspSolutionChanged which hasn't been guaranteed to be thread safe.
44+
var workspaceChangedDisposer = workspace.RegisterWorkspaceChangedHandler(OnLspWorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
4345

4446
lock (_gate)
4547
{
@@ -92,7 +94,9 @@ public void Dispose()
9294
}
9395

9496
/// <summary>
95-
/// Indicates whether the LSP solution has changed in a non-tracked document context. May be raised on any thread.
97+
/// Indicates whether the LSP solution has changed in a non-tracked document context.
98+
///
99+
/// <b>IMPORTANT:</b> Implementations of this event handler should do as little synchronous work as possible since this will block.
96100
/// </summary>
97101
public EventHandler<WorkspaceChangeEventArgs>? LspSolutionChanged;
98102
}

src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerViewModel.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ public StackTraceExplorerViewModel(IThreadingContext threadingContext, Workspace
5050
_threadingContext = threadingContext;
5151
_workspace = workspace;
5252

53+
// Main thread dependency as Workspace_WorkspaceChanged modifies an ObservableCollection
54+
_ = workspace.RegisterWorkspaceChangedHandler(Workspace_WorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
55+
5356
_classificationTypeMap = classificationTypeMap;
5457
_formatMap = formatMap;
5558

@@ -100,6 +103,15 @@ private void CallstackLines_CollectionChanged(object sender, System.Collections.
100103
NotifyPropertyChanged(nameof(IsInstructionTextVisible));
101104
}
102105

106+
private void Workspace_WorkspaceChanged(WorkspaceChangeEventArgs e)
107+
{
108+
if (e.Kind == WorkspaceChangeKind.SolutionChanged)
109+
{
110+
Selection = null;
111+
Frames.Clear();
112+
}
113+
}
114+
103115
private FrameViewModel GetViewModel(ParsedFrame frame)
104116
=> frame switch
105117
{

src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/CpsDiagnosticItemSource.cs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ internal sealed partial class CpsDiagnosticItemSource : BaseDiagnosticAndGenerat
1919
{
2020
private readonly IVsHierarchyItem _item;
2121
private readonly string _projectDirectoryPath;
22-
private readonly string? _analyzerFilePath;
2322

2423
private WorkspaceEventRegistration? _workspaceChangedDisposer;
2524

@@ -38,8 +37,6 @@ public CpsDiagnosticItemSource(
3837
_item = item;
3938
_projectDirectoryPath = Path.GetDirectoryName(projectPath);
4039

41-
_analyzerFilePath = CpsUtilities.ExtractAnalyzerFilePath(_projectDirectoryPath, _item.CanonicalName);
42-
4340
this.AnalyzerReference = TryGetAnalyzerReference(Workspace.CurrentSolution);
4441
if (this.AnalyzerReference == null)
4542
{
@@ -50,7 +47,9 @@ public CpsDiagnosticItemSource(
5047
// then connect to it.
5148
if (workspace.CurrentSolution.ContainsProject(projectId))
5249
{
53-
_workspaceChangedDisposer = Workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChangedLookForAnalyzer);
50+
// Main thread dependency as OnWorkspaceChangedLookForAnalyzer accesses the IVsHierarchy
51+
// and fires the PropertyChanged event
52+
_workspaceChangedDisposer = Workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChangedLookForAnalyzer, WorkspaceEventOptions.RequiresMainThreadOptions);
5453
item.PropertyChanged += IVsHierarchyItem_PropertyChanged;
5554

5655
// Now that we've subscribed, check once more in case we missed the event
@@ -119,11 +118,14 @@ private void OnWorkspaceChangedLookForAnalyzer(WorkspaceChangeEventArgs e)
119118
return null;
120119
}
121120

122-
if (string.IsNullOrEmpty(_analyzerFilePath))
121+
var canonicalName = _item.CanonicalName;
122+
var analyzerFilePath = CpsUtilities.ExtractAnalyzerFilePath(_projectDirectoryPath, canonicalName);
123+
124+
if (string.IsNullOrEmpty(analyzerFilePath))
123125
{
124126
return null;
125127
}
126128

127-
return project.AnalyzerReferences.FirstOrDefault(r => string.Equals(r.FullPath, _analyzerFilePath, StringComparison.OrdinalIgnoreCase));
129+
return project.AnalyzerReferences.FirstOrDefault(r => string.Equals(r.FullPath, analyzerFilePath, StringComparison.OrdinalIgnoreCase));
128130
}
129131
}

0 commit comments

Comments
 (0)