From 18e4175ac5a727d1f593daf4b5f81c6d55c5075f Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Tue, 26 Mar 2024 10:27:50 -0700 Subject: [PATCH 1/4] Don't get IVsRunningDocumentTable on background thread The `VisualStudioEditorDocumentManager` listens for running document table events and was retrieving the `IVsRunningDocumentTable` in its constructor. Because this is an `[ImportingConstructor]` it will almost certain be called on the thread pool and will implicitly marshal to the UI thread to perform a QI as part of retrieving the service. This can cause a hang if the UI thread is tied up with other work, such as when a solution is loaded. This change avoids retrieving the running doc table service until its needed and we are already running on the UI thread. --- .../Documents/VisualStudioEditorDocumentManager.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioEditorDocumentManager.cs b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioEditorDocumentManager.cs index bd57356da86..e6a064e56ae 100644 --- a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioEditorDocumentManager.cs +++ b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioEditorDocumentManager.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.ComponentModel.Composition; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Runtime.InteropServices; using Microsoft.AspNetCore.Razor; @@ -28,11 +29,12 @@ internal sealed class VisualStudioEditorDocumentManager( ProjectSnapshotManagerDispatcher dispatcher, JoinableTaskContext joinableTaskContext) : EditorDocumentManager(fileChangeTrackerFactory, dispatcher, joinableTaskContext) { - private readonly IVsRunningDocumentTable4 _runningDocumentTable = serviceProvider.GetService(throwOnFailure: true).AssumeNotNull(); + private readonly IServiceProvider _serviceProvider = serviceProvider; private readonly IVsEditorAdaptersFactoryService _editorAdaptersFactory = editorAdaptersFactory; private readonly Dictionary> _documentsByCookie = []; private readonly Dictionary _cookiesByDocument = []; + private IVsRunningDocumentTable4? _runningDocumentTable; private bool _advised; protected override ITextBuffer? GetTextBufferForOpenDocument(string filePath) @@ -219,10 +221,13 @@ public void DocumentRenamed(uint cookie, string fromFilePath, string toFilePath) } } + [MemberNotNull(nameof(_runningDocumentTable))] private void EnsureDocumentTableAdvised() { JoinableTaskContext.AssertUIThread(); + _runningDocumentTable ??= _serviceProvider.GetService(throwOnFailure: true).AssumeNotNull(); + if (!_advised) { _advised = true; From 9e109f88a7be2bdc6c3a936cb8f40db372e3d0de Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Tue, 26 Mar 2024 11:01:02 -0700 Subject: [PATCH 2/4] Don't get IVsAsyncFileChangeEx on background thread `VisualStudioFileChangeTrackerFactory` retrieves an `IVsAsyncFileChangeEx` service in its constructor. Because this is an `[ImportingConstructor]` it will likely be called on the thread pool and implicitly marshal to the UI thread to perform a QI as part of retrieving the service. This can cause a hang if the UI thread is tied up with other work, such as when a solution is loaded. This change avoids retrieving `IVsAsyncFileChangeEx` until its needed. This does block on retrieving the service. However, avoiding that will require a more substantial change that I'll follow up with on `main`. --- .../VisualStudioFileChangeTrackerFactory.cs | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioFileChangeTrackerFactory.cs b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioFileChangeTrackerFactory.cs index 50382c54f0c..2de9cbe2506 100644 --- a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioFileChangeTrackerFactory.cs +++ b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioFileChangeTrackerFactory.cs @@ -13,10 +13,10 @@ namespace Microsoft.VisualStudio.Editor.Razor.Documents; [Export(typeof(IFileChangeTrackerFactory))] internal class VisualStudioFileChangeTrackerFactory : IFileChangeTrackerFactory { - private readonly IErrorReporter _errorReporter; - private readonly IVsAsyncFileChangeEx _fileChangeService; - private readonly ProjectSnapshotManagerDispatcher _projectSnapshotManagerDispatcher; + private readonly ProjectSnapshotManagerDispatcher _dispatcher; private readonly JoinableTaskContext _joinableTaskContext; + private readonly IErrorReporter _errorReporter; + private readonly JoinableTask _getFileChangeServiceTask; [ImportingConstructor] public VisualStudioFileChangeTrackerFactory( @@ -25,10 +25,17 @@ public VisualStudioFileChangeTrackerFactory( ProjectSnapshotManagerDispatcher dispatcher, IErrorReporter errorReporter) { - _errorReporter = errorReporter; - _fileChangeService = (IVsAsyncFileChangeEx)serviceProvider.GetService(typeof(SVsFileChangeEx)); - _projectSnapshotManagerDispatcher = dispatcher; + _dispatcher = dispatcher; _joinableTaskContext = joinableTaskContext; + _errorReporter = errorReporter; + + var jtf = _joinableTaskContext.Factory; + _getFileChangeServiceTask = jtf.RunAsync(async () => + { + await jtf.SwitchToMainThreadAsync(); + + return (IVsAsyncFileChangeEx)serviceProvider.GetService(typeof(SVsFileChangeEx)); + }); } public IFileChangeTracker Create(string filePath) @@ -38,6 +45,9 @@ public IFileChangeTracker Create(string filePath) throw new ArgumentException(SR.ArgumentCannotBeNullOrEmpty, nameof(filePath)); } - return new VisualStudioFileChangeTracker(filePath, _errorReporter, _fileChangeService, _projectSnapshotManagerDispatcher, _joinableTaskContext); + // TODO: Make IFileChangeTrackerFactory.Create(...) asynchronous to avoid blocking here. + var fileChangeService = _getFileChangeServiceTask.Join(); + + return new VisualStudioFileChangeTracker(filePath, _errorReporter, fileChangeService, _dispatcher, _joinableTaskContext); } } From 956e9dbddd8a3ac4dcde5ca4813499e8f72395b6 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Tue, 26 Mar 2024 11:39:50 -0700 Subject: [PATCH 3/4] Add comment to describe why IVsRunningDocumentTable retrievel is deferred --- .../Documents/VisualStudioEditorDocumentManager.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioEditorDocumentManager.cs b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioEditorDocumentManager.cs index e6a064e56ae..e948736a9be 100644 --- a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioEditorDocumentManager.cs +++ b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioEditorDocumentManager.cs @@ -226,6 +226,8 @@ private void EnsureDocumentTableAdvised() { JoinableTaskContext.AssertUIThread(); + // Note: Because it is a COM interface, we defer retrieving IVsRunningDocumentTable until + // now to avoid implicitly marshalling to the UI thread, which can deadlock. _runningDocumentTable ??= _serviceProvider.GetService(throwOnFailure: true).AssumeNotNull(); if (!_advised) From 7a202966e1d09fb4fb586c9e71c4bb61563ce894 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Tue, 26 Mar 2024 11:48:56 -0700 Subject: [PATCH 4/4] Use IAsyncServiceProvider for simplicity --- .../Documents/VisualStudioFileChangeTrackerFactory.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioFileChangeTrackerFactory.cs b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioFileChangeTrackerFactory.cs index 2de9cbe2506..0793e7b6734 100644 --- a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioFileChangeTrackerFactory.cs +++ b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioFileChangeTrackerFactory.cs @@ -20,7 +20,7 @@ internal class VisualStudioFileChangeTrackerFactory : IFileChangeTrackerFactory [ImportingConstructor] public VisualStudioFileChangeTrackerFactory( - SVsServiceProvider serviceProvider, + [Import(typeof(SAsyncServiceProvider))] IAsyncServiceProvider serviceProvider, JoinableTaskContext joinableTaskContext, ProjectSnapshotManagerDispatcher dispatcher, IErrorReporter errorReporter) @@ -30,12 +30,7 @@ public VisualStudioFileChangeTrackerFactory( _errorReporter = errorReporter; var jtf = _joinableTaskContext.Factory; - _getFileChangeServiceTask = jtf.RunAsync(async () => - { - await jtf.SwitchToMainThreadAsync(); - - return (IVsAsyncFileChangeEx)serviceProvider.GetService(typeof(SVsFileChangeEx)); - }); + _getFileChangeServiceTask = jtf.RunAsync(serviceProvider.GetServiceAsync); } public IFileChangeTracker Create(string filePath)