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

Loading PR list blocks await JoinableTaskFactory.SwitchToMainThreadAsync #1537

Closed
jcansdale opened this Issue Mar 15, 2018 · 13 comments

Comments

Projects
None yet
4 participants
@jcansdale
Contributor

jcansdale commented Mar 15, 2018

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");
        }
    }
  1. Open Visual Studio 2015 with the GitHub pane visible and the PR list loading
  2. 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

@jcansdale jcansdale added the bug label Mar 15, 2018

@AArnott

This comment has been minimized.

Show comment
Hide comment
@AArnott

AArnott Mar 15, 2018

Collaborator

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.

Collaborator

AArnott commented Mar 15, 2018

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

This comment has been minimized.

Show comment
Hide comment
@AArnott

AArnott Mar 15, 2018

Collaborator

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?

Collaborator

AArnott commented Mar 15, 2018

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

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell Mar 15, 2018

@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.

sharwell commented Mar 15, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@AArnott

AArnott Mar 15, 2018

Collaborator

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.

Collaborator

AArnott commented Mar 15, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Mar 15, 2018

Contributor

@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.

Contributor

jcansdale commented Mar 15, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Mar 16, 2018

Contributor

@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?

Contributor

jcansdale commented Mar 16, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@AArnott

AArnott Mar 16, 2018

Collaborator

@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.

Collaborator

AArnott commented Mar 16, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell Mar 16, 2018

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.

sharwell commented Mar 16, 2018

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

This comment has been minimized.

Show comment
Hide comment
@AArnott

AArnott Mar 16, 2018

Collaborator

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.

Collaborator

AArnott commented Mar 16, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Mar 16, 2018

Contributor

@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:

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

Contributor

jcansdale commented Mar 16, 2018

@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:

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

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Mar 16, 2018

Contributor

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);
Contributor

jcansdale commented Mar 16, 2018

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

This comment has been minimized.

Show comment
Hide comment
@AArnott

AArnott Mar 16, 2018

Collaborator

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.

Collaborator

AArnott commented Mar 16, 2018

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.

@meaghanlewis meaghanlewis added this to Medium Priority in BUGS Apr 19, 2018

@jcansdale

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Aug 7, 2018

Contributor

This issue has been fixed twice. 😄

  1. #1737 makes the PR list complete loading much earlier

This will be good for other extension as well.

  1. We're doing the following when initializing menus/other UI:
    // 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.

Contributor

jcansdale commented Aug 7, 2018

This issue has been fixed twice. 😄

  1. #1737 makes the PR list complete loading much earlier

This will be good for other extension as well.

  1. We're doing the following when initializing menus/other UI:
    // 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.

@jcansdale jcansdale closed this Aug 7, 2018

BUGS automation moved this from Medium Priority to Done Aug 7, 2018

@meaghanlewis meaghanlewis added this to the 2.5.5 milestone Aug 7, 2018

@meaghanlewis meaghanlewis removed this from Done in BUGS Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment