Do ConfigureAwait(false)

[divega]

Creating an issue so that we can chat about this, and decide (again) whether we should be doing anything differently.

See aspnet/SignalR#545 for some context. This was found by @anpete while trying to understand what Npgsql is doing around it.

We came to what we have (practically not doing anything) through a long and painful analysis, but unfortunately it is very hard (for me at least) to remember or find exactly what we decided.

[anpete]

@davidfowl @roji It would be great to get your perspectives on this.

[roji]

For the Npgsql perspective... In general, libraries need run with a null synchronization context to allow their code to execute on the thread pool (and not, say, to be restricted to UI thread). Npgsql originally had ConfigureAwait(false) on every async single method invocation in the codebase; apart from being very cluttered, I had a more serious issue with ConfigureAwait(false).

There's actually a path inside Npgsql where code needs to execute on a special internal thread (to avoid a dependency on the threadpool, which sometimes creates deadlocks). In order to force this code to callback onto this internal thread, a special sync context is used (see SingleThreadSynchronizationContext.cs in case you're interested). This led to an odd situation: I need to be able to run the same code, sometimes with sync context null (the normal case) and sometimes with my own special sync context (the special case), but if you put ConfigureAwait(false) your sync context is always null. The solution I came up with was to set the sync context to null only at the user-facing public API methods, and then simply await down the stack without any need for ConfigureAwait(false). In effect, this removes the decision of which sync context is required from each individual method. Thanks to @anpete I just committed a new technique which is both safe and allocation-free, but you must take care to use it only from non-async top-level methods which wrap your async methods.

Note that the need to run the same methods with both sync context null and a specific sync context seems quite unique to Npgsql and I don't expect many other libraries to require this (I can elaborate on the Npgsql issue if you're interested). It does clean up lots of ConfigureAwait(false) in the codebase though :)

Let me know if you want any more info.

[ajcvickers]

This was discussed in triage with the following conclusions:

• EF calls into user code. This makes it more important the the context is preserved.
  • We could be selective and avoid the overhead on some low-level paths
• Deadlocks was not a major concern because they only happen when .Wait is used, and this is bad practice anyway
• Perf overhead was measured to be small
  • Perf overhead might be significant in tight loops, but these are a lot slower in async anyway, so it's not clear that it is worth doing anything here--but see idea above that we could change selected low-level paths
  • If async gets generally faster, then the relative perf gain could become more

Overall, we are not going to do anything immediately or generally on this, but based on other perf testing we could look at making targeted changes.

[davidfowl]

@ajcvickers are you currently doing ConfigureAwait everywhere in the code base?

[ajcvickers]

@davidfowl No, we don't call ConfigureAwait--that's what this discussion is about.

[davidfowl]

But it's closed now 😄 . Are we going to?

[bricelam]

@davidfowl No, we'll stick with our original decision to not call it. (reasoning above)

[davidfowl]

I'm not sure those points are all entirely reasonable. How did you test the performance differences? What sync context did you use?

Deadlocks was not a major concern because they only happen when .Wait is used, and this is bad practice anyway

That's not true. If there's a sync context that serializes execution of continuations (like ASP.NET does) and you have 2 tasks that are interdependent then it's possible to cause a deadlock without explicitly calling .Wait.

It's not always about performance, it might actually be about correctness.

[ajcvickers]

We held a new referendum on this issue. The outcome from this "People's Vote" is that we're going to do it. The sync context concerns are much less significant for .NET Core, and .NET Core is the future.

[roji]

@ajcvickers I can take this.

[ajcvickers]

Go for it!

[ajcvickers]

Removing milestone to discuss punting.

[ajcvickers]

Closing in favor of #10164