-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Remove UI thread use of UIContext #79347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
fc83290
de5a246
51f0d3d
9bfd110
455fbdc
0b01e4e
76f38e7
adc28b7
10db5b7
472cebc
8986e50
9a8d65c
c23a6bf
e4288e8
fbfcad3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,6 +100,7 @@ | |
| <_Dependency Remove="@(_Dependency)" Condition="$([MSBuild]::ValueOrDefault('%(Identity)', '').StartsWith('Microsoft.ServiceHub.'))"/> | ||
| <_Dependency Remove="@(_Dependency)" Condition="$([MSBuild]::ValueOrDefault('%(Identity)', '').StartsWith('System.Composition.'))"/> | ||
| <_Dependency Remove="@(_Dependency)" Condition="$([MSBuild]::ValueOrDefault('%(Identity)', '').StartsWith('Microsoft.Internal.VisualStudio.'))"/> | ||
| <_Dependency Remove="Azure.Core"/> | ||
| <_Dependency Remove="EnvDTE"/> | ||
| <_Dependency Remove="EnvDTE80"/> | ||
| <_Dependency Remove="EnvDTE90"/> | ||
|
|
@@ -128,6 +129,7 @@ | |
| <_Dependency Remove="stdole"/> | ||
| <_Dependency Remove="StreamJsonRpc"/> | ||
| <_Dependency Remove="System.Buffers" /> | ||
| <_Dependency Remove="System.ClientModel"/> | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The version we're pulling in predates the infamous source generator.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to pull it in at all?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WCF, wtf?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's pulling in through reference to Microsoft.Internal.VisualStudio.Shell.Framework. I have no idea why. And frankly, I'm not sure why we have this mechanism generally in DevDivInsertionFiles.csproj. The intent was so if you pull in a new dependency you don't forget to push it into VS if needed. But I'm not sure we've added a new one to this list in awhile. |
||
| <_Dependency Remove="System.Collections.Immutable"/> | ||
| <_Dependency Remove="System.Configuration.ConfigurationManager"/> | ||
| <_Dependency Remove="System.Diagnostics.DiagnosticSource"/> | ||
|
|
@@ -136,6 +138,7 @@ | |
| <_Dependency Remove="System.IO.Packaging"/> | ||
| <_Dependency Remove="System.IO.Pipelines"/> | ||
| <_Dependency Remove="System.Memory"/> | ||
| <_Dependency Remove="System.Memory.Data"/> | ||
| <_Dependency Remove="System.Numerics.Vectors"/> | ||
| <_Dependency Remove="System.Reflection.Metadata"/> | ||
| <_Dependency Remove="System.Reflection.MetadataLoadContext"/> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,17 +103,18 @@ internal abstract partial class VisualStudioWorkspaceImpl : VisualStudioWorkspac | |
| private readonly JoinableTaskCollection _updateUIContextJoinableTasks; | ||
|
|
||
| private readonly OpenFileTracker _openFileTracker; | ||
| private readonly UIContext _solutionClosingContext; | ||
|
|
||
| internal IFileChangeWatcher FileChangeWatcher { get; } | ||
|
|
||
| internal ProjectSystemProjectFactory ProjectSystemProjectFactory { get; } | ||
|
|
||
| private readonly Lazy<IProjectCodeModelFactory> _projectCodeModelFactory; | ||
| private readonly Lazy<ExternalErrorDiagnosticUpdateSource> _lazyExternalErrorDiagnosticUpdateSource; | ||
| private readonly IAsynchronousOperationListener _workspaceListener; | ||
| private bool _isExternalErrorDiagnosticUpdateSourceSubscribedToSolutionBuildEvents; | ||
|
|
||
| /// <summary> | ||
| /// Only read/written on hte UI thread. | ||
| /// Only read/written on the UI thread. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noooooooooooooooo |
||
| /// </summary> | ||
| private bool _isShowingDocumentChangeErrorInfoBar = false; | ||
| private bool _ignoreDocumentTextChangeErrors; | ||
|
|
@@ -138,18 +139,17 @@ public VisualStudioWorkspaceImpl(ExportProvider exportProvider, IAsyncServicePro | |
| ProjectSystemProjectFactory = new ProjectSystemProjectFactory( | ||
| this, FileChangeWatcher, CheckForAddedFileBeingOpenMaybeAsync, RemoveProjectFromMaps, _threadingContext.DisposalToken); | ||
|
|
||
| _solutionClosingContext = UIContext.FromUIContextGuid(VSConstants.UICONTEXT.SolutionClosing_guid); | ||
| _solutionClosingContext.UIContextChanged += SolutionClosingContext_UIContextChanged; | ||
|
|
||
| _openFileTracker = new OpenFileTracker( | ||
| this, | ||
| ProjectSystemProjectFactory, | ||
| exportProvider.GetExport<Microsoft.VisualStudio.Text.Editor.IEditorOptionsFactoryService>(), | ||
| _workspaceListener, | ||
| exportProvider.GetExportedValue<OpenTextBufferProvider>()); | ||
|
|
||
| InitializeUIAffinitizedServicesAsync().Forget(); | ||
|
|
||
| _lazyExternalErrorDiagnosticUpdateSource = new Lazy<ExternalErrorDiagnosticUpdateSource>(() => | ||
| exportProvider.GetExportedValue<ExternalErrorDiagnosticUpdateSource>(), | ||
| isThreadSafe: true); | ||
| _lazyExternalErrorDiagnosticUpdateSource = exportProvider.GetExport<ExternalErrorDiagnosticUpdateSource>(); | ||
|
|
||
| _updateUIContextJoinableTasks = new JoinableTaskCollection(_threadingContext.JoinableTaskContext); | ||
|
|
||
|
|
@@ -160,61 +160,17 @@ public VisualStudioWorkspaceImpl(ExportProvider exportProvider, IAsyncServicePro | |
|
|
||
| Logger.Log(FunctionId.Run_Environment, KeyValueLogMessage.Create( | ||
| static m => m["Version"] = FileVersionInfo.GetVersionInfo(typeof(VisualStudioWorkspace).Assembly.Location).FileVersion)); | ||
| } | ||
|
|
||
| internal ExternalErrorDiagnosticUpdateSource ExternalErrorDiagnosticUpdateSource => _lazyExternalErrorDiagnosticUpdateSource.Value; | ||
|
|
||
| internal void SubscribeExternalErrorDiagnosticUpdateSourceToSolutionBuildEvents() | ||
| { | ||
| // TODO: further understand if this needs the foreground thread for any reason. UIContexts are safe to read from the UI thread; | ||
| // it's not clear to me why this is being asserted. | ||
| _threadingContext.ThrowIfNotOnUIThread(); | ||
|
|
||
| if (_isExternalErrorDiagnosticUpdateSourceSubscribedToSolutionBuildEvents) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // TODO: https://github.com/dotnet/roslyn/issues/36065 | ||
| // UIContextImpl requires IVsMonitorSelection service: | ||
| if (ServiceProvider.GlobalProvider.GetService(typeof(IVsMonitorSelection)) == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // This pattern ensures that we are called whenever the build starts/completes even if it is already in progress. | ||
| KnownUIContexts.SolutionBuildingContext.WhenActivated(() => | ||
| { | ||
| KnownUIContexts.SolutionBuildingContext.UIContextChanged += (_, e) => | ||
| { | ||
| if (e.Activated) | ||
| { | ||
| ExternalErrorDiagnosticUpdateSource.OnSolutionBuildStarted(); | ||
| } | ||
| else | ||
| { | ||
| // A real build just finished. Clear out any results from the last "run code analysis" command. | ||
| this.Services.GetRequiredService<ICodeAnalysisDiagnosticAnalyzerService>().Clear(); | ||
| ExternalErrorDiagnosticUpdateSource.OnSolutionBuildCompleted(); | ||
| } | ||
| }; | ||
|
|
||
| ExternalErrorDiagnosticUpdateSource.OnSolutionBuildStarted(); | ||
| }); | ||
|
|
||
| _isExternalErrorDiagnosticUpdateSourceSubscribedToSolutionBuildEvents = true; | ||
| SubscribeToSourceGeneratorImpactingEvents(); | ||
| } | ||
|
|
||
| public async Task InitializeUIAffinitizedServicesAsync() | ||
| private void SolutionClosingContext_UIContextChanged(object sender, UIContextChangedEventArgs e) | ||
| { | ||
| // Yield the thread, so the caller can proceed and return immediately. | ||
| // Create services that are bound to the UI thread | ||
| await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(alwaysYield: true, _threadingContext.DisposalToken); | ||
|
|
||
| var solutionClosingContext = UIContext.FromUIContextGuid(VSConstants.UICONTEXT.SolutionClosing_guid); | ||
| solutionClosingContext.UIContextChanged += (_, e) => ProjectSystemProjectFactory.SolutionClosing = e.Activated; | ||
| ProjectSystemProjectFactory.SolutionClosing = e.Activated; | ||
| } | ||
|
|
||
| internal ExternalErrorDiagnosticUpdateSource ExternalErrorDiagnosticUpdateSource => _lazyExternalErrorDiagnosticUpdateSource.Value; | ||
|
|
||
| public Task CheckForAddedFileBeingOpenMaybeAsync(bool useAsync, ImmutableArray<string> newFileNames) | ||
| => _openFileTracker.CheckForAddedFileBeingOpenMaybeAsync(useAsync, newFileNames); | ||
|
|
||
|
|
@@ -1464,10 +1420,10 @@ protected override void Dispose(bool finalize) | |
| _textBufferFactoryService.TextBufferCreated -= AddTextBufferCloneServiceToBuffer; | ||
| _projectionBufferFactoryService.ProjectionBufferCreated -= AddTextBufferCloneServiceToBuffer; | ||
|
|
||
| if (_lazyExternalErrorDiagnosticUpdateSource.IsValueCreated) | ||
| { | ||
| _lazyExternalErrorDiagnosticUpdateSource.Value.Dispose(); | ||
| } | ||
| // UIContext.FromUIContextGuid internally has a map from the GUID to the context object itself that is stored in a static; | ||
| // if we don't unsubscribe, it will leak our workspace object which can cause memory leaks in tests that create a whole MEF container | ||
| // per test. | ||
| _solutionClosingContext?.UIContextChanged -= SolutionClosingContext_UIContextChanged; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure i entirely unnderstand why this is then ok. but i trust you.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CyrusNajmabadi What part is confusing?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole 'there is something static somewhere that is leaking, but this is actually an instance member that we're hooking up to'.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since I have to push to the PR, I'll clarify: we call UIContext.FromGuid() and internally that creates a static dictionary of UIContext GUID to -> UIContext object, so each test that runs will get the same object. |
||
| } | ||
|
|
||
| base.Dispose(finalize); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bump was needed because otherwise use of UIContext in a unit test would throw. This turns out to explain a behavior @jaredpar discovered where our subscription of stuff on a UI thread was throwing exceptions in unit tests, but since it was an async method that was fire-and-forget we never noticed.