Loading PR list blocks await JoinableTaskFactory.SwitchToMainThreadAsync

[jcansdale]

How to repro

1. Install the following repro
   AsyncPackageRepro.zip

    [Guid("aae76547-10c6-4a2d-b33a-76ded02ac07b")]
    [PackageRegistration(UseManagedResourcesOnly = true, AllowsBackgroundLoading = true)]
    [ProvideAutoLoad(Constants.vsContextNoSolution, PackageAutoLoadFlags.BackgroundLoad)]
    public sealed class MyAsyncPackage : AsyncPackage
    {
        protected override async Task InitializeAsync(
            CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
        {
            Trace.WriteLine("Executed when VS opens without a solution");
            await JoinableTaskFactory.SwitchToMainThreadAsync();
            Trace.WriteLine("Executed when PR list on GitHub pane has finished loading");
        }
    }

2. Open Visual Studio 2015 with the GitHub pane visible and the PR list loading
3. The await JoinableTaskFactory.SwitchToMainThreadAsync() line only returns once the PR list has completely finished refreshing (which can take a long time)

What is impacted

• This is a problem when installing the PR status bar UI (which must be done on the Main thread). The current PR sometimes don't appear for a long time.
• This will also be a problem for packages that auto-load in order to handle command visibility (e.g. GitHubPackage and InlineReviewsPackage).
• Any other extensions that use SwitchToMainThreadAsync when they auto-load

Notes

This hasn't been a problem in the past because we haven't been using the AllowsBackgroundLoading = true and PackageAutoLoadFlags.BackgroundLoad options. Developers are now being strongly encouraged to enable these options. We will potentially be delaying when other extensions wake up.

This isn't necessarily a problem when a command is executed because in this case InitializeAsync starts off on the Main thread and SwitchToMainThreadAsync is a nop. If however auto-load was started before the command is executed, it will likely still get blocked.

How to fix

Update: This appears to be the best solution:

protected override async Task InitializeAsync()
{
    // When SwitchToMainThreadAsync is called, use Normal priority (not Background)
    await JoinableTaskFactory.RunAsync(VsTaskRunContext.UIThreadNormalPriority, InitializeMenus);
}

async Task InitializeMenus()
{
    // This doesn't require the Main thread
    var menuService = (IMenuCommandService)(await GetServiceAsync(typeof(IMenuCommandService)));

    await JoinableTaskFactory.SwitchToMainThreadAsync();
    // This does require the Main thread
    menuService.AddCommands(...);
}

This can be resolved from inside InitializeAsync by using the following:
Unfortunately the following doesn't seem to work. SwitchToMainThreadAsync still doesn't return until after the GitHub pane has finished refreshing (this can take a while).

// `JoinableTaskFactory` is a property on `AsyncPackage`
await JoinableTaskFactory
   .WithPriority(VsTaskRunContext.UIThreadNormalPriority)
   .SwitchToMainThreadAsync();

I've tried the following as a workaround:

public static Task RunOnMainThreadNormalPriority(Action action)
{
    var service = (IVsTaskSchedulerService2)VsTaskLibraryHelper.ServiceInstance;
    var scheduler = service.GetTaskScheduler((uint)VsTaskRunContext.UIThreadNormalPriority);
    return Task.Factory.StartNew(action, default(CancellationToken),
        TaskCreationOptions.HideScheduler, scheduler);
}

This seems to work in simple cases, but if the executed code calls GetService it will deadlock (I think that's what's happening). We still need to find a solution.

We should also consider refreshing the PR list at a lower priority in order not to block other extensions.

Related

• Fix deadlock when VS is opended with GitHub pane visible: Fix deadlock when VS is opended with GitHub pane visible #1536
• Thanks @sharwell for his help on Gitter! https://gitter.im/Microsoft/extendvs?at=5aaa0ccd35dd17022e5f62a3

[AArnott]

Developers are now being strongly encouraged to enable these options. We will potentially be delaying when other extensions wake up.

Yes, that's actually the idea. Customers see a wide variance in startup performance and we're taking steps to try to ensure that startup and solution load happen as fast as possible since that tends to be what they care about most.

For a workaround, as I mention on gitter.im:

await this.JoinableTaskFactory.RunAsync(
    VsTaskRunContext.UIThreadNormalPriority,
    async delegate {
       await this.JoinableTaskFactory.SwitchToMainThreadAsync();
       // away you go
    });

Keep in mind though that you should use this sparingly, and do as little work as possible within such a pre-empting delegate as this.

[AArnott]

Unfortunately the following doesn't seem to work. SwitchToMainThreadAsync still doesn't return until after the GitHub pane has finished refreshing (this can take a while).

I'm not sure why the GitHub pane's refresh step would block a request to switch to the main thread unless that pane refresh was blocking the UI thread with its work. If that's the case, can it do some of this work either asynchronously or off the UI thread?

[sharwell]

@AArnott The GitHub extension uses Reactive-UI instead of vs-threading. It also doesn't appear to reset the UI scheduler priority (which defaults to DispatcherPriority.Normal), so until this is corrected the GitHub extension UI has ultimate priority when running in VS.

[AArnott]

Ya, anything with Normal priority will starve out not only background work but even user input as well.
That would be a good thing for github's use of Rx to change.

[jcansdale]

@AArnott @sharwell,

Thanks for all the info! We definitely want to fix this issue at source rather than use workarounds. Luckily it only seems to be a problem when VS opens with the GitHub pane active.

[jcansdale]

@AArnott this seems to be working well. 😄

I was wondering if there might be a simpler form of this if the code I'm executing isn't async?

For example, I'm currently doing this:

await JoinableTaskFactory.RunAsync(VsTaskRunContext.UIThreadNormalPriority, async () =>
{
    await JoinableTaskFactory.SwitchToMainThreadAsync();    
    menuService.AddCommands(....); // non-async code
});

The following doesn't compile, but I was wondering if something along these lines might?

await JoinableTaskFactory.Run(
    VsTaskRunContext.UIThreadNormalPriority,
    () => menuService.AddCommands(....));

Basically an equivalent of Task.Run(....), but running at UIThreadNormalPriority?

[AArnott]

@jcansdale What is your objection to using async? There's no way to avoid it. If you were already on the UI thread, you wouldn't have this issue to begin with. Switching to the UI thread is an async operation -- you shouldn't block the calling thread to do it.

[sharwell]

Switching to the UI thread is an async operation -- you shouldn't block the calling thread to do it.

📝 Keep in mind this isn't simply a point of "A is better than B because it's aligns with some mental coding model that I think is best". We regularly file internal bugs where blocking operations on a background thread (particularly thread pool threads) caused performance problems because they tried to synchronously invoke an operation on the UI thread.

[AArnott]

True. The real point though is that JoinableTaskFactory never switches to the UI thread at all, without an async delegate that includes await JTF.SwitchToMainThreadAsync(); in it. That's true even if you pass in a UI thread priority as a first argument. That first arg only influences what the UI priority will be when you later request a switch -- it doesn't do so implicitly.

[jcansdale]

@AArnott,

What is your objection to using async?

No particular objection to using async, it's just the methods that need to be the Main thread tend not to be async. It's often create a few commands using MEF and add them to IMenuCommandService. For example:

VisualStudio/src/GitHub.InlineReviews/InlineReviewsPackage.cs

Line 36 in 7d4c45f

menuService.AddCommands(

I was just curious if there was a simpler way to execute an action on the Main thread that avoids the risk of deadlock. 😉

[jcansdale]

That first arg only influences what the UI priority will be when you later request a switch -- it doesn't do so implicitly.

Ah, in that case the following seems to make sense. I wasn't 100% sure if RunAsync would change anything immediately:

await JoinableTaskFactory.RunAsync(VsTaskRunContext.UIThreadNormalPriority, InitializeMenus);

[AArnott]

Ya, it's an unfortunate artifact of the collision of APIs and extension methods. The JTF.RunAsync method you're calling is actually an extension method provided by VS to add VS-specific priorities. We had a choice of whether to automatically switch to the UI thread for the delegate you pass in, since the first arg would seem to suggest that might happen, or to hold to the built-in JTF.RunAsync methods which never switch contexts before invoking the delegate. There were pros/cons to each.
For better or worse, we went with the policy that all JTF.RunAsync overloads (including extension methods) should have the same behavior when invoking the delegate, which is to invoke it immediately on the calling thread.

[jcansdale]

This issue has been fixed twice. 😄

1. Speed up PR list and other improvements #1737 makes the PR list complete loading much earlier

This will be good for other extension as well.

2. We're doing the following when initializing menus/other UI:
   

   VisualStudio/src/GitHub.VisualStudio/GitHubPackage.cs

   Lines 46 to 48 in 49ad951

   	// Avoid delays when there is ongoing UI activity.
   	// See: https://github.com/github/VisualStudio/issues/1537
   	await JoinableTaskFactory.RunAsync(VsTaskRunContext.UIThreadNormalPriority, InitializeMenus);

This will let our UI appear even if another extension is hogging the UI thread.