Merge branch 'fixes/1493-using-ContinueWith' into feature/show-current-pr

Author: jcansdale

src/GitHub.TeamFoundation.14/Services/VSGitExt.cs

@@ -1,20 +1,25 @@
 using System;
 using System.Linq;
+using System.Threading.Tasks;
 using System.Collections.Generic;
 using System.ComponentModel.Composition;
 using GitHub.Models;
 using GitHub.Services;
 using GitHub.Logging;
+using GitHub.Helpers;
 using GitHub.TeamFoundation.Services;
 using Serilog;
 using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility;
-using Task = System.Threading.Tasks.Task;
 
 namespace GitHub.VisualStudio.Base
 {
     /// <summary>
     /// This service acts as an always available version of <see cref="IGitExt"/>.
     /// </summary>
+    /// <remarks>
+    /// Initialization for this service will be done asynchronously and the <see cref="IGitExt" /> service will be
+    /// retrieved on the Main thread. This means the service can be constructed and subscribed to on a background thread.
+    /// </remarks>
     [Export(typeof(IVSGitExt))]
     [PartCreationPolicy(CreationPolicy.Shared)]
     public class VSGitExt : IVSGitExt
@@ -24,7 +29,6 @@ public class VSGitExt : IVSGitExt
         readonly IGitHubServiceProvider serviceProvider;
         readonly IVSUIContext context;
         readonly ILocalRepositoryModelFactory repositoryFactory;
-        readonly object refreshLock = new object();
 
         IGitExt gitService;
         IReadOnlyList<ILocalRepositoryModel> activeRepositories;
@@ -41,52 +45,75 @@ public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory fact
             this.repositoryFactory = repositoryFactory;
 
             // The IGitExt service isn't available when a TFS based solution is opened directly.
-            // It will become available when moving to a Git based solution (cause a UIContext event to fire).
+            // It will become available when moving to a Git based solution (and cause a UIContext event to fire).
             context = factory.GetUIContext(new Guid(Guids.GitSccProviderId));
 
-            // Start with empty array until we have a change to initialize.
+            // Start with empty array until we have a chance to initialize.
             ActiveRepositories = Array.Empty<ILocalRepositoryModel>();
 
-            if (context.IsActive && TryInitialize())
+            PendingTasks = InitializeAsync();
+        }
+
+        async Task InitializeAsync()
+        {
+            try
             {
-                // Refresh ActiveRepositories on background thread so we don't delay startup.
-                InitializeTask = Task.Run(() => RefreshActiveRepositories());
+                if (!context.IsActive || !await TryInitialize())
+                {
+                    // If we're not in the UIContext or TryInitialize fails, have another go when the UIContext changes.
+                    context.UIContextChanged += ContextChanged;
+                    log.Debug("VSGitExt will be initialized later");
+                }
             }
-            else
+            catch (Exception e)
             {
-                // If we're not in the UIContext or TryInitialize fails, have another go when the UIContext changes.
-                context.UIContextChanged += ContextChanged;
-                log.Debug("VSGitExt will be initialized later");
-                InitializeTask = Task.CompletedTask;
+                log.Error(e, "Initializing");
             }
         }
 
         void ContextChanged(object sender, VSUIContextChangedEventArgs e)
         {
-            // If we're in the UIContext and TryInitialize succeeds, we can stop listening for events.
-            // NOTE: this event can fire with UIContext=true in a TFS solution (not just Git).
-            if (e.Activated && TryInitialize())
+            if (e.Activated)
+            {
+                PendingTasks = ContextChangedAsync();
+            }
+        }
+
+        async Task ContextChangedAsync()
+        {
+            try
+            {
+                // If we're in the UIContext and TryInitialize succeeds, we can stop listening for events.
+                // NOTE: this event can fire with UIContext=true in a TFS solution (not just Git).
+                if (await TryInitialize())
+                {
+                    context.UIContextChanged -= ContextChanged;
+                    log.Debug("Initialized VSGitExt on UIContextChanged");
+                }
+            }
+            catch (Exception e)
             {
-                // Refresh ActiveRepositories on background thread so we don't delay UI context change.
-                InitializeTask = Task.Run(() => RefreshActiveRepositories());
-                context.UIContextChanged -= ContextChanged;
-                log.Debug("Initialized VSGitExt on UIContextChanged");
+                log.Error(e, "UIContextChanged");
             }
         }
 
-        bool TryInitialize()
+        async Task<bool> TryInitialize()
         {
-            gitService = serviceProvider.GetService<IGitExt>();
+            gitService = await GetServiceAsync<IGitExt>();
             if (gitService != null)
             {
                 gitService.PropertyChanged += (s, e) =>
                 {
                     if (e.PropertyName == nameof(gitService.ActiveRepositories))
                     {
-                        RefreshActiveRepositories();
+                        // Execute tasks in sequence using thread pool (TaskScheduler.Default).
+                        PendingTasks = PendingTasks.ContinueWith(_ => RefreshActiveRepositories(), TaskScheduler.Default);
                     }
                 };
 
+                // Do this after we start listening so we don't miss an event.
+                await Task.Run(() => RefreshActiveRepositories());
+
                 log.Debug("Found IGitExt service and initialized VSGitExt");
                 return true;
             }
@@ -95,19 +122,23 @@ bool TryInitialize()
             return false;
         }
 
+        async Task<T> GetServiceAsync<T>() where T : class
+        {
+            // GetService must be called from the Main thread.
+            await ThreadingHelper.SwitchToMainThreadAsync();
+            return serviceProvider.GetService<T>();
+        }
+
         void RefreshActiveRepositories()
         {
             try
             {
-                lock (refreshLock)
-                {
-                    log.Debug(
-                        "IGitExt.ActiveRepositories (#{Id}) returned {Repositories}",
-                        gitService.GetHashCode(),
-                        gitService?.ActiveRepositories.Select(x => x.RepositoryPath));
+                log.Debug(
+                    "IGitExt.ActiveRepositories (#{Id}) returned {Repositories}",
+                    gitService.GetHashCode(),
+                    gitService?.ActiveRepositories.Select(x => x.RepositoryPath));
 
-                    ActiveRepositories = gitService?.ActiveRepositories.Select(x => repositoryFactory.Create(x.RepositoryPath)).ToList();
-                }
+                ActiveRepositories = gitService?.ActiveRepositories.Select(x => repositoryFactory.Create(x.RepositoryPath)).ToList();
             }
             catch (Exception e)
             {
@@ -136,6 +167,9 @@ private set
 
         public event Action ActiveRepositoriesChanged;
 
-        public Task InitializeTask { get; private set; }
+        /// <summary>
+        /// Tasks that are pending execution on the thread pool.
+        /// </summary>
+        public Task PendingTasks { get; private set; }
     }
 }

test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs

@@ -11,11 +11,10 @@
 using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility;
 using System.Threading.Tasks;
 using System.Linq;
-using GitHub.TeamFoundation.Services;
 
 public class VSGitExtTests
 {
-    public class TheConstructor
+    public class TheConstructor : TestBaseClass
     {
         [TestCase(true, 1)]
         [TestCase(false, 0)]
@@ -60,10 +59,10 @@ public void ActiveRepositories_ReadUsingThreadPoolThread()
         }
     }
 
-    public class TheActiveRepositoriesChangedEvent
+    public class TheActiveRepositoriesChangedEvent : TestBaseClass
     {
         [Test]
-        public void GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired()
+        public async Task GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired()
         {
             var context = CreateVSUIContext(true);
             var gitExt = CreateGitExt();
@@ -75,6 +74,7 @@ public void GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired()
             var eventArgs = new PropertyChangedEventArgs(nameof(gitExt.ActiveRepositories));
             gitExt.PropertyChanged += Raise.Event<PropertyChangedEventHandler>(gitExt, eventArgs);
 
+            await target.PendingTasks;
             Assert.That(wasFired, Is.True);
         }
 
@@ -108,7 +108,7 @@ public void WhenUIContextChanged_ActiveRepositoriesChangedIsFired()
 
             var eventArgs = new VSUIContextChangedEventArgs(true);
             context.UIContextChanged += Raise.Event<EventHandler<VSUIContextChangedEventArgs>>(context, eventArgs);
-            target.InitializeTask.Wait();
+            target.PendingTasks.Wait();
 
             Assert.That(wasFired, Is.True);
         }
@@ -125,13 +125,13 @@ public void WhenUIContextChanged_FiredUsingThreadPoolThread()
 
             var eventArgs = new VSUIContextChangedEventArgs(true);
             context.UIContextChanged += Raise.Event<EventHandler<VSUIContextChangedEventArgs>>(context, eventArgs);
-            target.InitializeTask.Wait();
+            target.PendingTasks.Wait();
 
             Assert.That(threadPool, Is.True);
         }
     }
 
-    public class TheActiveRepositoriesProperty
+    public class TheActiveRepositoriesProperty : TestBaseClass
     {
         [Test]
         public void SccProviderContextNotActive_IsEmpty()
@@ -150,7 +150,7 @@ public void SccProviderContextIsActive_InitializeWithActiveRepositories()
             var context = CreateVSUIContext(true);
             var gitExt = CreateGitExt(new[] { repoPath });
             var target = CreateVSGitExt(context, gitExt, repoFactory: repoFactory);
-            target.InitializeTask.Wait();
+            target.PendingTasks.Wait();
 
             var activeRepositories = target.ActiveRepositories;
 
@@ -167,7 +167,7 @@ public void ExceptionRefreshingRepositories_ReturnsEmptyList()
             var context = CreateVSUIContext(true);
             var gitExt = CreateGitExt(new[] { repoPath });
             var target = CreateVSGitExt(context, gitExt, repoFactory: repoFactory);
-            target.InitializeTask.Wait();
+            target.PendingTasks.Wait();
 
             var activeRepositories = target.ActiveRepositories;
 
@@ -188,6 +188,7 @@ public async Task ActiveRepositoriesChangedOrderingShouldBeCorrectAcrossThreads(
             var task2 = Task.Run(() => gitExt.ActiveRepositories = activeRepositories2);
 
             await Task.WhenAll(task1, task2);
+            await target.PendingTasks;
 
             Assert.That(target.ActiveRepositories.Single().LocalPath, Is.EqualTo("repo2"));
         }
@@ -219,7 +220,7 @@ static VSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = nul
         sp.GetService<IVSUIContextFactory>().Returns(factory);
         sp.GetService<IGitExt>().Returns(gitExt);
         var vsGitExt = new VSGitExt(sp, factory, repoFactory);
-        vsGitExt.InitializeTask.Wait();
+        vsGitExt.PendingTasks.Wait();
         return vsGitExt;
     }