Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] await should not capture SynchronizationContext by default #2746

Closed
ahdung opened this issue Aug 26, 2019 · 48 comments
Closed

[Proposal] await should not capture SynchronizationContext by default #2746

ahdung opened this issue Aug 26, 2019 · 48 comments

Comments

@ahdung
Copy link

ahdung commented Aug 26, 2019

I believe that the use of ConfigureAwait(true) is much less than ConfigureAwait(false), so instead of thinking about how to improve the ease of use of ConfigureAwait(false), let ConfigureAwait(false) be the default behavior, and let users who need ConfigureAwait(true) write it.

@canton7
Copy link

canton7 commented Aug 26, 2019

This would break thousands of applications. There are other proposals for achieving the same thing without breaking existing code, please search.

@ahdung
Copy link
Author

ahdung commented Aug 26, 2019

@canton7 I have searched.
Since Nullable reference types, there's nothing that can't be broken.

@Tom01098
Copy link

I don't know what you mean here, NRTs have been specifically designed to be backwards compatible.

@CyrusNajmabadi
Copy link
Member

I believe that the use of ConfigureAwait(true) is much less than ConfigureAwait(false)

Why do you believe that? In every application i've ever written, CA(true) far outweighs CA(false). I only do the latter for library code, and there is tons more non-library code than library-code out there.

Since Nullable reference types, there's nothing that can't be broken.

I don't know what you mean. NRT breaks no code at all. You can opt into it entirely without any of your code semantics changing at all. That was a major point of the design of it.

@ahdung
Copy link
Author

ahdung commented Aug 26, 2019

@CyrusNajmabadi non-library code != need CA(true), there is only UI and HttpContext need, you know that.

about NRT, i reserve opinion, to avoid expand the discussion, i will not discuss it any more.

@Tom01098
Copy link

If I understand you right, you want this to only work on specific types, such as HttpContext, why? Isn't that more confusing for a developer who has to check the documentation to know if they need to add ConfigureAwait(true) now?

@CyrusNajmabadi
Copy link
Member

@ahdung I don't know the point you're trying to make. You made a broad claim of: I believe that the use of ConfigureAwait(true) is much less than ConfigureAwait(false)

However, nothing we've seen has indicated that's the case. There are certainly users and codebases for which CA(false) is by far the dominant pattern. But in no way is it the broader use case. The vast majority of use is through normal await x without CA (which is the same as CA(true)), and which benefits from that being the default.

@CyrusNajmabadi
Copy link
Member

about NRT, i reserve opinion, to avoid expand the discussion, i will not discuss it any more.

That's fine. But then we go back to:

This would break thousands of applications. There are other proposals for achieving the same thing without breaking existing code, please search.

Proposing that ew break the vast majority of code out tehre to help a tiny minority isn't going to happen.

@ahdung
Copy link
Author

ahdung commented Aug 26, 2019

@Tom01098 I don't think it's a problem, as a knowledge point, class library users are obliged to learn it, just as anyone knows that they should not access the UI across threads.

@CyrusNajmabadi
Copy link
Member

@Tom01098 I don't think it's a problem

You are proposing that people have to unlearn everything they may know so far about tasks and CA. You are doing this, despite not having given sufficient explanation about why this would be beneficial. And you're proposal seems to involve potentially breaking nearly every app out there that uses CA.

This is a major problem. It's a complete non starter. It goes against a major pillar of C#/Roslyn in that breaking changes will only happen if both super valuable and of extremely low chance of being impactful on the existing ecosystem. This proposal violates both of those.

@yaakov-h
Copy link
Member

@CyrusNajmabadi

But in no way is it the broader use case. The vast majority of use is through normal await x without CA (which is the same as CA(true)), and which benefits from that being the default.

I find the exact opposite.

Capturing the current synchronization context is typically only needed in UI event handlers.

In console apps, ASP.NET Core, and in my own experience, unit tests, it has nil effect, as the synchronization context is the default threadpool context.

Most code that uses await is generally library code, since await is contagious. A single UI event handler using an implicit ConfigureAwait(true) is typically sitting on top of many lower calls that all have to call ConfigureAwait(false).

Since library code could be used by UI code, or in other scenarios where capturing the synchronization context can be anywhere from non-performant to full-blown deadlocking, it has to take the cautious approach and litter ConfigureAwait(false) everywhere.

This is a somewhat pervasive issue, which is why we have #645, #2542 and #2649, among others, and even the LDM has acknowledged it for addressing in a future release.

This particular Issue would introduce an unacceptable breaking chance, but you can't just dismiss the underlying need.

@CyrusNajmabadi
Copy link
Member

I find the exact opposite.

as i said:

There are certainly users and codebases for which CA(false) is by far the dominant pattern.

This particular Issue would introduce an unacceptable breaking chance, but you can't just dismiss the underlying need.

I never dismissed the underlying need.

@ahdung
Copy link
Author

ahdung commented Aug 26, 2019

@CyrusNajmabadi I agree that breaking is a real problem, i just think that it was a wrong decision let CA (true) as the default, so correct it better than fix it on the wrong direction.
Sometimes in order to have a better future, we have to make some sacrifices.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Aug 26, 2019

i just think that it was a wrong decision let CA (true) as the default

What are you basing that on?

so correct it better than fix it on the wrong direction.

That would be a massive breaking change. Which violates tenets that C# and Roslyn follow.

Sometimes in order to have a better future, we have to make some sacrifices.

You will need to both show that thsi would not only be of massive benefit, but that the sacrifices would be acceptable. So far, i've seen neither of those shown by you.

--

It's worth pointing out that the owners of the TPL themselves feel strongly that things be improved here. But even they (with all the data at their disposal) have never been able to make the above case. Unless you are coming with some staggering new source of data, i don't see how the equation could change here. It's simply not going to the case that such breaks will happen.

@canton7
Copy link

canton7 commented Aug 26, 2019

There would need to be an opt-in mechanism to avoid breaking 7 years' worth of existing code. Even NRTs have an opt-in mechanism. There are already proposals for specific opt-in mechanisms for CA.

@Tom01098
Copy link

There has only ever been one breaking change in C#, and that was on something which was fundamentally broken and unlikely to be used in the state it was before. As has been pointed out, this would actually change the semantics of every call to await in all programs written.

@CyrusNajmabadi
Copy link
Member

There are already proposals for specific opt-in mechanisms for CA.

Agreed. The only way this coudl happen would be an opt-in choice by the user to have different behavior only for the code under their control affected by that option.

@yaakov-h
Copy link
Member

There has only ever been one breaking change in C#

That's not strictly true.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Aug 26, 2019

There has only ever been one breaking change in C#,

Well... that's not actually true. Practically every release of C# has at least 1 breaking change. However, they're normally so minor as to probably not ever be noticed by the entirety of the C# ecosystem. :)

They fit in the "this is valuable enough, and the impact of changing is so minor" that they can be done.

@ahdung
Copy link
Author

ahdung commented Aug 26, 2019

@CyrusNajmabadi Yes, I can't prove it, maybe we need a broad vote something.

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi Yes, I can't prove it, maybe we need a broad vote something.

This is not a democracy.

The evidence and arguments have already been provided by the teams most knowledgeable about this domain. That was not sufficient to go against the general C#/Roslyn tenets around compat.

Your best bet woudl be to throw in your support for the proposals that improve the ergonomics for library authors, without sacrificing compat. Those proposals could actually happen. Proposals that break billions of lines of code across tens of thousands of companies is just not going to happen.

@ahdung
Copy link
Author

ahdung commented Aug 26, 2019

@CyrusNajmabadi What's the best way, you no right to judge, in this place, you are as user as I am.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Aug 26, 2019

I was one of the designers of the async/await feature, including working on it from inception to release and many versions afterwards :) I also was on the ldm for at least 10 years... not sure the total amount of time though. Can't remember exactly when i joined, and it wasn't a continuous stretch as i went off and did things at times like helping to create TypeScript :-)

@ahdung
Copy link
Author

ahdung commented Aug 26, 2019

@CyrusNajmabadi Guess you are a normal user, that's my fail. but it doesn't affect my point:

  1. let CA(true) as the default is a mistake;
  2. I believe that changing it is a better choice.

@canton7
Copy link

canton7 commented Aug 26, 2019

The hang-up is that breaking most UI applications written in the last 7 years is unacceptable. This is going nowhere unless you come up with a way of avoiding that level of breakage.

There are already proposals for this.

@qrli
Copy link

qrli commented Aug 26, 2019

This is really an education issue. Years ago someone was bite by careless use of await, and then come to the conclusion that ConfigureAwait(false) should be used everywhere. Then this crap spreads out everywhere, and now it is causing more trouble than deadlocking.

Concurrent programming is hard. It is impossible to guarantee carelessly written async code to be correct, no matter what default behavior is. Devs should know what sync context their code is supposed to run in, rather than saying removing all locks will make their program perfect.

@MgSam
Copy link

MgSam commented Aug 26, 2019

Of course the default cannot and should not be altered.

That said, I totally disagree with @CyrusNajmabadi. Are you really doing that much stuff on the UI thread that ConfigureAwait(true) is a sensible default?

I only do the latter for library code...

The vast majority of C# code out there is library code- or should be library code if it were properly factored (this includes web apps and desktop apps)- and the design that was chosen puts you squarely in the pit of failure. The design that was chosen optimizes for the "my first Windows Form Application" case over the work most developers are doing day in and day out.

Whenever I read code using await and there is no ConfigureAwait, it's almost an error. Just look at the popularity of ConfigureAwait analyzers for both VS proper and ReSharper to see the evidence of this.

IMHO, the async/await feature as it exists in the language today is broken in a fundamental way because of the poor default choice and the need for such a verbose helper method to be used everywhere.

@YairHalberstadt
Copy link
Contributor

As an aside, this is a library feature not a language feature. C# does not maintain which context is used. CoreFX does that.

@CyrusNajmabadi
Copy link
Member

That said, I totally disagree with @CyrusNajmabadi. Are you really doing that much stuff on the UI thread that ConfigureAwait(true) is a sensible default?

Having seen the design and position put forth by vs threading, I consider it the most same and sensible way to do things for the majority of code out there.

In a few places CA(false) is used. For the vast majority of other cases, you don't n need anything.

See https://github.com/Microsoft/vs-threading/blob/master/doc/library_with_jtf.md for more information on the jtf way of doing things

@ahdung
Copy link
Author

ahdung commented Aug 27, 2019

@qrli No one talking about can or not can use it except you, we talking about a choice.

@Tom01098
Copy link

@ahdung @qrli also explains why that choice is considered bad:

Devs should know what sync context their code is supposed to run in, rather than saying removing all locks will make their program perfect.

@ahdung
Copy link
Author

ahdung commented Aug 27, 2019

@Tom01098 so you mean if await not capture context, devs will not know these thing? what the logic.
This issue is about only one thing, it's choice, no more "philosophy".

@Tom01098
Copy link

@ahdung we already have the choice with using ConfigureAwait. Changing the default is a massive breaking change to the language.

@ahdung
Copy link
Author

ahdung commented Aug 27, 2019

@Tom01098 I agree that's the only&big problem.

@Duranom
Copy link

Duranom commented Sep 5, 2019

I never get the reason of people wanting ConfigureAwait by default false, most times they are backend only developers and don't understand there are more then just internal little libraries and their asp.net core apps.

When async/await was introduced there were 2 major framework of .NET that were context aware, GUI apps and ASP.NET (but also WWF, WCF). If ConfigureAwait(true) wasn't default then async/await would have caused thousand of issues for people and thousands more of people complaining then we had in the beginning now. Cause for some reason people had a hard time with async/await alone.

Now today you have ASP.NET Core and back-end developers there don't have to worry about using ConfigureAwait(true) anymore, in fact you don't have to define ConfigureAwait at all unless you create your own context aware things, which most don't do nor know how. So while it might sound logical to have ConfigureAwait(false) defined and as default it doesn't matter in that part, so turn off the analyzer that checks for that with ASP.NET Core apps and the libraries in the project that do not get shared outside of it.

But do take into account that there some other apps left where ConfigureAwait has impact, and for some a lot. Application like Console, UWP, WPF, WinForms, other Frameworks (with a context), Libraries and some other specialized types.

  1. Console, same as ASP.NET Core you don't need to use ConfigureAwait unless your going to do something fancy pancy where you need to be context aware, which most don't do or dare. (Though it is fun tbh)
  2. UWP/WPF/WinForms apps desktop and mobile, you seriously need to be context aware and having ConfigureAwait with true default makes most standard cases safe to use when forgetting it. When you get out of the simple cases you need to understand when to use ConfigureAwait with false and when with true.
  3. Other Frameworks, there are a lot of frameworks out there that have their own context mechanism, some have it working with async/await and some don't. There it is is extremely important to have true as a default as to ensure you have access to context data. (Think things like HttpContext that ASP.NET had)
  4. Libraries, these are used by GUI apps, other frameworks and other custom context aware specialized frameworks. With libraries you need to define ConfigureAwait anyway as the library doesn't know what you group you are targeting and it can be used by, but you do. So from the library perspective it is saver and better to have ConfigureAwait(true) as default so the more context-sensitive apps do not get screwed over.

Now, the only place where you would really want a default behavior change would be libraries, as you can make libraries for other apps but should not be context aware and you can make libraries for those that are like GUI components. It would be nice to have a flag in the future that will allow you to set the default for libraries as you know what the target group is but aside from libraries changing the default to false is either not needed nor desired.

@ahdung
Copy link
Author

ahdung commented Sep 6, 2019

@Duranom Classes not numbers, actually, unless a widespread vote, no one knows what is the real majority, I believe CA(false) is again.
"false by default" need, "flag for assembly/class/method" need too.

@Tom01098
Copy link

Tom01098 commented Sep 6, 2019

You could just use the Fody ConfigureAwait add-in which allows you to configure it at a global level.

No matter the outcome of a widespread "vote", this still wouldn't be changed in the language or BCL because of the huge backwards compatibility concerns.

@ahdung
Copy link
Author

ahdung commented Sep 6, 2019

@Tom01098 Thanks for your introduce, i know this thing, and i also know that this proposal is almost impossible to accept, so post this issue is more for expression.

@Andrew-Hanlon
Copy link

While I agree that this default was a mistake, I think there are simple, non-breaking, ways to address the issue. A project-level compilation flag would cover most of my gripes. But my preference would be for an alternative 'asyncf' keyword that made all awaits within the method default non-synchronized. Are there proposals for such a keyword?

@Tom01098
Copy link

The compilation flag introduces a dialect to the language where it becomes harder to reason about other people’s code online.

I also think asyncf is so close to async that it would be ridiculously easy to misspell it by accident.

@Andrew-Hanlon
Copy link

Andrew-Hanlon commented Sep 18, 2019

@Tom01098 I really just used 'asyncf' as a strawman. The point is a keyword that is non-breaking but minimal and easy to use.

@HaloFour
Copy link
Contributor

Currently the compiler is blissfully unaware of ConfigureAwait or SynchronizationContext, as long as whatever you're trying to await follows the "awaitable" pattern everything works fine. It's within the BCL that there's this notion of synchronization or a default behavior as to what that should be. Because of this I would prefer a BCL approach where you can disable synchronization for the body of a method (or whatever scope you choose). Here's a quick&dirty version of how this could be implemented:

SharpLab

@Andrew-Hanlon
Copy link

Andrew-Hanlon commented Sep 18, 2019

@HaloFour Why not both? Your suggested approach is great and wouldn't interfere with a method scoped (keyword driven) version (which I believe would account for the vast majority of uses).

@HaloFour
Copy link
Contributor

@Andrew-Hanlon

Why not both? Your suggested approach is great and wouldn't interfere with a method scoped (keyword drive) version (which I believe would account for the vast majority of uses).

If it can be managed reasonably well with a helper method I'd make the argument that it doesn't make sense to also add to the language. Adding features to the language is significantly more expensive and will likely take a long time, assuming that it's championed by the LDM at all. Any implementation as a language feature would require the C# language to take more direct dependencies on the implementation details as to how synchronization is implemented within the BCL. I'm not making the argument that it shouldn't happen, only that if the itch is scratched ....

@Andrew-Hanlon
Copy link

Andrew-Hanlon commented Sep 18, 2019

@HaloFour I mean obviously everything is down to priorities... but I personally don't think the 'itch is scratched' (nor, I assume, do most in these topics) since the point is to reduce code/cognitive clutter. Maybe there are also optimizations (perhaps only micro) that become possible if the compiler was context-aware.

@JohnNilsson
Copy link

It was mentioned above that a developer should know which context is required. So perhaps a slightly different take on this topic, instead of debating ergonomics of choosing defaults, how can the compiler help the developer know?

Would it be possible to add type annotations, on the apis that has certain context requirements and then have the compiler use that to help enforce things?

A kind of statically checked async scope if you will.

@jjxtra
Copy link

jjxtra commented Apr 7, 2020

What are the cons of using a ThreadPool or Task static bool called ConfigureAwaitDefault that can default to true and be set to false manually in the startup/main method?

I get that 3rd party library code might break when setting to false, that's why it would be an opt-in and make sure you test the hell out of your server application thing...

@YairHalberstadt
Copy link
Contributor

Whilst adding ConfigureAwait(false) everywhere is definitely a pain, a breaking change as big as this one is not really possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests