Potential for Port Exhaustion
lrhoads opened this issue · 7 comments
This was discovered in using RestSharp in .Net Core 2.2, which utilizes HttpWebRequest and instantiates a new HttpWebRequest per instance of RestClient.
With the change in .Net Core to wrap HttpClient, this is allowing the potential for port exhaustion. I feel like this implementation should be more responsible and use a static or perhaps a factory that could be overridden if needed.
It's surprising that dotnet core is using HttpClient in this way. There are Microsoft articles detailing this pitfall.
From Issues with the original HttpClient class available in .NET Core
The original and well-known HttpClient class can be easily used, but in some cases, it isn't being properly used by many developers.
As a first issue, while this class is disposable, using it with the using statement is not the best choice because even when you dispose HttpClient object, the underlying socket is not immediately released and can cause a serious issue named ‘sockets exhaustion’. For more information about this issue, see You're using HttpClient wrong and it's destabilizing your software blog post.
We recently updated HttpWebRequest in the common path to have improved perf in .NET 5, so this may be resolved for you.
However, I will note that HttpWebRequest currently exists for backward-compatibility only -- we are only investing minimally in it on a case by case basis. We encourage migrating to HttpClient to use the latest features and for maximum performance.
While I do understand that it is only used for backward compatibility, this issue is not something that the user of a 3rd party library can fix on their own. In addition, it makes for a rather difficult to diagnose problem when using a library that is suffering from a bait and switch on HttpWebRequest.
I will be opening an issue on the RestSharp project, as it would their responsibility to change their product but still feels like a minor change to make here. As is, this introduces a problem that was not present in HttpWebRequest prior.
While it is reasonable to ask people to change their usage, it also seems an easy win to at least change this into a static usage of HttpClient.
still feels like a minor change to make here
What do you believe is that "minor change"?
Given the network limitations of just using a static, and the DI requirements of IHttpClientFactory, seems reasonable that a short-term middle approach of a static with thread-safe expiration/replacement would be fairly straightforward change.
@lrhoads, the problem is in the mismatch of how settings are applied between the two models. With HttpWebRequest, all of the configuration happens on that instance, configured for each request. For HttpClient/HttpClientHandler/SocketsHttpHandler, the configuration is done on the client/handler, for all requests that get sent through it. You suggest creating a static singleton to be shared by the individual HttpWebRequest instances, and that's exactly what dotnet/corefx#41462 did, which is what @scalablecory was referring to when he said "we recently updated HttpWebRequest in the common path to have improved perf in .NET 5". This only applies to a specific set of properties values, though; if you veer off that, you end up still getting a new HttpClient instance for those other requests. We're not planning to do more than that at this time.
@stephentoub, Looking at the changes in that PR, I would agree that the implementation does address the issue very nicely.
Sorry, I didn't see that change sooner based on the reference to .NET 5 or I would have acquiesced sooner. :) Thanks for the feedback.