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

Cleanup use of ConfigureAwait (Discussion) #1313

Closed
stevejgordon opened this issue Oct 5, 2016 · 13 comments
Closed

Cleanup use of ConfigureAwait (Discussion) #1313

stevejgordon opened this issue Oct 5, 2016 · 13 comments
Assignees
Labels

Comments

@stevejgordon
Copy link
Member

After chatting on Slack to David Fowler (genius) he explained that there is no sync context in ASP.NET Core so the call to ConfigureAwait(false) is redundant and does nothing.

Old ASP.NET has a sync context for legacy reasons.

I won't pretend to understand 100% how sync contexts work but having had it confirmed I would recommend we take this call off all Tasks we are awaiting. It will helpfully make the code less intimidating for newcomers. Not an urgent issue.

@MisterJames @dpaquette - Before we work on this I wanted to check others are happy with the reasoning and approach?

@tonysurma I'll let you decide which milestone this fits into. I'm happy to work on it and in reality it's mostly find/replace and then a big commit after it tests correctly.

@dpaquette
Copy link
Collaborator

I would be happy to see the .ConfigureAwait(false) removed since it adds a lot of noise and isn't well understood. I don't fully understand why it is no longer needed in Core. I'm going to try to dig in to that purely out of curiosity.

@MisterJames
Copy link
Contributor

Same boat, happy to see it go. Thanks for bringing this into scope @stevejgordon.

@stevejgordon
Copy link
Member Author

stevejgordon commented Oct 6, 2016

Cool, I caught David again earlier and verified that this is true even when running on full .NET framework as we do currently. Basically old ASP.NET used the sync context to restore things like HttpContext.Current on the right thread and there where things linked to the IIS pipeline.

All gone in ASP.NET core so will be nice to clean this up.

@mgmccarthy
Copy link
Collaborator

@stevejgordon, I'm pretty floored by this information, especially since in conversations with @SeanFeldman, he recommended the use of ConfigureAwait.

I also do not fully understand the difference between the synchronization context in ASP.NET "Classic" and ASP.NET Core, but it looks like David Fowler is an ASP.NET Core Architect, so I'm thinking we should follow his lead here.

I was pushing the use of ConfigureAwait on the handlers a while a ago, and it resulted in a lot of unnecessary code being written, sorry about that.

@stevejgordon
Copy link
Member Author

@mgmccarthy Don't worry about it. From the information at hand at the time it seemed like the correct approach. Unfortunately there are no obvious documents covering this and for me at least it's a complicated subject to digest.

@SeanFeldman
Copy link
Contributor

My sincere apology. I'm quite surprised by this guidance myself considering ASP.NET core is not a UI related code. Though I'm not an ASP.NET core architect (and human). Will try to get more information on why exactly this is not needed. Also, the difference between a controller and let's say database access component under the same roof of ASP.NET core app.

@SeanFeldman
Copy link
Contributor

ASP.NET Core application is an entry point for the code. Since the entry point is not making any context switching (like the HttpContext.Current), it is indeed not needed to restore the context, and therefore by default .ConfiguraAwait(true), or not specifying .ConfigureAwait(true) at all, will work.

Simplification of the code is a very strong and compelling point. Especially when the code used in the project is never going to be used from any over point of entry that could be UI code.
To contrast that, there are two things that I just can’t omit:

  1. If the purpose of the project is to teach good habits, dropping .ConfigureAwait(false) will never force people to think about in any other code they’ll write and use async. Arguably, not the strongest point for HTBox, but had to bring it up.
  2. Performance. Looking at the .ConfigureAwait() code and execution path, when false is not passed in (ie default true is used), the code has to go through the logic within the main if-block. If false is used (ie explicit .ConfigureAwait(false) is used), execution is fast forwarded to a single block execution.

Again, one could argue that it’s a small optimization. Perhaps for this project using .ConfigureAwait(false) might not be necessary if the reasons above are considered of a lesser importance. Sorry for causing you any friction.

@mgmccarthy
Copy link
Collaborator

@SeanFeldman, thanks for taking the time to explain some of the finer intricacies of ConfigureAwait.

I agree with the "good habits" section, and understanding when and why a developer should use .ConfigureAwait(false) is a good idea. On the flip side is most people don't understand it, and we want to lighten the cognitive load for contributors to the project.

All I know is I feel like I understand ConfigureAwait(false) now by working on this project (and in discussions with you), and before, I didn't know what it meant.... so either way, thanks for the time you spent with the project teaching us correct usage.

@tonysurma tonysurma added this to the Technical Backlog milestone Oct 7, 2016
@stevejgordon
Copy link
Member Author

@SeanFeldman No need to apologies for raising it. I learnt a lot from your information and as I'd never been aware of it previously. I still can't confess to feeling I understand it fully, but I know enough to use ConfigureAwait in my older 4.x apps in many cases.

To your points above:

  1. While I agree on sharing good practice I would suggest that if it's not required in Core we need to guide people that way since this is a Core project. Otherwise we risk going to other way and encouraging use where not necessary.
  2. Thanks for checking into that. If the gain is worth the extra code and possibly confusion that it may be worth leaving but I've just taken it out of another application and seen no noticeable difference. I'd love to understand this more though. Do you have a link to the source as I'd like to see if I can understand this better for my own experience.

@SeanFeldman
Copy link
Contributor

@stevejgordon the links are embedded in my comment #1313 (comment)
You'll need to use desktop rendering to see those.

@SeanFeldman
Copy link
Contributor

I found this ironic, considering it's involving the person that recommended against usage of ConfigureAwait(false) for this project, being corrected by one of the TPL folks. Food for thought.

@mgmccarthy
Copy link
Collaborator

@SeanFeldman, now that .ConfigureAwait() has been pounded in everyone's heads, Microsoft has the job of getting RID of it for ASP.NET Core. A good problem to have IMHO. Thanks for sharing.

@RoyiNamir
Copy link

@stevejgordon I think there is a small thing which hasn't been discussed about.

When I create a .net core project(webpi) for exsmple , it runs under a console application :
image

And console apps doesn't have synchronization context.

So should I or shouldn't I use configureAwait in my webapi core app ?

(it's not a library nor consumed by ui - pure simpile webapi project).

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

No branches or pull requests

7 participants