From 323f33042dc24f824fb57175a05f443c263a7b22 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 23 Feb 2018 10:52:35 +0000 Subject: [PATCH 1/6] Ensure consistent task ordering using ContinueWith Lock prevent tasks being executed at the same time but doesn't stop them being started out of order. --- .../Services/VSGitExt.cs | 33 ++++++++++--------- .../GitHub.TeamFoundation/VSGitExtTests.cs | 4 ++- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 6dd1d5fd12..81f86e3328 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -1,5 +1,6 @@ using System; using System.Linq; +using System.Threading.Tasks; using System.Collections.Generic; using System.ComponentModel.Composition; using GitHub.Models; @@ -8,7 +9,6 @@ using GitHub.TeamFoundation.Services; using Serilog; using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; -using Task = System.Threading.Tasks.Task; namespace GitHub.VisualStudio.Base { @@ -24,7 +24,6 @@ public class VSGitExt : IVSGitExt readonly IGitHubServiceProvider serviceProvider; readonly IVSUIContext context; readonly ILocalRepositoryModelFactory repositoryFactory; - readonly object refreshLock = new object(); IGitExt gitService; IReadOnlyList activeRepositories; @@ -41,23 +40,24 @@ 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. ActiveRepositories = Array.Empty(); + InitializeTask = Task.CompletedTask; + if (context.IsActive && TryInitialize()) { // Refresh ActiveRepositories on background thread so we don't delay startup. - InitializeTask = Task.Run(() => RefreshActiveRepositories()); + QueueRefreshActiveRepositories(); } else { // 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; } } @@ -68,7 +68,7 @@ void ContextChanged(object sender, VSUIContextChangedEventArgs e) if (e.Activated && TryInitialize()) { // Refresh ActiveRepositories on background thread so we don't delay UI context change. - InitializeTask = Task.Run(() => RefreshActiveRepositories()); + QueueRefreshActiveRepositories(); context.UIContextChanged -= ContextChanged; log.Debug("Initialized VSGitExt on UIContextChanged"); } @@ -83,7 +83,7 @@ bool TryInitialize() { if (e.PropertyName == nameof(gitService.ActiveRepositories)) { - RefreshActiveRepositories(); + QueueRefreshActiveRepositories(); } }; @@ -95,19 +95,22 @@ bool TryInitialize() return false; } + void QueueRefreshActiveRepositories() + { + // Execute tasks in sequence on thread pool. + InitializeTask = InitializeTask.ContinueWith(_ => RefreshActiveRepositories(), TaskScheduler.Default); + } + 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) { diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index 153d6295fc..bc6ebca7a2 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -63,7 +63,7 @@ public void ActiveRepositories_ReadUsingThreadPoolThread() public class TheActiveRepositoriesChangedEvent { [Test] - public void GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired() + public async Task GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired() { var context = CreateVSUIContext(true); var gitExt = CreateGitExt(); @@ -75,6 +75,7 @@ public void GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired() var eventArgs = new PropertyChangedEventArgs(nameof(gitExt.ActiveRepositories)); gitExt.PropertyChanged += Raise.Event(gitExt, eventArgs); + await target.InitializeTask; Assert.That(wasFired, Is.True); } @@ -188,6 +189,7 @@ public async Task ActiveRepositoriesChangedOrderingShouldBeCorrectAcrossThreads( var task2 = Task.Run(() => gitExt.ActiveRepositories = activeRepositories2); await Task.WhenAll(task1, task2); + await target.InitializeTask; Assert.That(target.ActiveRepositories.Single().LocalPath, Is.EqualTo("repo2")); } From 5200e72947a9fb68fe10d848f031afd3e498c5d6 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 23 Feb 2018 11:03:08 +0000 Subject: [PATCH 2/6] Rename InitializeTask to PendingTasks PendingTasks is used to execute tasks in order on the thread pool. --- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 13 +++++++------ .../GitHub.TeamFoundation/VSGitExtTests.cs | 14 +++++++------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 81f86e3328..d98e3edda5 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -43,11 +43,9 @@ public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory fact // 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(); - InitializeTask = Task.CompletedTask; - if (context.IsActive && TryInitialize()) { // Refresh ActiveRepositories on background thread so we don't delay startup. @@ -97,8 +95,8 @@ bool TryInitialize() void QueueRefreshActiveRepositories() { - // Execute tasks in sequence on thread pool. - InitializeTask = InitializeTask.ContinueWith(_ => RefreshActiveRepositories(), TaskScheduler.Default); + // Execute tasks in sequence using thread pool (TaskScheduler.Default). + PendingTasks = PendingTasks.ContinueWith(_ => RefreshActiveRepositories(), TaskScheduler.Default); } void RefreshActiveRepositories() @@ -139,6 +137,9 @@ private set public event Action ActiveRepositoriesChanged; - public Task InitializeTask { get; private set; } + /// + /// Tasks that are pending execution on the thread pool. + /// + public Task PendingTasks { get; private set; } = Task.CompletedTask; } } \ No newline at end of file diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index bc6ebca7a2..fb7c7f82ae 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -75,7 +75,7 @@ public async Task GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired() var eventArgs = new PropertyChangedEventArgs(nameof(gitExt.ActiveRepositories)); gitExt.PropertyChanged += Raise.Event(gitExt, eventArgs); - await target.InitializeTask; + await target.PendingTasks; Assert.That(wasFired, Is.True); } @@ -109,7 +109,7 @@ public void WhenUIContextChanged_ActiveRepositoriesChangedIsFired() var eventArgs = new VSUIContextChangedEventArgs(true); context.UIContextChanged += Raise.Event>(context, eventArgs); - target.InitializeTask.Wait(); + target.PendingTasks.Wait(); Assert.That(wasFired, Is.True); } @@ -126,7 +126,7 @@ public void WhenUIContextChanged_FiredUsingThreadPoolThread() var eventArgs = new VSUIContextChangedEventArgs(true); context.UIContextChanged += Raise.Event>(context, eventArgs); - target.InitializeTask.Wait(); + target.PendingTasks.Wait(); Assert.That(threadPool, Is.True); } @@ -151,7 +151,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; @@ -168,7 +168,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; @@ -189,7 +189,7 @@ public async Task ActiveRepositoriesChangedOrderingShouldBeCorrectAcrossThreads( var task2 = Task.Run(() => gitExt.ActiveRepositories = activeRepositories2); await Task.WhenAll(task1, task2); - await target.InitializeTask; + await target.PendingTasks; Assert.That(target.ActiveRepositories.Single().LocalPath, Is.EqualTo("repo2")); } @@ -221,7 +221,7 @@ static VSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = nul sp.GetService().Returns(factory); sp.GetService().Returns(gitExt); var vsGitExt = new VSGitExt(sp, factory, repoFactory); - vsGitExt.InitializeTask.Wait(); + vsGitExt.PendingTasks.Wait(); return vsGitExt; } From 68d098a8906eb26d204ecda6745b54f1da979e40 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 26 Feb 2018 12:12:53 +0000 Subject: [PATCH 3/6] Do initialization asynchronously on Main thread This will allow this service to be used by a package that is initializing on a background thread. --- .../Services/VSGitExt.cs | 68 ++++++++++++------- .../GitHub.TeamFoundation/VSGitExtTests.cs | 7 +- 2 files changed, 48 insertions(+), 27 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index d98e3edda5..835db8214d 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -6,6 +6,7 @@ using GitHub.Models; using GitHub.Services; using GitHub.Logging; +using GitHub.Helpers; using GitHub.TeamFoundation.Services; using Serilog; using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; @@ -46,34 +47,57 @@ public VSGitExt(IGitHubServiceProvider serviceProvider, IVSUIContextFactory fact // Start with empty array until we have a chance to initialize. ActiveRepositories = Array.Empty(); - if (context.IsActive && TryInitialize()) + PendingTasks = InitializeAsync(); + } + + async Task InitializeAsync() + { + try { - // Refresh ActiveRepositories on background thread so we don't delay startup. - QueueRefreshActiveRepositories(); + 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"); + 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) { - // Refresh ActiveRepositories on background thread so we don't delay UI context change. - QueueRefreshActiveRepositories(); - context.UIContextChanged -= ContextChanged; - log.Debug("Initialized VSGitExt on UIContextChanged"); + PendingTasks = ContextChangedAsync(); } } - bool TryInitialize() + 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) + { + log.Error(e, "UIContextChanged"); + } + } + + async Task TryInitialize() + { + // GetService must be called on the Main thread. + await ThreadingHelper.SwitchToMainThreadAsync(); + gitService = serviceProvider.GetService(); if (gitService != null) { @@ -81,10 +105,14 @@ bool TryInitialize() { if (e.PropertyName == nameof(gitService.ActiveRepositories)) { - QueueRefreshActiveRepositories(); + // 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; } @@ -93,12 +121,6 @@ bool TryInitialize() return false; } - void QueueRefreshActiveRepositories() - { - // Execute tasks in sequence using thread pool (TaskScheduler.Default). - PendingTasks = PendingTasks.ContinueWith(_ => RefreshActiveRepositories(), TaskScheduler.Default); - } - void RefreshActiveRepositories() { try @@ -140,6 +162,6 @@ private set /// /// Tasks that are pending execution on the thread pool. /// - public Task PendingTasks { get; private set; } = Task.CompletedTask; + public Task PendingTasks { get; private set; } } } \ No newline at end of file diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index fb7c7f82ae..7dfe6543c2 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/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,7 +59,7 @@ public void ActiveRepositories_ReadUsingThreadPoolThread() } } - public class TheActiveRepositoriesChangedEvent + public class TheActiveRepositoriesChangedEvent : TestBaseClass { [Test] public async Task GitExtPropertyChangedEvent_ActiveRepositoriesChangedIsFired() @@ -132,7 +131,7 @@ public void WhenUIContextChanged_FiredUsingThreadPoolThread() } } - public class TheActiveRepositoriesProperty + public class TheActiveRepositoriesProperty : TestBaseClass { [Test] public void SccProviderContextNotActive_IsEmpty() From dcd806fb80f7939dd173424c0f742d5964eb217b Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 26 Feb 2018 14:45:01 +0000 Subject: [PATCH 4/6] Add comment about using from background thread --- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 835db8214d..1d68f100f0 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -16,6 +16,10 @@ namespace GitHub.VisualStudio.Base /// /// This service acts as an always available version of . /// + /// + /// Initialization for this service will be done asynchronously and the service will be + /// retrieved on the Main thread. This means the service to be constructed and subscribed to on a background thread. + /// [Export(typeof(IVSGitExt))] [PartCreationPolicy(CreationPolicy.Shared)] public class VSGitExt : IVSGitExt From 2a3cef5db0a427940758a4075b3153fb8a25ca25 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 26 Feb 2018 15:12:00 +0000 Subject: [PATCH 5/6] Factor out GetServiceAsync --- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 1d68f100f0..4956c270f6 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -99,10 +99,7 @@ async Task ContextChangedAsync() async Task TryInitialize() { - // GetService must be called on the Main thread. - await ThreadingHelper.SwitchToMainThreadAsync(); - - gitService = serviceProvider.GetService(); + gitService = await GetServiceAsync(); if (gitService != null) { gitService.PropertyChanged += (s, e) => @@ -125,6 +122,13 @@ async Task TryInitialize() return false; } + async Task GetServiceAsync() where T : class + { + // GetService must be called from the Main thread. + await ThreadingHelper.SwitchToMainThreadAsync(); + return serviceProvider.GetService(); + } + void RefreshActiveRepositories() { try From 16aabcd600ba8b999a084c1a5ac9f2d73d71f246 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 27 Feb 2018 09:40:48 +0000 Subject: [PATCH 6/6] Make grammarful English --- src/GitHub.TeamFoundation.14/Services/VSGitExt.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 4956c270f6..19205a5e5f 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -18,7 +18,7 @@ namespace GitHub.VisualStudio.Base /// /// /// Initialization for this service will be done asynchronously and the service will be - /// retrieved on the Main thread. This means the service to be constructed and subscribed to on a background thread. + /// retrieved on the Main thread. This means the service can be constructed and subscribed to on a background thread. /// [Export(typeof(IVSGitExt))] [PartCreationPolicy(CreationPolicy.Shared)]