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

Conversation

@jcansdale
Copy link
Collaborator

@jcansdale jcansdale commented Mar 14, 2018

As a workaround for potential delays before await ThreadingHelper.SwitchToMainThreadAsync() returns, we changed to using await ThreadingHelper.MainThreadDispatcher.InvokeAsync(() => ...). Unfortunately rather than simply delays, this workaround can cause deadlock.

This PR:

  • Revert back to using await ThreadingHelper.SwitchToMainThreadAsync()

These issues happen when Visual Studio is opened with the GitHub pane visible. It seems SwitchToMainThreadAsync doesn't return until the PR list has finished loading. 😨

We do need to find out what's causing this, but in a separate issue/PR. For the moment, let's get rid of the deadlock!

ThreadingHelper.MainThreadDispatcher.InvokeAsync was causing worse problems than ThreadingHelper.SwitchToMainThreadAsync.
Revert back to using ThreadingHelper.SwitchToMainThreadAsync.
@jcansdale jcansdale force-pushed the fixes/deadlock-SwitchToMainThreadAsync branch from 4d7a426 to 226c5ab Compare March 14, 2018 19:57
@shana
Copy link
Contributor

shana commented Mar 14, 2018

If it's deadlocking, that means that the await for the InvokeAsync is happening on the main thread, that's the only thing I can think of that would cause a deadlock. Maybe it's possible that the InitializeAsync method is called on the main thread in some conditions. Can we repro the deadlock reliably? If we can, then maybe only doing the InvokeAsync if we're not on the main thread can solve this.

@grokys grokys merged commit 6166b0d into master Mar 15, 2018
@grokys grokys deleted the fixes/deadlock-SwitchToMainThreadAsync branch March 15, 2018 09:47
@jcansdale
Copy link
Collaborator Author

@shana @grokys,

It seems we need to use the following:

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

I guess the default priority for extensions marked with AllowsBackgroundLoading = true must be UIThreadBackgroundPriority. This is too low for setting up UI elements like commands and the PR status bar element.

I'll follow up with another issue/PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants