Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Allow VSGitExt to be constructed and subscribed to from background thread #1506

Closed
wants to merge 6 commits into from
Closed

Allow VSGitExt to be constructed and subscribed to from background thread #1506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
323f330
Ensure consistent task ordering using ContinueWith
jcansdale Feb 23, 2018
5200e72
Rename InitializeTask to PendingTasks
jcansdale Feb 23, 2018
68d098a
Do initialization asynchronously on Main thread
jcansdale Feb 26, 2018
dcd806f
Add comment about using from background thread
jcansdale Feb 26, 2018
2a3cef5
Factor out GetServiceAsync
jcansdale Feb 26, 2018
16aabcd
Make grammarful English
jcansdale Feb 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
96 changes: 65 additions & 31 deletions src/GitHub.TeamFoundation.14/Services/VSGitExt.cs
View file
Open in desktop
@@ -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
Expand All @@ -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;
Expand All @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should we be on a b/g thread for this? Or are we now saying that it can be run on the UI thread too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be on a background or Main thread. We explicitly change to a background or Main thread when required.

{
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there could still be a (very small chance of a) race here if PropertyChanged is called concurrently by 2 threads, I think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure the issue before was with RefreshActiveRepositories being called from the constructor and the PropertyChanged event at the same time.

It was called with Task.Run from the constructor here:
https://github.com/github/VisualStudio/blob/master/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs#L53

...and the PropertyChanged event here:
https://github.com/github/VisualStudio/blob/master/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs#L86

I don't think we need to worry about the PropertyChanged event being called concurrently by 2 threads.

}
};

// Do this after we start listening so we don't miss an event.
await Task.Run(() => RefreshActiveRepositories());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that PropertyChanged can be called on any thread, isn't there still a chance of a race here when team explorer is loaded because this isn't using PendingTasks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be part of the initial PendingTasks (see PendingTasks = InitializeAsync() above). When PropertyChange events arrive, they should be executed after it.


log.Debug("Found IGitExt service and initialized VSGitExt");
return true;
}
Expand All @@ -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>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way we're exposing MEF implementations as services is by having a MEF shim that redirects to the real instance instead, like we do for GitHubServiceProvider, UsageTracker (https://github.com/github/VisualStudio/blob/docs/clarify-tokens/src/GitHub.VisualStudio/Services/UsageTrackerDispatcher.cs and https://github.com/github/VisualStudio/blob/docs/clarify-tokens/src/GitHub.VisualStudio/Services/UsageTracker.cs) and registering the service in the ServiceProviderPackage. This is how this one should also probably be registered so we don't have to keep switching threads to get at it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do something similar in this PR. I can avoid switching threads by using IAsyncServiceProvider to retrieve IGitExt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done as you've suggested in #1510. This PR is now obsolete.

}

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)
{
Expand Down Expand Up @@ -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; }
}
}
21 changes: 11 additions & 10 deletions test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs
View file
Open in desktop
Expand Up @@ -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)]
Expand Down Expand Up @@ -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();
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
}
Expand All @@ -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()
Expand All @@ -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;

Expand All @@ -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;

Expand All @@ -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"));
}
Expand Down Expand Up @@ -219,7 +220,7 @@ static IReadOnlyList<IGitRepositoryInfo> CreateActiveRepositories(params string[
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;
}

Expand Down