tmenier / Flurl

Fluent URL builder and testable HTTP client for .NET
https://flurl.dev
MIT License
4.21k stars 388 forks source link

configureAwait on callback events blocks updates to #546

Open a-teece opened 4 years ago

a-teece commented 4 years ago

When using the callbacks available in settings the function Flurl is not executing the delegate in the context of the calling code block - this causes the update of member variables to only work "sometimes"; Intermittent due to the fact the ConfigureAwait does nothing if the task has already completed.

This seems to be because configureAwait(false) is set when raising the event.

I appreciate that setting configureAwait(false) is all over Flurl, and that this is likely correct and best-practice. However; as described in https://devblogs.microsoft.com/dotnet/configureawait-faq one exception to the rule "if you’re writing general-purpose library code, use ConfigureAwait(false)" is when passing a delegate to be invoked.

Can i suggest that the calling of these delegates does not set ConfigureAwait(false).

tmenier commented 4 years ago

Interesting. I think you might be right here, but a code sample would be helpful. Would you be able write a minimal example that demonstrates the issue? Perhaps with a Task.Delay to get it to repro consistently? (I'm almost willing to take a leap of faith on it but I'm really curious to actually see it.)

a-teece commented 4 years ago

I'll try to throw something together next week. I faced/found the issue when using Flurl in a MSTest v2 project. I hooked into the AfterCallAsync (also tried AfterCall) in order to store the response in a member variable.

I'll setup something using Tasks myself instead of MSTest. Should be a simple repo.

tmenier commented 4 years ago

Sounds good, thanks!

a-teece commented 4 years ago

So initially i've been unable to replicate using a .net Core Console App. Further reading on the subject suggests that if SynchronizationContext.Current is null then ConfigureAwait(false) will actually do nothing/isn't really required - without a sync context the await can execute in any context and gain the perf and reliability benefits of that.

However my issue was found in a test using SpecFlow and MSTestv2. SpecFlow is supplying a SynchronizationContext.

So i've replicated the issue with WinForms which also supplies a context (so that GUI updates are done on the GUI thread) to avoid cross-thread updates. Sure enough you get a cross-thread error. I believe if ConfigureAwait(false) wasn't used when calling the delegate then this sample would work perfectly. Flurl WinForms Replication.zip

tmenier commented 4 years ago

Thanks for the repro. I think I understand the problem better now, and the ConfigureAwait(false) on the call to your delegate isn't the real problem. In this example, you still have access to the sync context inside yourCallback; the ConfigureAwait(false) affects the continuation, i.e. everything happing inside the library after your callback completes.

await yourCallback(call).ConfigureAwait(false);
// now the sync context is gone

You can prove this to yourself in your repro by wiring your callback to BeforeCallAsync rather than AfterCallAsync. In order for the sync context to not be lost in AfterCallAsync, I'd need to make sure the library never uses ConfigureAwait(false) at any point before invoking that delegate. That includes the underlying HttpClient call.

I'm not sure what to do here. I don't want to just drop all the ConfigureAwaits in that call stack and lose the efficiency benefits to support a somewhat fringe scenario. But I'll keep this open and give some thought to providing some kind of opt-in way to flow the context. Let me know if you have any specific ideas.