dotnet / csharplang

The official repo for the design of the C# programming language
11.05k stars 1.01k forks source link

[Proposal] Forcing ConfigureAwait(false) on assembly level #2542

Open ashmind opened 8 years ago

ashmind commented 8 years ago

Problem

It's recommended to always use ConfigureAwait(false) in certain kinds of libraries. While it's definitely possible to use an analyzer (e.g. ConfigureAwaitChecker.Analyzer) to catch those cases, the analyzer has to be installed separately and the resulting code is awkward and verbose.

Potential solutions

Option A

Provide an assembly-level attribute that would force compiler to generate (pattern-based) ConfigureAwait(false) calls for each await.

Option B
  1. Implement CallerMethodAttribute from #351
  2. Add support for method info attributes to pattern-based GetAwaiter calls
  3. This would allow for a new overload GetAwaiter([CallerMethod] MethodBase method = null)
  4. Task could use this overload to look for some attribute on method.Assembly and return a correspondingly preconfigured awaiter.
alrz commented 8 years ago

@ashmind A quick question, does the following (from #114 comment) needs ConfigureAwait(false) on everyawait?

static async Task<TResult> UsingAsync<T, TResult>(this T disposable, Func<T, Task<TResult>> func)
    where T : IAsyncDisposable
{
    try { return await func(disposable); }
    finally { await disposable.DisposeAsync(); }
}

assuming that it is in a library.

ashmind commented 8 years ago

@alrz I don't see why not, but I can't say I understand all the edge cases perfectly. Let me know if I'm missing something here.

paulomorgado commented 8 years ago

Take this sample code:

void Main()
{
    SynchronizationContext.SetSynchronizationContext(new WindowsFormsSynchronizationContext());
    Console.WriteLine(SynchronizationContext.Current?.GetType().Name);
    M().Wait();
    Console.WriteLine(SynchronizationContext.Current?.GetType().Name);
}

async Task M()
{
    Console.WriteLine(SynchronizationContext.Current?.GetType().Name);
    await Task.CompletedTask.ConfigureAwait(false);
    Console.WriteLine(SynchronizationContext.Current?.GetType().Name);
    await Task.Delay(10).ConfigureAwait(false);
    Console.WriteLine(SynchronizationContext.Current?.GetType().Name);
}

You'll find that the output is this:

WindowsFormsSynchronizationContext
WindowsFormsSynchronizationContext
WindowsFormsSynchronizationContext
null
WindowsFormsSynchronizationContext

ConfigureAwait(false) has no effect on completed tasks.

If a task is already completed, then the execution continues on the same thread (that might be a thread with a synchronization context). If you don't use ConfigureAwait(false) on subsequent calls, their continuation will be posted to the captured synchronization context.

So, to be effective, if you use ConfigureAwait(false) in one method, you'll have to use it on all awaits.

HaloFour commented 8 years ago

I'd say that this is a CoreFX issue. ConfigureAwait is nothing more than an instance method on Task/Task<T>, the language provides no specific support for calling it.

ashmind commented 8 years ago

@HaloFour But can it be implemented in CoreFX without option B support by compiler? How would you know which assembly you are currently awaiting in?

HaloFour commented 8 years ago

Considering how loose the awaiter spec allows for resolving the GetAwaiter method I'm kind of surprised that it doesn't allow for the method to contain optional parameters today. I'd be game for allowing it, and of course I'm all for the expansion of the caller info attributes.

vermorel commented 8 years ago

+1 It's indeed a pain to have ConfigureAwait(false) everywhere in practice for all libraries.

NickCraver commented 7 years ago

I'd love to see a [assembly:TaskConfigureAwait(false)] added to the framework. People are working around this (but not in netstandard) with Fody today: https://www.nuget.org/packages/ConfigureAwait.Fody

async/await is great, but there are a few very big and very consistent pain points for library authors and app owners alike. At the top of my list are:

We've had async for over 4 years now and neither of these has improved at all. Any language feature which requires repeating something verbose in almost every place is a bad experience. On top of that, forgetting to add the verbose thing (and many new to async do) defaulting to failure (overhead in this case) adds to the priority, IMO. Please, let's give library authors a break here.

...or we can make .ConfigureAwait(false) a VS 2017 built-in keyboard shortcut.

petertiedemann commented 7 years ago

Just want to confirm that this is a major pain for library authors. Installing analyzers into hundreds of projects and then having to set them all to Error severity just to ensure we add this ugly ConfigureAwait call is pretty de-motivating.

But another alternative could be to introduce another await-style keyword as syntactic sugar for await with ConfigureAwait ( false ). Not sure what a good name would be though.

ufcpp commented 7 years ago

How do you guys think of an approach with Task-like (Generalized Async Return Types): https://github.com/ufcpp/ContextFreeTask

This requires no IL-weaving or no new compiler feature.

lanwin commented 7 years ago

@ufcpp pretty nice solution.

Why they didnt added something like that to the TPL?

asbjornu commented 7 years ago

I too find the current default maddening. What percentage does sync-requiring GUI code represent of all C# code being written today, anyway? Sigh.

alrz commented 7 years ago

..or we can make .ConfigureAwait(false) a VS 2017 built-in keyboard shortcut.

That's actually possible. We could add ConfigureAwait(false); to the completion list off the task.

So that would be C Enter.

AgentFire commented 6 years ago

That's actually possible. We could add ConfigureAwait(false); to the completion list off the task.

That'd still produce hude code redundancy. Personally, I never use the same any line of code more than 2 times per solution. Code repeatedness makes my eyes bleed.

Looking forward to ThreadPool.MakeDefaultConfigureAwait(false); with a desperate hope in my eyes.

HaloFour commented 6 years ago

@AgentFire

Proposals for BCL additions can be made on the CoreFX repository as any such helper method won't require language changes. You'd have to describe exactly what you expect it to do.

AgentFire commented 6 years ago

@HaloFour

here's no reliable way I see that this could be implemented purely in library; it would need to be based in language support. As such, I'm going to close this out. Thanks for the interest.

Meh

HaloFour commented 6 years ago

@AgentFire

That's a proposal for an attribute, not a helper method. The failing of trying to manage that via an attribute is that it's far from obvious how the behavior is applied and when. A helper method that is invoked imperatively would not have that same issue.

dominikjeske commented 5 years ago

It would be great to have this - my code is polluted with lot of ConfigureAwait. Pleas do something with this – it should not be a big thing.

enihcam commented 5 years ago

Or maybe we can add a function-level attribute? For example:

[ConfigureAwait(false)]
public async Task FooBar() {...}
dominikjeske commented 5 years ago

It should be for method, class and assembly level ;)

wachulski commented 5 years ago

@ashmind What about:

Option C

Incorporate ConfigureAwaitChecker.Analyzer (counterpart) into native Roslyn analysers set with 2 diagnostics (and fixers):

As for per solution/project configuration means new .editorconfig options might be introduced:

ashmind commented 5 years ago

@wachulski This is very practical, but I feel we should have more ambition and aim for cleaner code. It's like if instead of await we got an analyzer that lets us write ContinueWith() better.

sharwell commented 5 years ago

Over the past year, vs-threading has reversed my view on this issue. I am very much looking forward to no longer needing to specify ConfigureAwait outside of rare edge cases where ConfigureAwait(false) is required for correctness.

petertiedemann commented 5 years ago

@sharwell Can you enlighten us on how vs-threading resolves this (it seems to be focused on applications, and i am doing libraries that can be used in any context)?

sharwell commented 5 years ago

@petertiedemann A good starting place is this recent document: https://github.com/Microsoft/vs-threading/blob/master/doc/library_with_jtf.md

The basic idea is a library that avoids the use of ConfigureAwait will execute code on the synchronization context(s) that appear at the entry points to the library API. If those entry points occur on resource-limited contexts (e.g. the single main thread), the caller is expected to use a deadlock mitigation strategy. If the library further uses JoinableTaskContext internally (as opposed to just the implicit dependency from using ConfigureAwait(true)), the caller is expected to use vs-threading as the mitigation strategy.

effyteva commented 5 years ago

Almost 3 years since the proposal, Perhaps it's time to take care of this? Assembly/Class/Method attributes could save tons of hassle. There's a Fody addin for that https://github.com/Fody/ConfigureAwait, which we haven't tried, as we prefer a native solution, as modifying the IL doesn't seem like a reliable solution for us.

CyrusNajmabadi commented 5 years ago

we prefer a native solution, as modifying the IL doesn't seem like a reliable solution for us.

If there's a working solution... why not use it? :) Is there actually something unreliable here?

tmat commented 5 years ago

@CyrusNajmabadi I have yet to see an IL rewriter that correctly preserves debugging information. Also EnC won't work if you IL rewrite compiler outputs. I generally do not recommend using IL rewriters.

CyrusNajmabadi commented 5 years ago

It's seems like a particularly onerous restriction to eliminate this entire class of tools. It basically means that all of that functionality must always be built into the compiler. That seems unfortunate.

tmat commented 5 years ago

Generators would address that.

davidroth commented 5 years ago

Agree with @tmat: IL rewriters come with significant downsides:

alexk8 commented 4 years ago

Please assign a milestone, this feature is awaited.ConfigureAwait(false) by all c# devs :) Now we have await on almost each line of code. Do we still need to write ConfigureAwait(false) or use IL-rewriters mandatory?

yaakov-h commented 4 years ago

@alexk8 as a whole, solving the ConfigureAwait problem is earmarked for some future release, be it C# 9.0 or 10.0 or 11.0 etc. [source]

This particular solution, not so much.

heku commented 4 years ago

Or maybe can you introduce a compiler flag to change the default value?

e.g. in .csproj

<TaskConfigureAwaitDefaultValue>False</TaskConfigureAwaitDefaultValue>

Then C# compiler compiles await task as await task.ConfigureAwait(false)

theunrepentantgeek commented 4 years ago

@heku The Language Team’s stance on introduction of language dialects is unlikely to change, especially for a flag like this that could make two versions of what is otherwise an identical assembly incompatible.

pablocar80 commented 4 years ago

@theunrepentantgeek what if instead of an assembly-level flag, we add a class-level attribute? The attribute would make all methods of the class automatically wrapped inside ConfigureAwait(false) for all callers of the class.

theunrepentantgeek commented 4 years ago

Check out the other issues that discuss trying to eliminate ConfigureAwait() calls - it's been extensively discussed.

Essentially, however, my understanding boils down to this:

The language has no knowledge of implementation details. The async and await keywords only require that the types involved are awaitable.

The most commonly used implementation (by far) is the Task Parallel Library (TPL), It's the TPL that introduces the Task and ValueTask types, along with the concept of a synchronization context.

Crucially, nothing in the language requires you to use the TPL implementation.

Adding a language feature to address this library issue seems (to me) like the wrong approach. It would irrevocably bind the feature to a very specific implementation, and very likely tie things up so tightly that any future innovation in the area would be impossible.

It would be far better to work out how to move the design of the TPL forward in a useful way that eliminates the ConfigureAwait() warts while still solving the problems that Synchronization Contexts were introduced to solve.

But, that's a discussion for a different repo.

blankensteiner commented 4 years ago

It's just mind-blowing and very sad that this is still an issue in 2020. Microsoft, I don't want to be a jerk, but I truly believe in being honest and direct and I have to say that in my opinion you have drop the ball on this one and it's just not good enough. You want feedback, you want to be open source, you want a community, well then don't ignore a major plain like this one for years. I get that it's hard to solve and maybe it's falling between two chairs and so on, but it has been years now! Please, solve this. If I still have to ConfigureAwait(false) things when .NET 5 comes out, you'll have to explain to my kids why my brains are on the ceiling. No, seriously though, I'm happy with my choice to be a .NET developer and the general direction of .NET Core and .NET 5, but you need to fix this and you need to fix it now... And no, ConfigureAwait.Fody is not an option, it doesn't calculate a new checksum and thereby making it impossible to upload the snupkg to NuGet. Hoping for a status update and an ETA.

HaloFour commented 4 years ago

@blankensteiner

Hoping for a status update and an ETA.

A proposal without a champion on the language design team has no status or ETA. This project is opensource and as a result you are free to fork it and do anything you want, but being opensource doesn't mean that the team has to implement any proposed feature requests. Thus far this is deemed to not be a language issue. C#, as a language, has no concept of synchronization contexts. All proposals here want to tie await to the TPL, which is undesirable.

blankensteiner commented 4 years ago

@HaloFour

This project is opensource and as a result you are free to fork it and do anything you want, but being opensource doesn't mean that the team has to implement any proposed feature requests

Right, so, a good solution to an entire community begging for a solution is for everyone to just fork our common platform and do what we like?

I simply don't believe there are people out there that think ConfigureAwait(false)'ing every await is the right solution. I can't say which team within Microsoft should solve this, nor can I say which solution is the best, but I do see a major pain going unanswered for years and years. Surely, we can all agree that someone has to do something and that someone is within Microsoft.

HaloFour commented 4 years ago

@blankensteiner

I simply don't believe there are people out there that think ConfigureAwait(false)'ing every await is the right solution.

The TPL optimized for library consumers, where synchronization is the more commonly necessary. If you're putting ConfigureAwait(false) you're either doing it wrong, or you are only writing libraries and represent a minority of the development community. The majority of developers don't need to do anything.

Either way, C# doesn't know that ConfigureAwait even exists. The "awaiter" pattern is completely synchronization agnostic. IMO, this is better solved via APIs in or on top of TPL.

blankensteiner commented 4 years ago

@HaloFour

The majority of developers don't need to do anything.

Having worked with software development 12 years now, in multiple companies, having had hundreds of developer colleagues, doing all kinds of application types, I can't think of anyone who hasn't written to write a library at some point. The number of times I had to help people debug something and found the problem to be missing ConfigureAwait(false) can't be counted using my fingers alone.

Either way, C# doesn't know that ConfigureAwait even exists. The "awaiter" pattern is completely synchronization agnostic. IMO, this is better solved via APIs in or on top of TPL.

I'm sure you are right. Which repo/Microsoft-team do I have to contact? Even better, do you know any specific people?

CyrusNajmabadi commented 4 years ago

It's just mind-blowing and very sad that this is still an issue in 2020.

No one has an adequate solution for this issue.

You want feedback, you want to be open source, you want a community, well then don't ignore a major plain like this one for years. I get that it's hard to solve and maybe it's falling between two chairs and so on, but it has been years now!

We do not have a solution that we find adequate for this problem. Wanting this badly doesn't change that.

but you need to fix this and you need to fix it now...

We don't have a solution that people think is adequate to fix the issue :-/

Hoping for a status update and an ETA.

There is no status update. We're in the same boat as before. Hopefully someone can come up with a solution that all stakeholders feel is suitable.

CyrusNajmabadi commented 4 years ago

Having worked with software development 12 years now, in multiple companies, having had hundreds of developer colleagues, doing all kinds of application types, I can't think of anyone who hasn't written to write a library at some point.

That's not what was said. What was said was:

The TPL optimized for library consumers, where synchronization is the more commonly necessary. If you're putting ConfigureAwait(false) you're either doing it wrong

The design is to optimize for the majority case at the expense of the minority. You're in the minority, and thus this is unpleasant for you.

CyrusNajmabadi commented 4 years ago

I simply don't believe there are people out there that think ConfigureAwait(false)'ing every await is the right solution.

Thinking that we don't hav ethe right solution doesn't magically make a right solution appear. There are tons of things i do not like, but i don't know how to solve them. So the status quo is the best state we know of.

I can't say which team within Microsoft should solve this, nor can I say which solution is the best, but I do see a major pain going unanswered for years and years. Surely, we can all agree that someone has to do something and that someone is within Microsoft.

Teams at MS have looked at this, and no one has been able to come up with a suitable improvement. If you have a proposal, please let us know! :)

HaloFour commented 4 years ago

@blankensteiner

I'm sure you are right. Which repo/Microsoft-team do I have to contact? Even better, do you know any specific people?

I've proposed the following API that would temporarily reset the SynchronizationContext and TaskScheduler within a using block:

https://github.com/dotnet/runtime/issues/1448

That would apply to all methods called within that block so that it would only need to be used at the boundary of any libraries or other such code.

I'm not saying that this is the best answer, but it's a relatively easy API both to implement and use. I could see it being combined with analyzers that could help identify when you'd need to use it.

jmarolf commented 4 years ago

Currently this is a potential solution that I would be most ok with: https://github.com/dotnet/csharplang/issues/2649

But there is still a lot of design work ahead of us.

HaloFour commented 4 years ago

@jmarolf

But there is still a lot of design work ahead of us.

I already noted my many objections within the comments of that proposal. It's a shotgun and it will only result in very subtle bugs, assuming that it can survive design around the 5+ orthogonal features that would have to be individually shipped to enable it at all.

~20 or so LOC in the BCL would obviate the need for a language solution. Keep the concerns of the TPL to the TPL. That seems to me a better option than expecting everyone to have to copy&paste their own flavor of an await operator within their own project and hope that they scoped it carefully enough to only affect the code they intend it to.

jmarolf commented 4 years ago

Which is why I think the most correct answer is @CyrusNajmabadi's: We have no workable solution at this time. Creeping knowledge of how the BCL implements Task into the language is not something anyone in the LDM is excited about. Having a solution in the runtime or framework that could be idiomatically exposed in the language would be best and imho and Stephen's suggestion is a step in that direction but it has a lot of design issues (a lot) that would need to be worked out. I can see a path forward in https://github.com/dotnet/csharplang/issues/2649 but I do think there are lots and lots of usability and design questions that may kill it.

CyrusNajmabadi commented 4 years ago

On a personal level, i like @HaloFour 's proposal the most. The solution is visible in your source and easy to add. I don't like any sort of implicit changes here. Extension operators and implicit assembly changes are highly undesirable ot me. When it comes to await behavior and async/concurrency, i want to be able to see what's going on in the code front and center.

once you have a handful of awaits, i think it's very reasonable to just do:

public async Task<Something> DoSomeStuff()
{
    using var _ = Task.DisableSynchronizationContext()

    // no more sync context to worry about
    var x = await DoSomething();
    var y = await DoSomethingElse();
    var z = await DoOtherStuff();
    return await MixItAllTogether(x, y, z);
}

A very big positive is that it's simple, can be done today, and is very visible in the code. It's something i woudl accept into codebases i work on, unlike several other proposals that are highly problematic.