Skip to content

Add ConfigureAwait(false) to all task awaitings (#22)#23

Merged
Choc13 merged 1 commit intowintoncode:masterfrom
govorovvs:issue/22
Dec 16, 2017
Merged

Add ConfigureAwait(false) to all task awaitings (#22)#23
Choc13 merged 1 commit intowintoncode:masterfrom
govorovvs:issue/22

Conversation

@govorovvs
Copy link
Copy Markdown
Contributor

@govorovvs govorovvs commented Dec 14, 2017

Addresses #22

@Choc13 Choc13 self-requested a review December 15, 2017 22:48
@Choc13
Copy link
Copy Markdown
Collaborator

Choc13 commented Dec 15, 2017

Hi again @govorovvs and thanks for the PR. Is there any documentation you can point me towards that suggests doing this as a standard practice for all awaitable task calls? It's not something I've come across before and was not aware it was something that was recommended.

Is this causing you problems when you are using the lib, or are you just trying to apply some best practices to the code base?

@govorovvs
Copy link
Copy Markdown
Contributor Author

@Choc13, Not using .ConfigureAwait(false) may causing performance problems.
Here is explanation: aspnet/AspNetKatana#50

@Choc13
Copy link
Copy Markdown
Collaborator

Choc13 commented Dec 16, 2017

OK, I've read through that issue and the linked resources from there. It would seem that you are correct and for libs it is almost certainly better to use .ConfigureAwait(false) as libs shouldn't be aware of the SynchronizationContext in which they are running and therefore don't need to ensure that the continuation is performed on the same context.

However on the flip side it would seem that adding ConfigureAwait(false) is a micro-optimisation and given that the async calls here aren't invoked with a high frequency I'm not sure it's going to make much difference.

I'm inclined to accept though given that it's recommended by Microsoft and that there doesn't seem to be any conceivable downside.

@jhickson do you have any thoughts on this PR, it seems like something you probably encountered in Winton.Extensions.Threading.Actor?

@govorovvs
Copy link
Copy Markdown
Contributor Author

Be aware using ConfigureAwait(false) with [ThreadStatic] (Actor.cs file) because function will continue execution in other thread after awaiting. In this case it's better to use AsyncLocal

@jhickson
Copy link
Copy Markdown

@Choc13, yes, this is something that should be done. In the context of Winton.Extensions.Threading.Actor, the issues are different because often you do want the synchronisation context to be remembered. However, in your library adding ConfigureAwait(false) is safer.

Hi @govorovvs. In the context in which the [ThreadStatic] in Actor.cs is used, it needs to be [ThreadStatic]: [AsyncLocal] is not appropriate.

@Choc13
Copy link
Copy Markdown
Collaborator

Choc13 commented Dec 16, 2017

@jhickson thanks for the confirming, I’ll approve.

@Choc13 Choc13 merged commit ce4a399 into wintoncode:master Dec 16, 2017
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

Successfully merging this pull request may close these issues.

3 participants