-
Notifications
You must be signed in to change notification settings - Fork 631
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
Comments
I would be happy to see the |
Same boat, happy to see it go. Thanks for bringing this into scope @stevejgordon. |
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. |
@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. |
@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. |
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. |
ASP.NET Core application is an entry point for the code. Since the entry point is not making any context switching (like the 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.
Again, one could argue that it’s a small optimization. Perhaps for this project using |
@SeanFeldman, thanks for taking the time to explain some of the finer intricacies of I agree with the "good habits" section, and understanding when and why a developer should use All I know is I feel like I understand |
@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:
|
@stevejgordon the links are embedded in my comment #1313 (comment) |
I found this ironic, considering it's involving the person that recommended against usage of |
@SeanFeldman, now that |
@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 : 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). |
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.
The text was updated successfully, but these errors were encountered: