Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Should ConfigureAwait(false) be called in NancyHost? #2471

Closed
xt0rted opened this issue May 30, 2016 · 11 comments
Closed

Should ConfigureAwait(false) be called in NancyHost? #2471

xt0rted opened this issue May 30, 2016 · 11 comments

Comments

@xt0rted
Copy link
Contributor

xt0rted commented May 30, 2016

While looking through the code and using the ReSharper ConfigureAwait Checker plugin I came across Nancy.Hosting.Self/NancyHost.cs#L133-134.

Should line 133 and/or 134 call .ConfigureAwait(false)? The other warning I'm getting is on line 134 because it's not awaited but I wasn't sure if that should be or not.

Those are the only two spots outside of the test projects that don't call ConfigureAwait.

@aidapsibr
Copy link
Contributor

Since NancyHost is the consumer of the task it does need to capture context.

@thecodejunkie
Copy link
Member

@khellang @damianh @grumpydev

@damianh
Copy link
Member

damianh commented Aug 25, 2016

Probably should be called from everywhere.

Explicit or assembly rewrite with a Fody plugin?

On 25 Aug 2016 12:51 p.m., "Andreas Håkansson" [email protected]
wrote:

@khellang https://github.com/khellang @damianh
https://github.com/damianh @grumpydev https://github.com/grumpydev


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2471 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADgXNGXUvhA1_ukmq0UdHjK25k1thVKks5qjYHOgaJpZM4Ipc5S
.

@horsdal
Copy link
Member

horsdal commented Aug 26, 2016

I think line 133 should have a CAF.
Basically every await outside of tests should have a CAF AFAIK.

@thecodejunkie
Copy link
Member

Been out of the loop a bit, but would adding that ConfigureAwait call fix #2534 ?

@jchannon
Copy link
Member

What if you host by something else?

On 30 August 2016 at 12:41, Andreas Håkansson [email protected]
wrote:

Been out of the loop a bit, but would adding that ConfigureAwait call fix
#2534 #2534 ?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2471 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGaprHexEiti95Sm36yjkIm-kZVeZlPks5qlBb7gaJpZM4Ipc5S
.

@thecodejunkie
Copy link
Member

thecodejunkie commented Aug 30, 2016

Well Nancy is a library and if you consume it yourself you are subjected to following the laws of async, thus making sure you properly do your own ConfigureAwait-handling. Or am I thinking about this in the wrong way?

@damianh
Copy link
Member

damianh commented Aug 30, 2016

You are thinking about this in the correct way :)

@thecodejunkie
Copy link
Member

@damianh so we should get this in, right?

@damianh
Copy link
Member

damianh commented Aug 30, 2016

Yep!

On 30 Aug 2016 2:45 p.m., "Andreas Håkansson" [email protected]
wrote:

@damianh https://github.com/damianh so we should get this in, right?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2471 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADgXAXWJHLsU8hmOdflob1K6rQngM1jks5qlCXWgaJpZM4Ipc5S
.

@thecodejunkie
Copy link
Member

Oh wait.. this is an issue, not a pull-request.. @xt0rted mind sending a PR and I'll get it in asap?

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

No branches or pull requests

6 participants