API can deadlock when called from synchronous code. Should be using ConfigureAwait(false).

[jherby2k]

Details about Problem

The nuget v3 API uses async/await extensively, but doesn't seem to use ConfigureAwait(false) internally. This means synchronous APIs have to go out of their way to avoid deadlocks by calling your code on the threadpool, like this:

var publishedPackages = Task.Run(async () =>
{
    return await (await customRepository
            .GetResourceAsync<PackageSearchResource>(cancellationTokenSource.Token)
            .ConfigureAwait(false))
        .SearchAsync("SomePackage",
            new SearchFilter(true), 0, 100, NullLogger.Instance,
            cancellationTokenSource.Token)
        .ConfigureAwait(false);
 }).Result

It would be super cool if you could add some .ConfigureAwait(false) calls wherever await is used. I don't believe there are any drawbacks.

[NewellClark]

A quick search reveals that PackageSearchResourceV3 does not use ConfigureAwait(false) at all.
Edit: actually, it appears none of the usages of await in Nuget.Protocol/Resources use ConfigureAwait(false).

[zivkan]

On a personal note, I think all of NuGet's code under src\NuGet.Core (anything that ships as part of our "SDK", or in the dotnet cli) should use ConfigureAwait(false).

However, these assemblies also run in Visual Studio, which has its own "set of rules". In particular, it uses a special threading library that has something called a JoinableTaskFactory, which should be used (without ConfigureAwait(false), or with ConfigureAwait(true)), in order to avoid UI thread deadlocks. The vs-threading repo here on github has docs, with a specific section explicitly about whether or not to use ConfigureAwait(false): https://github.com/microsoft/vs-threading/blob/5810823fb821954cdb31e1e29db1aca0227f751d/doc/cookbook_vs.md#should-i-await-a-task-with-configureawaitfalse

After talking to the vs-threading experts within Microsoft a few months ago, the general recommendation we got was: code that never needs the UI thread can use ConfigureAwait(false) everywhere, otherwise don't use it at all, or use JoinableTaskFactory.

The problem is that the authentication flow in Visual Studio starts off with a button click or some other action in VS's UI, goes through various NuGet dlls, including NuGet.Protocol.dll, and if the HTTP response comes back with a HTTP unauthorized status, it calls AcquireCredentialsAsync, which in Visual Studio pops up a dialog, and hence needs the UI thread. As per the VS threading rules, this means the JoinableTaskFactory's SynchronizationContext needs to be passed through all awaits to avoid a deadlock, if the whole thing started on the UI thread in a synchronous method.

There's already an issue regarding changing NuGet to not prompt automatically for credentials. If this is implemented, then perhaps we'll be theoretically safe to use ConfigureAwait(false) in our src\NuGet.Core projects.

Alternatively perhaps someone could investigate if there's any other way to mitigate when a VS component has a synchronous method running on the UI thread, calls into NuGet, which causes an unauthorized HTTP response, which needs to show the credential login UI, if NuGet was to use ConfigureAwait(false). Without the confidence that this would actually work, we can't make the requested change at this time.

Customers, such as yourselves, who use NuGet's SDK in your own apps, can either start using Microsoft.VisualStudio.Threading and JoinableTaskFactory in your own apps (the name of the package indicates that it was created by the Visual Studio team, not that the package can only be used in Visual Studio). Or alternatively call NuGet's APIs with await Task.Run(...) to ensure it's on a background thread. For what it's worth, NuGet's Visual Studio component mostly ensures that we're running on a background thread before calling lower level libraries like NuGet.Protocol.

As I wrote in the first paragraph, I think it would be great for us to use ConfigureAwait(false), but I don't have time to test the credential scenario at this time. Because of the risk of UI deadlocks, if we make this change, I'd want to do it right after a Visual Studio update ships GA, so we have the maximum time in VS previews to find problems. While VS 16.8 shipped last week, because of the "holiday period", now is just not a good time to try risky changes, as there will be less testing with so many people on leave, and fewer people in the NuGet team to revert the change if it indeed causes problems. Maybe we'll look into this later, but now is just not a good time.