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

async calls to HTTP clients #77

Closed
wants to merge 1 commit into from

Conversation

szybkiwx
Copy link

We started using your library some time ago, but recently we met the problem described here http://stackoverflow.com/questions/32005912/running-xunit-tests-on-teamcity-using-async-methods
Our XUnit2 test when running in parallel (default option) falls into deadlocks on some automated build agents (it's very hard to reproduce in local environments).
This pull request reflects the changes we made to the library to utilize async methods of HttpClient properly and get rid of a problem:

  1. Target .NET 4.5
  2. Use async/await all the way down
  3. Update xunit to 2.1 make async methods testing easier

@neilcampbell
Copy link
Member

neilcampbell commented Jan 20, 2017

Hi @szybkiwx
Thanks for the PR. Sorry it's taken a while to review this one.

  1. I would like to keep version compatibility with .NET 4.0, as I know some users which still use Pact with .NET 4.0.
  2. In some instances I don't actually want to expose the underlying mechanics that an HTTP request is being sent, however I your issue is definitely valid. I have an idea for how I can make things work.
  3. 👍

Leave it with me and I will hopefully get some time tonight to have a play.

@neilcampbell
Copy link
Member

@szybkiwx I made a change, which should hopefully fix the xunit 2 issues.
Are you able to test it out for me in your build environment that you were having issues with?

To install use Install-Package PactNet -Version 1.3.0-beta -Pre

Let me know how you go, then we can move onto the other parts of the PR.

@szybkiwx
Copy link
Author

Hi @neilcampbell, that was quick:) Unfortunatelly your fix from 1.3.0-beta did not work, however I took liberty to fiddle with the code you commited to master, did some additional reading and here's what I came up with:
I replaced all calls to RunSync extension methods with this ugly monster
Task.Factory.StartNew(() => _httpClient.SendAsync(request).Result).Result;
and it actually DID work. Basically this is the .NET 4.0 way of writing Task.Run(() => _httpClient.SendAsync(request)).Result;. The .NET 4.5+ .Run method is calling Task.Factory.StartNew and unwraps the inner Task (please refer to https://blogs.msdn.microsoft.com/pfxteam/2011/10/24/task-run-vs-task-factory-startnew/ ) . Citing this comment http://stackoverflow.com/a/32247745/532038 doing that "gets rid of the SynchronizationContext by forcing the function into the thread pool", and single-threaded SynchronizationContext XUnit2 uses is the root cause of the deadlock. I compiled the code manallly and uploaded it with our tests to the build agent and builds are green now.
I perfectly understand that you wish to keeping backward compatible with existing clients, although all sources I've browsed discourage from using sync wrappers around async methods and advise to make it asyn all the way down (e.g. https://blogs.msdn.microsoft.com/pfxteam/2012/04/13/should-i-expose-synchronous-wrappers-for-asynchronous-methods/). Anyway, hope this helps:)

@neilcampbell
Copy link
Member

Thanks @szybkiwx, awesome stuff!
I just published a new version of the package (1.3.0), which uses the method you mentioned above. If you could give that a go, it would be much appreciated!

With regards to passing the task all the way back to the consumer, I do agree however my goal here was to hide the internal workings of those methods, as in the future we may not use a HTTP (or any async) call to setup the mock interactions. The early version of the library actually didn't.

@szybkiwx
Copy link
Author

Hi, sorry for late answear, been quite busy recently. Unfortunately that didn't work as expected but I still need some time to dig into it.

@neilcampbell
Copy link
Member

No worries @szybkiwx! Keep me posted with how you go!

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.

2 participants