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

Is there will be a truely IDisposable HttpClient #27601

Closed
John0King opened this issue Oct 11, 2018 · 13 comments
Closed

Is there will be a truely IDisposable HttpClient #27601

John0King opened this issue Oct 11, 2018 · 13 comments
Labels
area-System.Net.Http question Answer questions and provide assistance, not an issue with source code or documentation.
Milestone

Comments

@John0King
Copy link

I know there some good thing happen on current HttpClient implementation and the new HttpClientFactory.
but the HttpClientFactory is not a good choice for "lib" development . and I'm not sure about the dispose behavior of SocketsHttpHandler .
Will there be a new HttpClient ( maybe called HttpClientCng or HttpClient2 ) that design to work with using keyword?

using(var client = new HttpClient2())
{
   ...
}
@stephentoub
Copy link
Member

What problem are you hoping to see solved?

@John0King
Copy link
Author

@stephentoub the current HttpClient implement IDisposable but shouldn't work with using keyword.

using(var client = new HttpClient())
{
   //will facing probem in height concurrent  :  number of TCP connections drain .
   // is this problem solved by SocketsHttpHandler ?
}

and that's one reason of why we should use HttpClientFactory, and the HttpClientFactory is not good for "library" developer.

before using HttpClient , the HttpWebRequest seems not have this kind issue.
this why I hoping there be a truely IDisposable HttpClient.

@John0King
Copy link
Author

Can we solve those annoying concurrent problem once for all ? Or there's a guidance for solve those problem?

@stephentoub
Copy link
Member

HttpClient is meant to be stored and reused over and over; it also offers Dispose in order to give you explicit control over when its resources/connections are torn down, whereas you don't have such explicit control with WebRequest, where the underlying pool isn't exposed.

What are you trying to do in your library where this doesn't work for you?

@karelz
Copy link
Member

karelz commented Oct 22, 2018

@John0King the guidance is to reuse HttpClient for perf reasons (to avoids establishing new connection for each request and to avoid port exhaution).
If you are worried about DNS changes, you may want to use SocketsHttpHandler.PooledConnectionLifetime (https://github.com/dotnet/corefx/issues/11224#issuecomment-394184095) or HttpClientFactory or recycle your 'static' instance on regular basis (https://github.com/dotnet/corefx/issues/11224#issuecomment-415850524), which is what HttpClientFactory does.

@John0King
Copy link
Author

Why we can't keep the pool and the poolManager as a global pool ? then those annoying concurrent problem will gone once for all .
Beside , the benefit of baseurl and defaut header will gone if use a static instance of HttpClient.
That's why ask can we have a new HttpClient2 if the current HttpClient implement can not be change .

@stephentoub
Copy link
Member

stephentoub commented Oct 23, 2018

Why we can't keep the pool and the poolManager as a global pool ?

You can. Create a static field to store it.

Beside , the benefit of baseurl and defaut header will gone if use a static instance of HttpClient.

You can have any number of HttpClient instances that are all sitting on top of the same HttpClientHandler, e.g.

private static readonly HttpClientHandler s_handler = new HttpClientHandler();
...
using (var hc = new HttpClient(s_handler, disposeHandler: false))
{
    hc.BaseAddress = ...;
    hc.DefaultRequestHeaders.Add(...);
    ... // make whatever requests here
} // the handler won't be disposed here, and it's actually what owns all the resources / the pool

@John0King
Copy link
Author

John0King commented Oct 23, 2018

@stephentoub I wonder if you have 20 lib that access differenct network resouce (each lib have a configuration like this, but your app do not call those method) , Will there be more than 20 port in used but do nothing?
And what about CookieContainer and Certificates

if there is a new HttpClient2 (eg. use a globalPool),

class HttpClient2:IDisposable
{
      // add current HttpClient properties
      // add current HttpClientHandler properties
      public TimeSpan PooledConnectionTimeout { get; set;}
      public static void ResetPooledConnectionLifetime()  // reset dns of pool 
      
      // lifetime hook event
     public Task BeforeSend(Action<HttpClient2,HttpRequestMessage> handler)
     public Task AfterSend(Action<HttpClient2,HttpResponseMessage> handler)
     public Task WhenException(Action<HttpClient2,Exception> handler)
     
    // this is for retry
    public Task<HttpResponseMessage> SendWithoutLifetimeHookAsync(HttpRequestMessage request,CancellationToken cancellationToken)
   //
 }

and this can be a just a .net standar lib

@stephentoub
Copy link
Member

The connections in a pool don't stick around forever; they're cleaned up after a period of inactivity. And if you use SocketsHttpHandler directly (instead of HttpClientHandler, which wraps it), you have control over things like the idle timeout for connections, total lifetime of a connection, etc. Further, configuration like Certificates is directly related to those connections, and you absolutely need to have different connections for different certificates in use.

@John0King
Copy link
Author

So,What's your point ?

  • Suggestion lib develper to use static field and likely with a manager
  • Force lib developer use DI for get HttpClient (use HttpClientFactory)
  • Continue optimize current HttpClient
  • Or may consider add a new HttpClient future

@stephentoub
Copy link
Member

So,What's your point ?

To use HttpClient, either via HttpClientFactory, or via an instance stored for reuse (either the HttpClient itself or if necessary the underlying handler with new HttpClient instances created around that shared handler).

@John0King
Copy link
Author

IMO, there must be a Ioc Container to use HttpClientFactory which is not good for a lib developer who only want to access network. and for second choice , SocketHttpHandler only work in .net core 2.1+ .
so I think a pooled HttpClient will be a very good choice for lib developer .
Advantage:

  • very easy to use without worry height concurrent problem
  • IDisposable behavior match the suggestion eg. using(IDisposable)
  • release as a nuget package that let .net 4.5+ can consume (the internal impliment can use SocketHttpHandler)

Just curious, Is the HttpWebRequest is just like this pooled HttpClient that just with legacy api ?

@karelz
Copy link
Member

karelz commented Oct 23, 2018

HttpWebRequest is compat-only API. It is built on top of HttpClient in .NET Core. It is rather inefficient and does not support all knobs on ServicePoint. We do not recommend anyone to use it - it is not where we plan to invest, unless we have to (i.e. large number of customers hitting problems with it without ability to migrate).

Overall your original question seems to be answered, so closing.

@karelz karelz closed this as completed Oct 23, 2018
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

No branches or pull requests

4 participants