Add analyzer to force ConfigureAwait(false) on all awaits

[JamesNK]

We don't add ConfigureAwait(false) to ASP.NET Core code because there is no sync context. Microsoft.Extensions.* code could execute in client apps. Not using ConfigureAwait(false) can create deadlocks when a user mixes sync with async, e.g. dotnet/extensions#999.

Consider introducing an analyzer to enforce ConfigureAwait(false) everywhere. A quick search of this repos source code displays many non-test awaits that don't follow best practice.

I just had some good success using https://www.nuget.org/packages/ConfigureAwaitChecker.Analyzer/

[rynowak]

don't follow best practice.

And like most "best practices" it's only correct like 90% of the time 😆

[JamesNK]

That's what pragma suppress is for.

[rynowak]

Or use an explicit .ConfigureAwait(true). I just wanted to make a pithy remark in general. "Best practice" usually hides some nuanced guidance that needs to be understood.

[JamesNK]

Ha. Best practice is code for I don't want to explain why we want it 😋

https://stackoverflow.com/a/27851460/11829

[analogrelay]

@ericstj Does dotnet/runtime already have an analyzer like this? I see some references to it. If so, this might be much easier for the components moving to dotnet/runtime.

[ericstj]

We enable CA2007 /cc @stephentoub

[stephentoub]

We enable CA2007

Yup.
https://github.com/dotnet/runtime/blob/a21da8f6d945002bbb7cdb426c148867f60be528/eng/CodeAnalysis.ruleset#L95

[sharwell]

CA2007 is outdated (among other things, doesn't support ValueTask). You'll want to use the vs-threading one instead, and disable CA2007 so you aren't getting duplicate diagnostics and fixes.

[stephentoub]

CA2007 is outdated (among other things, doesn't support ValueTask). You'll want to use the vs-threading one instead, and disable CA2007 so you aren't getting duplicate diagnostics and fixes.

The sheer noise from vs-threading analyzers was prohibitive.

If CA2007 is outdated, it should be fixed. Or if it's not being invested in because the vs-threading analyzers are being incorporated into these packages as a replacement, we'll wait for those before making any changes.

At present, CA2007 is what exists for us and our customers.

[analogrelay]

This is mostly done now. The dotnet/runtime repo already does this. We should turn on CA2007 in this repo as well though.

[javiercn]

@JamesNK is there anything for us to do here?

Should we close this? If not, do you want to bring it for someone to pick it up in the DoI?

[JamesNK]

I don't know what Andrew ment when he said this is mostly done. I don't think we've done anything 😬

Basically I think we should turn on CA2007 for all our libraries that can run outside of ASP.NET Core. We're writing code without ConfigureAwait(false) because we're assuming that there is never a sync context, but some of our libraries can be used in WinForms or WPF. That's created bugs (linked in initial comment).

We should come up with a rule to apply the analyzer setting across the repo. Perhaps all projects that are:

• Published as NuGet packages
• IsAspNetCoreApp is false

Note that there are some projects where the rule will be turned off. For example, the .NET SignalR client has a different strategy to avoid sync context issues (@BrennanConroy can you confirm, its been a couple of years since I looked at the .NET SignalR client code).

tldr; DoI should pick this up

[BrennanConroy]

I don't know what Andrew ment when he said this is mostly done.

I think Andrew meant we had added .ConfigureAwait(false) to most of the Extensions code.

For example, the .NET SignalR client has a different strategy to avoid sync context issues

Yeah, we have the weird ForceAsync method. It's very error prone though as you need to remember to add it to any new public top-level methods. We might want to discuss possibly changing that.