Comments (22)
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:
- Add a constructor that takes a
FlurlHttpSettings
object. - Extract an
IFlurlClient
interface so it can be easily mocked.
Thoughts?
from flurl.
- 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.
Related Issues (20)
- HttpTest WithRequestJson does not use JsonSerializer set on FlurlClientCache HOT 3
- Deadlock (or something weird) when calling `GetStringAsync` HOT 2
- 4.0.0 HOT 2
- System.EntryPointNotFoundException on PostJsonAsync HOT 1
- Add support for CancellationToken in IFlurlResponse asynchronous methods HOT 1
- Add option to disable conversion of plus signs to spaces for query parameters HOT 3
- According to RFC-7578 the Content-Disposition filename* parameter in multipart MUST NOT be used
- IFlurlClientBuilder.AddMiddleware cuts the chain of delegating handlers HOT 1
- Encode Issue HOT 2
- .net 8 blazor wasm PlatformNotSupportedException HOT 7
- GetJsonAsync<T> is not working after upgrading the Flurl Http to version 4 >= HOT 5
- The response ended prematurely HOT 2
- Trimming unused code causes the exception - Response could not be deserialized to JSON HOT 4
- Not able to configure Delegating handler in Flurl clientless DI configuration HOT 2
- Collection was modified; enumeration operation may not execute. HOT 1
- DownloadFileAsync => GetFileNameFromPath does not respect redirects HOT 1
- `OnRedirect` does not work with custom `HttpClient` HOT 4
- Serialization based on response status code HOT 1
- Support for Setting HttpRequestMessage.HttpVersionPolicy in FlurlHttpSettings
- SetQueryParam() parameters with the same name will be overwritten, either not overwritten or optionally overwritten HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from flurl.