Skip to content

/ VisualStudio

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

wants to merge 6 commits into from

Conversation

@jcansdale
Copy link
Collaborator

@jcansdale jcansdale commented Feb 23, 2018 •
edited

This PR allows the VSGitExt service to be constructed and subscribed to from background thread. This is necessary to allow the package from Show current PR on status bar #1396 to load on a background thread (package option AllowsBackgroundLoading = true).

Notes on using Task.ContinueWith to guarantee ordering of RefreshActiveRepositories

I wanted to understand what I could have done to keep the ordering of RefreshActiveRepositories consistent when I moved it off the main thread to the thread pool. In the VSGitExt constructor, it was changed from RefreshActiveRepositories() to Task.Run(() => RefreshActiveRepositories).

The body of RefreshActiveRepositories can be changed to use sync, but this doesn't guarantee the order in which tasks start executing on the thread pool. For example:

        async Task Lock()
        {
            var refreshLock = new object();
            var writer = new StringWriter();
            var tasks = new List<Task>();

            for (int x = 0; x < 10; x++)
            {
                var y = x;
                tasks.Add(Task.Run(() =>
                {
                    lock (refreshLock)
                    {
                        writer.Write(y + ", ");
                    }
                }));
            }

            await Task.WhenAll(tasks);
            Console.WriteLine(writer);
        }

Might output: 0, 3, 4, 5, 6, 7, 8, 9, 1, 2,

Interestingly the TaskCreationOptions.PreferFairness flag doesn't seem to make any difference when starting tasks:

        async Task PreferFairness()
        {
            var refreshLock = new object();
            var writer = new StringWriter();
            var tasks = new List<Task>();

            for (int x = 0; x < 10; x++)
            {
                var y = x;
                tasks.Add(Task.Factory.StartNew(() =>
                {
                    lock (refreshLock)
                    {
                        writer.Write(y + ", ");
                    }
                }, TaskCreationOptions.PreferFairness));
            }

            await Task.WhenAll(tasks);
            Console.WriteLine(writer);
        }

Which outputs: 1, 3, 4, 5, 6, 7, 8, 9, 2, 0,

An alternative approach is to use ContinueWith:

        async Task ContinueWith()
        {
            var writer = new StringWriter();
            var task = Task.CompletedTask;

            for (int x = 0; x < 10; x++)
            {
                var y = x;
                task = task.ContinueWith(_ => writer.Write(y + ", "), TaskScheduler.Default);
            }

            await task;
            Console.WriteLine(writer);
        }

Which consistently outputs: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9,

I prefer this approach because the intended ordering is explicit when the ContinueWith method is used.

What do you think?
jcansdale added 2 commits Feb 23, 2018
@jcansdale
Ensure consistent task ordering using ContinueWith
323f330 
Lock prevent tasks being executed at the same time but doesn't stop them being started out of order.
@jcansdale
Rename InitializeTask to PendingTasks
Loading status checks…
5200e72 
PendingTasks is used to execute tasks in order on the thread pool.
@jcansdale jcansdale requested a review from grokys Feb 23, 2018
@jcansdale jcansdale assigned shana and unassigned shana Feb 23, 2018
@jcansdale jcansdale requested a review from shana Feb 23, 2018
@jcansdale
Do initialization asynchronously on Main thread
Loading status checks…
68d098a 
This will allow this service to be used by a package that is initializing on a background thread.
@jcansdale jcansdale mentioned this pull request Feb 26, 2018
Merged
8 of 9 tasks complete
@jcansdale
Add comment about using from background thread
Loading status checks…
dcd806f
@jcansdale jcansdale changed the title Use Task.ContinueWith to guarantee ordering of RefreshActiveRepositories Allow VSGitExt to be constructed and subscribed to from background thread Feb 26, 2018
@jcansdale
Factor out GetServiceAsync
Loading status checks…
2a3cef5
@grokys
grokys reviewed Feb 26, 2018
View changes
src/GitHub.TeamFoundation.14/Services/VSGitExt.cs Outdated

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 to be constructed and subscribed to on a background thread.

This comment has been minimized.

Sign in to view
@grokys

grokys Feb 26, 2018
Contributor

"This means the service to be constructed and subscribed to" isn't very grammarful English ;)

This comment has been minimized.

Sign in to view
@jcansdale

jcansdale Feb 26, 2018
Author Collaborator

Ooops. I wish VS had an English grammar checker!
src/GitHub.TeamFoundation.14/Services/VSGitExt.cs
{
// Refresh ActiveRepositories on background thread so we don't delay startup.
InitializeTask = Task.Run(() => RefreshActiveRepositories());
if (!context.IsActive || !await TryInitialize())

This comment has been minimized.

Sign in to view
@grokys

grokys Feb 26, 2018
Contributor

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?

This comment has been minimized.

Sign in to view
@jcansdale

jcansdale Feb 26, 2018
Author Collaborator

This can be on a background or Main thread. We explicitly change to a background or Main thread when required.
src/GitHub.TeamFoundation.14/Services/VSGitExt.cs
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);

This comment has been minimized.

Sign in to view
@grokys

grokys Feb 26, 2018
Contributor

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

This comment has been minimized.

Sign in to view
@jcansdale

jcansdale Feb 26, 2018
Author Collaborator

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.
src/GitHub.TeamFoundation.14/Services/VSGitExt.cs
}
};

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

This comment has been minimized.

Sign in to view
@grokys

grokys Feb 26, 2018
Contributor

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?

This comment has been minimized.

Sign in to view
@jcansdale

jcansdale Feb 26, 2018
Author Collaborator

This line should be part of the initial PendingTasks (see PendingTasks = InitializeAsync() above). When PropertyChange events arrive, they should be executed after it.
@jcansdale
Make grammarful English
Loading status checks…
16aabcd
@shana
shana requested changes Feb 27, 2018
View changes
src/GitHub.TeamFoundation.14/Services/VSGitExt.cs
{
// GetService must be called from the Main thread.
await ThreadingHelper.SwitchToMainThreadAsync();
return serviceProvider.GetService<T>();

This comment has been minimized.

Sign in to view
@shana

shana Feb 27, 2018
Collaborator

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.

This comment has been minimized.

Sign in to view
@jcansdale

jcansdale Feb 27, 2018
Author Collaborator

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

This comment has been minimized.

Sign in to view
@jcansdale

jcansdale Feb 28, 2018
Author Collaborator

I've done as you've suggested in #1510. This PR is now obsolete.
@jcansdale
Copy link
Collaborator Author

@jcansdale jcansdale commented Feb 28, 2018

This PR has been merged into and extended by #1510. I'll close this one and continue development there.
@jcansdale jcansdale closed this Feb 28, 2018
@jcansdale jcansdale mentioned this pull request Feb 28, 2018
Merged
@jcansdale jcansdale deleted the fixes/1493-using-ContinueWith branch Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grokys grokys

@shana shana

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@jcansdale @shana @grokys
You can’t perform that action at this time.