Merge pull request #20149 from jasonmalinowski/filechangetracker-improvements

Some performance and reliability fixes around file change tracking

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<uint> s_none = new Lazy<uint>(() => /* value doesn't matter*/ 42424242, LazyThreadSafetyMode.ExecutionAndPublication);
+        private static readonly Lazy<uint?> s_none = new Lazy<uint?>(() => null, LazyThreadSafetyMode.ExecutionAndPublication);
 
         private readonly IVsFileChangeEx _fileChangeService;
         private readonly string _filePath;
@@ -23,12 +23,30 @@ internal sealed class FileChangeTracker : IVsFileChangeEvents, IDisposable
 
         /// <summary>
         /// 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.
         /// </summary>
-        private Lazy<uint> _fileChangeCookie;
+        private Lazy<uint?> _fileChangeCookie;
 
         public event EventHandler UpdatedOnDisk;
 
+        /// <summary>
+        /// Operations on <see cref="IVsFileChangeEx"/> 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 <see cref="EnsureSubscription"/> will bypass that queue
+        /// and ensure it happens quickly.
+        /// </summary>
+        private static Task s_lastBackgroundTask = Task.CompletedTask;
+
+        /// <summary>
+        /// The object to use as a monitor guarding <see cref="s_lastBackgroundTask"/>. 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 <see cref="StartFileChangeListeningAsync"/> on the UI
+        /// thread, I don't expect to see contention.
+        /// </summary>
+        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<uint>(() =>
+            _fileChangeCookie = new Lazy<uint?>(() =>
+            {
+                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)
-            {
-                var hr = _fileChangeService.UnadviseFileChange(_fileChangeCookie.Value);
-
-                // 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))
+            if (_fileChangeCookie == s_none)
             {
-                    Marshal.ThrowExceptionForHR(hr);
+                return;
             }
 
+            var fileChangeCookie = _fileChangeCookie.Value;
             _fileChangeCookie = s_none;
+
+            // 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))
+                {
+                }
             }
         }
 

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);
         }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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);
+            }
+        }
     }
 }