Allow HttpTest to work correctly when different tests are running in parallel

[kroniak]

The case:

In one test class I use some code like:

using (var httpTest = new HttpTest())
            {
                httpTest
                    .RespondWithJson(new { count = 0, total = 0, order = "[]" });
                var result = await _client.CheckOrdersAuthAsync(); //using Flurl.Http
                httpTest.ShouldHaveCalled($"{CheckOrdersUrl}&limit=0")
                    .WithVerb(HttpMethod.Get)
                    .Times(2);
                Assert.Equal(true, result);
            }

and in the other test class:

            var result = await client.CheckOrdersAuthAsync(); //using a real Flurl.http method
            Assert.Equal(true, result);

I have many tests and when I run them all together real test sometimes uses HttpTest responces and throw exceptions.

It's bug or I does not understand something?

[tmenier]

There's a notable limitation in how HttpTest works: it puts Flurl in test mode globally so it is not thread-safe. The result if is that if you try to run tests in parallel, you could definitely run into issues. Are you running tests in parallel?

[kroniak]

Yes, in parallel. How to get around this? There are a way?

[tmenier]

There is no workaroud currently. I will keep this issue open and explore some possibilities when I have some time.

[kroniak]

Do you think about
[ThreadStatic] attribute on settings static field on http class or threadlocal class for settings? It also applies lazy interface.

[tmenier]

This could be tricky. Since we're usually dealing with async code, I think you'll want to look at Logical CallContext for this.

http://blog.stephencleary.com/2013/04/implicit-async-context-asynclocal.html

Even then, if the code a user is testing is multithreaded this could break down. I'll be interested to see what you come up with though.

[tmenier]

If this isn't ready I would be ok with shelving it for later. (1.1?)

[kroniak]

@tmenier I want to fix it ASAP. And I am testing new packages with Xamarin and win10 yet. There are problem with it.

[kroniak]

Why did you mark this issue ready?

[tmenier]

No idea how that happened. I moved it back.

By the way, I'm going to try and wrap up #48 and some misc refactoring this week, then I'll be ready for 1.0. Any thoughts on when you think you'll be ready? No rush, I'll actually be away most of next week anyway.

[kroniak]

@tmenier I think, think and I see that only one right way is remove a static global configuration for Flurl.Http. Global static config pattern is antipattern.
In modern application right way to use DI to inject to service (FlurlClient) specific options like timeout and others to overload default settings.

Like:

public void ConfigureServices(IServiceCollection services)
        {
            services.AddFlurlClient(options =>
                options.Timeout(Configuration.GetConnectionString("Flurl:Timeout")));
        }

HttpTest too be able working with DI.
What do you think about remove static GlobalConfig??

I see 2 cases which can not works with global static config:

• parallel testing
• using varios config for client in different threads.

[tmenier]

The # 1 goal of Flurl is to save keystrokes. The trade-off is that some of the most keystroke-saving patterns do not lend themselves well to DI. For example, what exactly do you inject into a class or method that does this?

await "http://api.com".GetAsync();

I knew from the start this would create a problem for testing, which is why I created the test helpers. Parallel testing has become much more common since I started this, so I agree this is an issue that should be addressed or clearly documented as a limitation.

Your second concern (clients in different threads) is addressed by the fact that each client gets a copy the global config (via FlurlClient.Settings). So the global settings mainly exist just to hold defaults. I don't agree with the general comment that global config is "bad". I think the way Flurl does it is appropriate.

Which leaves testing. There's nothing preventing someone from using DI and Flurl in the same project. They would just wrap the details of the HTTP calls into service objects that can be injected/mocked/stubbed. Use of Flurl would be an implementation detail of the "real" implementation. I think this is an appropriate architecture for bigger applications.

For simpler cases, which Flurl provides helpers for, I think the ease of use comes with the tradeoff of no parallel testing, or we try to fix this by storing the response queue and call log on the logical call context rather than in global scope. I think this would be worthwhile.

[kroniak]

@tmenier IOC injects option into FlurlClient when it was created.

public FlurlClient(IOptions options){}

services.AddFlurlOptions(new ...)
             .AddFlurlClient();

In diff situation options maybe different.
I try to do something to case when options will be different in different threads. Global static options are my pain. How to do that without global FlurlClient?

[kroniak]

@tmenier Please answer what do you think about "How to do that without global FlurlClient?"?

[tmenier]

Sorry. What IoC container are you using? Doesn't it allow some sort of factory method for resolving a type? In that method I would create a FlurlClient, configure its settings via the Settings property, and return it. Again, each FlurlClient instance gets its own copy of those settings. The global settings basically just give you a way to set defaults. I don't think they're getting in the way of doing IoC correctly.

Maybe all you really want is a new FlurlClient constructor that takes a settings object as a parameter? I would be open to adding that. It fits the IoC resolution pattern a little better, event though I think using a factory method like I described is an ok work-around.

[tmenier]

@kroniak I've taken a break from working on Flurl but I'm ready to get back at it. Parallel testing is my top priority.

I tried storing HttpTest on the logical call context and it worked like a charm. The bad news is it's not supported on all platforms. I believe I can do some platform sniffing and get it to work on .NET 4.5/4.6 and Core, and simply state that parallel testing with HttpTest doesn't work when the tests are running on other platforms. That should be satisfactory for most.

But I also want to make sure that Flurl is DI friendly. I still think you would have to give up syntax like "http://api.com".GetAsync() if you are doing DI with FlurlClient, but if you commit to using WithUrl / WithClient then it can be done. In my mind, these changes would make FlurlClient more DI/testing friendly:

1. Add a constructor that takes a FlurlHttpSettings object.
2. Extract an IFlurlClient interface so it can be easily mocked.

Thoughts?

[kroniak]

1. Customers must to add FlurlClient object to all API classes to work with specific classes. It's great refactoring.

I see one way - It's isolating static fields for threads. In different threads will be different options. Thread can change it with FlurlHttp.Settings or gets default.

[tmenier]

The problem with using thread local storage is that we're dealing with async code and often not resuming on the same thread, so I don't think that will work here. But this is exactly what logical call context (and the newer AsyncLocal) were created to address.

[tmenier]

Parallel tests are working (except on PCL)! I think this test proves it. Here's the platform-specific call context stuff if you're curious. Other suggested changes moved to #146. Prerelease for Flurl.Http 1.1 coming shortly.

[tmenier]

Please test the prerelease https://www.nuget.org/packages/Flurl.Http/1.1.0-pre

[kroniak]

@tmenier Great job! I am testing!

[tmenier]

@kroniak #148 was reported but I am not able to repro. Interested to hear your results.

[kroniak]

@tmenier I have no issue with parallel testing!