dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.08k stars 4.69k forks source link

Better HttpClient #53371

Closed John0King closed 3 years ago

John0King commented 3 years ago

The current HttpClient have 2 main problem: 1) ingleton HttpClient doesn't respect DNS changes #18348 2)using IDisposeable patter cause server's port be exhaust

and IHttpClientFactory is a heavy library to solve those two problem, but It's heavy and the source of problem is not be resolved.

and SocketHttpClientHandler is not available on browser platform, and PooledConnectionLifetime is only exist on SocketHttpClientHandler, so static instance is not a reliable solution.

I have a issue discussed before in #27601 , and some is talk to me to use IHttpClientFactory directly, or create something similar to mange HttpClient instance, but Can we all agree that the problem is happen in HttpClient , the IHttpClientFactory solution can be use temporary , but will we use it forever ?

HttpClientFactory is heavy for library usage, and because that , most library just rely on you to pass an HttpClient to it or an IHttpclientFactory to it. it indeed change the way how we build our library that using Http api !!!!!

but the fact is , look at other language, it seems no other language that complaint their http api , (Java,Php, Python, node, go, rust , ect.) , .Net is the only platform that all developer complaint the HttpClient is terrible. and I wonder how other languge or curl handle this !

My current thought is to create a another lightweight HttpClient.

  1. it has a DnsPool and a ConnectionPool.
  2. support IDisposable
  3. support interceptor mode instead of decorate patter.

it should design to be use with using(var http = new HttpClientVNext() ), and each instance have it's own DnsPool , all instance share a default ConnectionPool, so after dispose, only dnsPool be cleared, but the connection still managed by ConnectionPool and intercepter should like asp.net core middleware

class HttpClientVNext
{
    public HttpMessageHandler {get, private set }  // allow us change the implementation

   public DnsPool  DnsPool {get;}  // DnsPool should be able to set default dns timeout

   public ConnectionPool ConnectionPool {get;}

   public void SetHandler(HttpMessageHandler  handler);

   public void SetDnsPool(DnsPool   pool)
   public void SetConnectionPool(ConnectionPool  pool);  

   public IntercepterBuilder Intercepters {get; set;}

   .... Send, get,post  ect. ........

}

usage

public class MyApiClient
{
protect HttpClientVNext  HttpClient;
    public MyApiClient()
 {
    HttpClient = new HttpClientVNext();
    HttpClient.BaseUrl = "https://www.myapi.com";

    HttpClient.Intercepters.UseRetryIntercepter( 2); // extension method
   HttpClient.Intercepters.Use( async (context, next)=> {  context.Request.header["Authorazation"] = "Bearer xxxxxx" ; await next(context);     } );
 }

  public Dto  GetList()
  {
       return HttpClient.GetJsonAsync("/api/list") // extension method.
   }
}
ghost commented 3 years ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

Issue Details
The current HttpClient have 2 main problem: 1) ingleton HttpClient doesn't respect DNS changes #18348 2)`using IDisposeable` patter cause server's port be exhaust and `IHttpClientFactory` is a heavy library to solve those two problem, but It's heavy and the source of problem is not be resolved. and `SocketHttpClientHandler` is not available on browser platform, and `PooledConnectionLifetime` is only exist on `SocketHttpClientHandler`, so static instance is not a reliable solution. I have a issue discussed before in #27601 , and some is talk to me to use `IHttpClientFactory` directly, or create something similar to mange `HttpClient` instance, but Can we all agree that the problem is happen in `HttpClient` , the `IHttpClientFactory` solution can be use temporary , but will we use it forever ? `HttpClientFactory` is heavy for library usage, and because that , most library just rely on you to pass an `HttpClient` to it or an `IHttpclientFactory` to it. it indeed change the way how we build our library that using Http api !!!!! but the fact is , look at other language, it seems no other language that complaint their http api , (Java,Php, Python, node, go, rust , ect.) , .Net is the only platform that all developer complaint the `HttpClient` is terrible. and I wonder how other languge or `curl` handle this ! My current thought is to create a another lightweight `HttpClient`. 1. it has a `DnsPool` and a `ConnectionPool`. 2. support `IDisposable` 2. support interceptor mode instead of decorate patter. it should design to be use with `using(var http = new HttpClientVNext() )`, and each instance have it's own `DnsPool` , all instance share a default `ConnectionPool`, so after dispose, only `dnsPool` be cleared, but the connection still managed by `ConnectionPool` and intercepter should like `asp.net core middleware` ```C# class HttpClientVNext { public HttpMessageHandler {get, private set } // allow us change the implementation public DnsPool DnsPool {get;} // DnsPool should be able to set default dns timeout public ConnectionPool ConnectionPool {get;} public void SetHandler(HttpMessageHandler handler); public void SetDnsPool(DnsPool pool) public void SetConnectionPool(ConnectionPool pool); public IntercepterBuilder Intercepters {get; set;} .... Send, get,post ect. ........ } ``` usage ```C# public class MyApiClient { protect HttpClientVNext HttpClient; public MyApiClient() { HttpClient = new HttpClientVNext(); HttpClient.BaseUrl = "https://www.myapi.com"; HttpClient.Intercepters.UseRetryIntercepter( 2); // extension method HttpClient.Intercepters.Use( async (context, next)=> { context.Request.header["Authorazation"] = "Bearer xxxxxx" ; await next(context); } ); } public Dto GetList() { return HttpClient.GetJsonAsync("/api/list") // extension method. } } ```
Author: John0King
Assignees: -
Labels: `area-System.Net.Http`, `untriaged`
Milestone: -
antonfirsov commented 3 years ago

@John0King you should take a look at LLHTTP (a prototype project), which aims to provide all the building blocks to manage your pools, and build a different high-level API: https://github.com/dotnet/designs/blob/main/accepted/2021/flexible-http.md https://github.com/dotnet/runtimelab/tree/feature/LLHTTP2

stephentoub commented 3 years ago

The current HttpClient have 2 main problem

Neither of these require a new class.

For "ingleton HttpClient doesn't respect DNS change", HttpClient (via SocketsHttpHandler) allows you to set a maximum lifetime on how long connections are able to be reused. If you set that to something like a minute, then after that minute the next request will end up having to open a new connection, which will entail requerying DNS. So it does end up respecting DNS changes on whatever frequency you specify. And if we decided that was insufficient and we wanted to respect DNS TTL (which would require new support for getting those), that could be done without any additional surface area, purely as an implementation detail of SocketsHttpHandler. And if we wanted to configure it, it could be new properties on SocketsHttpHandler.

For "using IDisposeable patter cause server's port be exhaust", it's disposable for the same reason other types are disposable: dispose it when you're done with it and want its resources cleaned up. The HttpClient instance stores resources like a connection pool, so it shouldn't be disposed until you want to clean up that connection pool. It should be created, used repeatedly for all relevant requests, and then when you're done with it, dispose it (or just exit the app).

geoffkizer commented 3 years ago

There's an old issue about respecting DNS TTL here: https://github.com/dotnet/runtime/issues/23652

We closed that because we thought PooledConnectionLifetime was sufficient to address the scenario. But if you think we should reconsider, feel free to reactivate that issue or file a new one with an explanation of why the current solution is lacking.

John0King commented 3 years ago

@stephentoub @geoffkizer PooledConnectionLifetime is only exists on SocketsHttpHandler , and it's not a common used HttpMessageHandler , and it do not support "blazor webassembly" I think

[System.Runtime.Versioning.UnsupportedOSPlatform("browser")]
public sealed class SocketsHttpHandler : System.Net.Http.HttpMessageHandler

in fact, we should using HttpClientHandler / DelagteHandler , and then we lost PooledConnectionLifetime property.

If you set that to something like a minute, then after that minute the next request will end up having to open a new connection, which will entail requerying DNS.

a minute sound like short , but imagine ”Azure“ have 1 minute dns problem !!! one request broken is allowed, 1 minute of all request do not sound good !! benchmark of asp.net core have 1,276,414 per second request, and 1 minute is 76,584,840 request (I know HttpClientFactory can be fix solution, but like I said, it shouldn't be long time solution)

those two pain point happened just after HttpClient born (in .net fx 4) we should confirm that the experience of HttpClient is painful, that also why HttpClientFactory come in, and there no such design in Java/Node/Php/Go

I do not want to introduce break changes to current app/lib, that's why I ask for a new class, and with the better experience (Intercepters).

we already do a lot of performance improve to the SocketsHttpHandler , but the design of HttpClient is lacking us.

stephentoub commented 3 years ago

PooledConnectionLifetime is only exists on SocketsHttpHandler , and it's not a common used HttpMessageHandler

SocketsHttpHandler is the default handler used by HttpClient, since .NET Core 2.1.

it do not support "blazor webassembly"

The handler used by HttpClient under the covers in webassembly just wraps the browser's HTTP API. It will inherit whatever DNS handling is done by the browser.

a minute sound like short

I chose a random number. Make it as short as you like. :-) But DNS generally does not change that quickly.

John0King commented 3 years ago

@stephentoub

The handler used by HttpClient under the covers in webassembly just wraps the browser's HTTP API. It will inherit whatever DNS handling is done by the browser.

but will it fail when using SocketHttpClientHandler ?

and PooledConnectionLifetime do not exist on DelagteHandler and HttpClientHandler , so this is how we just that

// initialize
var shh = new SocketHttpHandler(){ PooledConnectionLifetime = TimeSpan.FromMinutes(1) }
var myDelegateHandler = new MyDelegateHandler(new MyOption{ ... }, shh); 
var httpclient = new HttpClient(myDelegateHandler )  // you should use static  filed to hold it
StaticClasss.HttpClient = httpClient;

// using
StaticClasss.HttpClient.GetJsonAsync<T>();

compare to java

HttpClientBuilder builder = HttpClientBuilder.create(); 
builder.addRequestInterceptorFirst(new MyRequestInterceptor())
var httpclient = builder.build();
httpclient.excute(xxxx);
// easy understand   you can use it per request and  it has  interceptor concept
//and I'm not a java develper,  Iet's me know if I'm wrong
stephentoub commented 3 years ago

so this is how we just that

I don't know what that myDelegateHandler is... it'd be just:

StaticClasss.HttpClient = new HttpClient(new SocketHttpHandler { PooledConnectionLifetime = TimeSpan.FromMinutes(1) });
John0King commented 3 years ago

@stephentoub

  1. does Blazor webassembly can use SocketHttpHandler
  2. do you think the current HttpClient static instance is the best ?
  3. do you consider that how library authors use the Httpclient to write there own service client ?

the reasons I'm here, because there many pain when you use HttpClient now, and HttpClientFactory try resolve that, but it also introduce their pattern into library authors.

and because that, previously "Identity Model" use there own service client and now they just use extension method on HttpClient, because there no good design pattern so solve the HttpClient problem and there no good "common" design pattern to resove HttpClientFactory registration.

the http ability is a very very very basic requirement for a programing lanague !! why after all those complain on such basic library and you still not pay much attention on this.

If I'm wrong , please give me a best principle on how library author work with HttpClient of follow example:

example 1:

  1. access different set of api.
  2. those api need some key-based authentication
  3. handle returned http error as some of Result or Exception
  4. DI is not a must have requirement

example 2:

  1. be a client that use your own server api.
  2. interceptor base authentication
  3. strong type client

In above example I facing :

  1. who own HttpClient ? (my library of caller of my library)
  2. how do I configurate the HttpClient if my "service client" do not own HttpClient
  3. how do I reslove Dns issue if I own the Httpclient
  4. how the caller of this service client resolve the Dns problem is my service client setting the PooledConnectionLifetime too longer (the caller think it is long for their case) ?
  5. should I using disposing on my service client, and if not , when I dispose the HttpClient ?
  6. HttpClientFactory with DI ? which DI Framework should I support ? and where should I put the configuration code (intercetpor like configuration) ? (DI registration or my service client)
geoffkizer commented 3 years ago

Hi John, thanks for your feedback.

does Blazor webassembly can use SocketHttpHandler

No. Blazor runs in the browser and thus is constrained to the capabilities of the browser. SocketsHttpHandler is intended for environments where you have unconstrained network access.

do you think the current HttpClient static instance is the best ?

The general guidance is: You should use the same HttpClient instance for requests to the same server/service. The exception is if you want to use different settings for different requests -- for example, different client certs -- in which case, you can use different HttpClient instances with different settings as appropriate. But you should still use the same HttpClient instance for requests that share the same settings.

Storing an HttpClient instance in a static variable is one way to achieve this, but not the only way.

An instance of an HttpClientHandler, like SocketsHttpHandler, is responsible for managing HTTP connection pools. So in general, when you use the same HttpClient instance, you are reusing connections from the same pool. If you use a different HttpClient instance, you are not reusing connections, which can have a strongly negative impact on your performance.

You should only dispose an HttpClient instance if it is no longer needed for future requests.

how do I reslove Dns issue if I own the Httpclient how the caller of this service client resolve the Dns problem is my service client setting the PooledConnectionLifetime too longer (the caller think it is long for their case) ?

I am assuming that you are talking about DNS TTLs here.

The usual reason that people care about DNS TTL is because they are doing DNS round robin for load balancing. When using DNS round robin, the DNS TTL is usually set to a small value, so that clients will adapt reasonably quickly to the change in DNS records that occurs when a server is added or removed from the round robin.

We don't currently have access to DNS TTL information, because standard APIs like getaddrinfo() do not provide TTL information.

Instead, we expose the PooledConnectionLifetime setting to allow you to choose how long an HTTP connection should continue to be used. If you set this to a value similar to your DNS TTL, you should expect a similar level of liveness in terms of connecting to the appropriate servers.

This isn't an ideal solution, but it seems to work reasonably well in practice. We have considered other ways to improve this; feedback welcome.

the http ability is a very very very basic requirement for a programing lanague !! why after all those complain on such basic library and you still not pay much attention on this.

I assure you, we care deeply about making sure people can use HttpClient effectively. I think we have not always done a good job of explaining how to do this. Suggestions welcome.

karelz commented 3 years ago

Triage: Seems to be answered above. We are looking for options to warn users about disposing HttpClient too often. DNS does not require new API and we have it on our backlog.

Closing as covered elsewhere. Thanks!

John0King commented 3 years ago

@karelz @geoffkizer I test on blazor , and it will throw exception when using SocketSocketHttpHandler, so, I don't think this problem is been resolve , or have an workaround to write a cross platform and heigh performance "Rest service client" . explain : if you try so avoid port exhaust, then we need an static httpclientHander , and if we also need avoid dns problem , then we need SocketSocketHttpHandler and PooledConnectionLifetime, and it won't work on blazor wasm !!!

and yes, maybe you can make it respect DNS changes, but what can I do if there is an urgent dns change ? restart the server should not be an acceptable solution, and there are no httpclient.DnsCache.Clear() api that I can put a "clear button" in my system.

for hundreds times in my mind , I ask me that why there isn't a static sealed httpclienthandler , and I can build a more featured HttpClient with it.

After the journey of from HttpWebRequest and to WebClient and now the HttpClient , Is there any good thing happen with the HttpClient ? from what I can see, the port exhaust start with HttpClient and the DNS problem start with HttpClient , and HttpClientFactory is a library to solve that.

IMO, we should start a new design base on the current socket code base and create a better and new HttpClient, and solve above problem from beginning.

geoffkizer commented 3 years ago

I test on blazor , and it will throw exception when using SocketSocketHttpHandler,

Yes, as has been mentioned many times above, SocketsHttpHandler does not work on Blazor. It cannot work on Blazor. Blazor runs in the browser and thus is limited to the capabilities of the browser.

explain : if you try so avoid port exhaust, then we need an static httpclientHander , and if we also need avoid dns problem , then we need SocketSocketHttpHandler and PooledConnectionLifetime, and it won't work on blazor wasm !!!

This is beyond our control. In Blazor, it's the browser that controls HTTP traffic. If you are seeing port exhaustion or other issues there, please complain to the relevant browser, not us.

and yes, maybe you can make it respect DNS changes, but what can I do if there is an urgent dns change ? restart the server should not be an acceptable solution, and there are no httpclient.DnsCache.Clear() api that I can put a "clear button" in my system.

Do you have a concrete suggestion here? This problem is not unique to us. You can set your DNS TTL to whatever you want, but there is no way in DNS to invalidate aside from TTLs. Even if we gave you an API like "DnsCache.Clear" it would simply fall back to using the DNS TTL. That's just how DNS works.

for hundreds times in my mind , I ask me that why there isn't a static sealed httpclienthandler , and I can build a more featured HttpClient with it.

I honestly don't have any idea what you want here. If you want to put your HttpClient in a static variable, go for it. That's a fine thing to do.

the port exhaust start with HttpClient

Only if you are using HttpClient incorrectly.

and the DNS problem start with HttpClient ,

Nope, the PooledConnectionLifetime setting is essentially the same as the old ServicePoint.ConnectionLeaseTimeout property.

and HttpClientFactory is a library to solve that.

It is?

IMO, we should start a new design base on the current socket code base and create a better and new HttpClient, and solve above problem from beginning.

Yeah, thanks for your opinion, but we aren't gonna do that.

We are very interested in solving customer problems and improving things for our users. We love getting useful feedback that helps us do this. The more focused and actionable your feedback is, the more likely it is to actually impact the code.

John0King commented 3 years ago

@geoffkizer 1st, Thank you for your repy. 2nd, I get your current decision : to continue improve HttpClient instead of create a new one.

but I think you don't get my point of why I mention blazor. so a little explain here:

for example : solution like this:

[service 1 : main site]
|-MyName.Web(host or full  mvc  app that call  AppService)
|-MyName.Service2.StrongClient (a library calling service2 in strong type)
------------------------------------------------------------
[service 2:  some web api]
|-MyName2.Host (an api host )
|-MyName2.AppService.Contract (app service's interface)
|-MyName2.AppService (app service implementation)
--------------------------------------------------
[clients]
|-MyName.DesktopClient (example of a client , and use MyName.Service2.StrongClient to call service2)
|-MyName.BlazorClient (  example of another client , and use MyName.Service2.StrongClient to call service2)

MyName.Service2.StrongClient may work both in the service and the client (the blazor Client), that's why I keep asking will the SocketHttpHandler work on blazor. and my question is : How to create a library that cross the service side and blazor client side (not CORS issue) ?

from your above answer , it's seems we must write different code for blazor.

I honestly don't have any idea what you want here.

I can build a rich featured httpclient with a simple fetch like api or HttpWebRequest api,
and it's also very easy to use the HttpClient if there no "port exhaust" issue. eg. (using var client = new HttpClient())

it is the "port exhaust" issue that make HttpClient hard to use, and static instance is a way to solve that.

and for what I want here is to asking a new HttpClient[v2|Light|VNext] that solve above issue and add support for interceptor pattern and the new type is to avoid breaking change.