Allow HttpTest to work correctly when different tests are running in parallel about flurl HOT 22 CLOSED

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.

from flurl.

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?

from flurl.

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

from flurl.

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

from flurl.

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

from flurl.

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.

from flurl.

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

from flurl.

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

from flurl.

Why did you mark this issue ready?

from flurl.

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.

from flurl.

@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.

from flurl.

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.

from flurl.

@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?

from flurl.

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

from flurl.

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.

from flurl.

@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?

from flurl.

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.

from flurl.

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.

from flurl.

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

from flurl.

@tmenier Great job! I am testing!

from flurl.

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

from flurl.

@tmenier I have no issue with parallel testing!

from flurl.