diff --git a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/FileChangeTracker.cs b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/FileChangeTracker.cs index 19d8413d0a553f9c0175bbc4ad1a24261d7f804e..ef40a9fc74c1a3f85061f0e680dfd29a08c0172d 100644 --- a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/FileChangeTracker.cs +++ b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/FileChangeTracker.cs @@ -15,7 +15,7 @@ internal sealed class FileChangeTracker : IVsFileChangeEvents, IDisposable { private const uint FileChangeFlags = (uint)(_VSFILECHANGEFLAGS.VSFILECHG_Time | _VSFILECHANGEFLAGS.VSFILECHG_Add | _VSFILECHANGEFLAGS.VSFILECHG_Del | _VSFILECHANGEFLAGS.VSFILECHG_Size); - private static readonly Lazy s_none = new Lazy(() => /* value doesn't matter*/ 42424242, LazyThreadSafetyMode.ExecutionAndPublication); + private static readonly Lazy s_none = new Lazy(() => null, LazyThreadSafetyMode.ExecutionAndPublication); private readonly IVsFileChangeEx _fileChangeService; private readonly string _filePath; @@ -23,12 +23,30 @@ internal sealed class FileChangeTracker : IVsFileChangeEvents, IDisposable /// /// The cookie received from the IVsFileChangeEx interface that is watching for changes to - /// this file. + /// this file. This field may never be null, but might be a Lazy that has a value of null if + /// we either failed to subscribe over never have tried to subscribe. /// - private Lazy _fileChangeCookie; + private Lazy _fileChangeCookie; public event EventHandler UpdatedOnDisk; + /// + /// Operations on synchronize on a single lock within that service, so there's no point + /// in us trying to have multiple threads all trying to use it at the same time. When we queue a new background thread operation + /// we'll just do a continuation after the previous one. Any callers of will bypass that queue + /// and ensure it happens quickly. + /// + private static Task s_lastBackgroundTask = Task.CompletedTask; + + /// + /// The object to use as a monitor guarding . This lock is not strictly necessary, since we don't need + /// to ensure the background tasks happen entirely sequentially -- if we just removed the lock, and two subscriptions happened, we end up with + /// a 'branching' set of continuations, but that's fine since we're generally not running things in parallel. But it's easy to write, + /// and easy to delete if this lock has contention itself. Given we tend to call on the UI + /// thread, I don't expect to see contention. + /// + private static readonly object s_lastBackgroundTaskGate = new object(); + public FileChangeTracker(IVsFileChangeEx fileChangeService, string filePath) { _fileChangeService = fileChangeService; @@ -65,44 +83,81 @@ public void StartFileChangeListeningAsync() { if (_disposed) { - throw new ObjectDisposedException(typeof(FileChangeTracker).Name); + throw new ObjectDisposedException(nameof(FileChangeTracker)); } Contract.ThrowIfTrue(_fileChangeCookie != s_none); - _fileChangeCookie = new Lazy(() => + _fileChangeCookie = new Lazy(() => { - Marshal.ThrowExceptionForHR( - _fileChangeService.AdviseFileChange(_filePath, FileChangeFlags, this, out var newCookie)); - return newCookie; + try + { + Marshal.ThrowExceptionForHR( + _fileChangeService.AdviseFileChange(_filePath, FileChangeFlags, this, out var newCookie)); + return newCookie; + } + catch (Exception e) when (ShouldTrapException(e)) + { + return null; + } }, LazyThreadSafetyMode.ExecutionAndPublication); - // file change service is free-threaded. start running it in background right away - Task.Run(() => _fileChangeCookie.Value, CancellationToken.None); + lock (s_lastBackgroundTaskGate) + { + s_lastBackgroundTask = s_lastBackgroundTask.ContinueWith(_ => _fileChangeCookie.Value, CancellationToken.None, TaskContinuationOptions.None, TaskScheduler.Default); + } + } + + private static bool ShouldTrapException(Exception e) + { + if (e is FileNotFoundException) + { + // The IVsFileChange implementation shouldn't ever be throwing exceptions like this, but it's a + // transient file system issue (perhaps the file being deleted while we're changing subscriptions) + // and so there's nothing better to do. We'll still non-fatal to track the rate this is happening + return FatalError.ReportWithoutCrash(e); + } + else if (e is PathTooLongException) + { + // Nothing better we can do. We won't be able to open this file either, and thus we'll do our usual + // reporting of unopenable/missing files to the output window as usual. + return true; + } + else + { + return false; + } } public void StopFileChangeListening() { if (_disposed) { - throw new ObjectDisposedException(typeof(FileChangeTracker).Name); + throw new ObjectDisposedException(nameof(FileChangeTracker)); } // there is a slight chance that we haven't subscribed to the service yet so we subscribe and unsubscribe // both here unnecessarily. but I believe that probably is a theoretical problem and never happen in real life. // and even if that happens, it will be just a perf hit - if (_fileChangeCookie != s_none) + if (_fileChangeCookie == s_none) { - var hr = _fileChangeService.UnadviseFileChange(_fileChangeCookie.Value); + return; + } + + var fileChangeCookie = _fileChangeCookie.Value; + _fileChangeCookie = s_none; - // Verify if the file still exists before reporting the unadvise failure. - // This is a workaround for VSO #248774 - if (hr != VSConstants.S_OK && File.Exists(_filePath)) + // We may have tried to subscribe but failed, so have to check a second time + if (fileChangeCookie.HasValue) + { + try + { + Marshal.ThrowExceptionForHR( + _fileChangeService.UnadviseFileChange(fileChangeCookie.Value)); + } + catch (Exception e) when (ShouldTrapException(e)) { - Marshal.ThrowExceptionForHR(hr); } - - _fileChangeCookie = s_none; } } diff --git a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioAnalyzer.cs b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioAnalyzer.cs index 536f6bb852f564c18afc6752a087b033ec145ed3..29af6b55698ace7fb3f1610946fbf66d8bf2f9ee 100644 --- a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioAnalyzer.cs +++ b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioAnalyzer.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Reflection; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.VisualStudio.LanguageServices.Implementation.TaskList; @@ -31,7 +32,6 @@ public VisualStudioAnalyzer(string fullPath, IVsFileChangeEx fileChangeService, _tracker = new FileChangeTracker(fileChangeService, fullPath); _tracker.UpdatedOnDisk += OnUpdatedOnDisk; _tracker.StartFileChangeListeningAsync(); - _tracker.EnsureSubscription(); _hostDiagnosticUpdateSource = hostDiagnosticUpdateSource; _projectId = projectId; _workspace = workspace; @@ -55,7 +55,9 @@ public AnalyzerReference GetReference() { if (File.Exists(_fullPath)) { - _analyzerReference = new AnalyzerFileReference(_fullPath, _loader); + // Pass down a custom loader that will ensure we are watching for file changes once we actually load the assembly. + var assemblyLoaderForFileTracker = new AnalyzerAssemblyLoaderThatEnsuresFileBeingWatched(this); + _analyzerReference = new AnalyzerFileReference(_fullPath, assemblyLoaderForFileTracker); ((AnalyzerFileReference)_analyzerReference).AnalyzerLoadFailed += OnAnalyzerLoadError; } else @@ -108,5 +110,30 @@ private void OnUpdatedOnDisk(object sender, EventArgs e) { UpdatedOnDisk?.Invoke(this, EventArgs.Empty); } + + /// + /// This custom loader just wraps an existing loader, but ensures that we start listening to the file + /// for changes once we've actually looked at the file. + /// + private class AnalyzerAssemblyLoaderThatEnsuresFileBeingWatched : IAnalyzerAssemblyLoader + { + private readonly VisualStudioAnalyzer _analyzer; + + public AnalyzerAssemblyLoaderThatEnsuresFileBeingWatched(VisualStudioAnalyzer analyzer) + { + _analyzer = analyzer; + } + + public void AddDependencyLocation(string fullPath) + { + _analyzer._loader.AddDependencyLocation(fullPath); + } + + public Assembly LoadFromPath(string fullPath) + { + _analyzer._tracker.EnsureSubscription(); + return _analyzer._loader.LoadFromPath(fullPath); + } + } } }