Warning about incorrect lifecycle policy #2315
Conversation
xml/System.Net.Http/HttpClient.xml
Outdated
| @@ -28,6 +28,10 @@ | |||
| <format type="text/markdown"><![CDATA[ | |||
|
|
|||
| ## Remarks | |||
|
|
|||
| > [!WARNING] | |||
| > <xref:System.Net.Http.HttpClient> is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors. | |||
There was a problem hiding this comment.
Thanks @jzabroski. Unfortunately, this recommendation is not complete.
We need to warn users also about having long-existing HttpClient instances as that may lead to stale DNS usage. That can lead to non-functional connections.
The recommendation should be either to:
- Use
HttpClientinstances as statics, which are recycled on regular basis, or - Use
HttpClientFactorwhich works around this problem, or - Use static instances on .NET Core while setting
System.Net.Http.SocketsHttpHandler.PooledConnectionLifetime.
xml/System.Net.Http/HttpClient.xml
Outdated
| @@ -56,7 +60,7 @@ | |||
|
|
|||
| 9. <xref:System.Net.Http.HttpClient.SendAsync%2A> | |||
|
|
|||
| <xref:System.Net.Http.HttpClient> is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors. Below is an example using HttpClient correctly. | |||
| Below is an example using HttpClient correctly: | |||
There was a problem hiding this comment.
The below example is technically also incomplete and needs update
There was a problem hiding this comment.
Yes, I agree. I also dislike how the Examples section comes before the Remarks, but I figured we should tackle one issue at a time. I see you are a developer rather than a tech writer, so your mindset is probably more of a developer than a tech writer (if I may suggest).
Would you rather we re-work the whole article in one go or do this piece-wise?
There was a problem hiding this comment.
Yes, I am developer and have very little tech writer mindset - but I know people who can help with that 😄 - paging @mairaw @rpetrusha
Rather than piece-wise, I would prefer overall re-work to accomodate to all needs. (unless there are reasons against I am not aware of)
BTW: This is one of the key topics on our backlog - it was brought up in MVP session feedback we had in March at MVP Summit. I just didn't get to publishing the full list of feedback points as our detailed plan/roadmap.
There was a problem hiding this comment.
I would be happy to do the rework. Give me about 1-2 weeks. I'll fit it in as I have free time; would rather underpromise and over-deliver.
There was a problem hiding this comment.
Thanks so much. It will be great help!
There was a problem hiding this comment.
Yeah, that sounds like a great link
There was a problem hiding this comment.
@karelz What are your thoughts about adding some examples of using Polly and https://www.nuget.org/packages/Polly.Extensions.Http/ nuget PackageReference to a project. These are "opinionated" libraries to assist in transient fault handling.
Polly project is a member of the .NET Foundation https://github.com/App-vNext/Polly.Extensions.Http
It is basically the modern equivalent of Microsoft Enterprise Library Transient Fault Handling Application Block (last updated in 2013). Enterprise Library logic has about 4M nuget downloads, whereas Polly has 12M.
There was a problem hiding this comment.
TODO: Consider adding link to Polly Nuget Package on https://docs.microsoft.com/en-us/dotnet/standard/microservices-architecture/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests#issues-with-the-original-httpclient-class-available-in-net-core as part of this PR. The phrase Polly is used throughout this page without defining what it is. e.g, "resilient HTTP calls by integrating Polly with it."
I also think the "Issues with using HttpClient" section on that page can be tidied up into a list of numbered points and the whole "As a first issue," and "But there’s a second issue" language can be consolidated into markdown numbered points.
It should read like something like this:
HttpClientis intended to be instantiated once and reused throughout the life of an application. DespiteHttpClientimplementingIDisposable, constantly constructing and disposing ofHttpClientinstances can cause serious system-wide issues.<insert explanation of socket exhaustion here />- However, a singleton or static
HttpClientdoesn't intelligently handle transient faults such as DNS changes (which can occur on failover), as explained in this issue at the .NET Core GitHub repo.
Blah blah please use HttpClientFactory with Polly.
There was a problem hiding this comment.
I don't think we should add links to packages which are not part of the platform itself. Even if they are .NET Foundation projects.
I can be convinced if there is precedent in .NET docs (@mairaw?) - in which case I expect deeper ecosystem scouting should happen first, so that we do not promote just one nuget packge we happen to know about ...
There was a problem hiding this comment.
@karelz I thought this over, and I think you're right, ".NET Foundation" by itself is not enough to warrant inclusion in any Microsoft Docs project. At minimum, the project should ALSO support strong named assemblies in a sensible manner.
The lack of (sensible) strong named assemblies in Json.NET is exactly what has caused hundreds if not thousands of issues for Microsoft-employed developers and the outstanding community, mostly ASP.NET developer community.
You can read about it here: http://james.newtonking.com/archive/2012/04/04/json-net-strong-naming-and-assembly-version-numbers and JamesNK/Newtonsoft.Json#1001 where you can see that the Azure SDK team was bit by strong naming policy: Azure/azure-sdk-for-net#4380
Anyway, this is just some thoughts to share with you - it's a tangent but the general problem of AutoGenerateBindingRedirects and loading the right assembly has been something I've been on a war path for the last 6+ months.
rpetrusha
added
the
✨ 1st-time dotnet-api-docs contributor!
There was a problem hiding this comment.
Thanks, @jzabroski, for contributing to the dotnet/docs repo and making the guidance for the use of HttpClient more prominent. I've suggested that SocketException be made a link, but @karelz has added some more substantial comments for you to consider.
Co-Authored-By: jzabroski <[email protected]>
|
Thank you so much, @jzabroski. We appreciate your contribution, so take the time you need. When you've made the updates, just add the changes-addressed label so that we know your PR is ready for a new review. |
|
I guess my point is the web page on handling DNS faults already mentions
Polly as a proper noun without explaining what it is. I saw Steve Sanderson
created his own Retry shim which to me suggests very senior expert level
engineers may not think Polly is all its crackered up to be 😛
…On Mon, Apr 22, 2019, 5:43 PM Karel Zikmund ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In xml/System.Net.Http/HttpClient.xml
<#2315 (comment)>
:
> @@ -56,7 +60,7 @@
9. <xref:System.Net.Http.HttpClient.SendAsync%2A>
- <xref:System.Net.Http.HttpClient> is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors. Below is an example using HttpClient correctly.
+Below is an example using HttpClient correctly:
I don't think we should add links to packages which are not part of the
platform itself. Even if they are .NET Foundation projects.
I can be convinced if there is precedent in .NET docs ***@***.***
<https://github.com/mairaw>?) - in which case I expect deeper ecosystem
scouting should happen first, so that we do not promote just one nuget
packge we happen to know about ...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2315 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADNH7MHMWGIURKZMPOUYXDPRYWORANCNFSM4HGJWUKQ>
.
|
|
If it is already mentioned from MS docs, then that is different story :) ... is that the case? |
|
@jzabroski Do you still plan to implement the recommended changes in this PR? |
tdykstra
added
the
waiting-on-feedback
|
@tdykstra Yes. I was thinking about exactly this topic this weekend. |
tdykstra
added
doc-enhancement
|
@jzabroski Do you still plan to implement the recommended changes in this PR? |
tdykstra
added
needs-author-action
|
Yes.
…On Mon, Feb 3, 2020, 3:45 PM Tom Dykstra ***@***.***> wrote:
@jzabroski <https://github.com/jzabroski> Do you still plan to implement
the recommended changes in this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2315?email_source=notifications&email_token=AADNH7OY3PNEOFKLRZGS6V3RBCF7LA5CNFSM4HGJWUK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKVQN7Y#issuecomment-581633791>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADNH7JR7XPDKJFFX4K5QKDRBCF7LANCNFSM4HGJWUKQ>
.
|
|
Will wrap this up this weekend.
…On Mon, Feb 3, 2020 at 5:34 PM John Zabroski ***@***.***> wrote:
Yes.
On Mon, Feb 3, 2020, 3:45 PM Tom Dykstra ***@***.***> wrote:
> @jzabroski <https://github.com/jzabroski> Do you still plan to implement
> the recommended changes in this PR?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#2315?email_source=notifications&email_token=AADNH7OY3PNEOFKLRZGS6V3RBCF7LA5CNFSM4HGJWUK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKVQN7Y#issuecomment-581633791>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AADNH7JR7XPDKJFFX4K5QKDRBCF7LANCNFSM4HGJWUKQ>
> .
>
|
Items to improve
|
|
@jzabroski will you have a chance to continue working on this PR? |
|
I created this issue to track the remaining work: https://github.com/dotnet/dotnet-api-docs/issues/4786 |
|
@karelz You had requested changes on this PR. Can you re-review it to see if it can be merged in its current state, with the other improvement items tracked by #4786? |
|
Ping @dotnet/ncl - Can someone in the networking team take a look at this PR? |
There was a problem hiding this comment.
Thanks @jzabroski!
We've learned more about HttpClientFactory and we should reword its part and not recommend it as option when HttpClient is used directly. It is suited only for DI environments. And HttpClient can be created per request when it is created by HttpClientFactory.
| > - Use <xref:System.Net.Http.HttpClientFactoryExtensions.CreateClient(IHttpClientFactory)> to create the <xref:System.Net.Http.HttpClient>. | ||
| > - On .NET Core and .NET 5.0 and later, use static instances and set <xref:System.Net.Http.SocketsHttpHandler.PooledConnectionLifetime>. |
There was a problem hiding this comment.
| > - Use <xref:System.Net.Http.HttpClientFactoryExtensions.CreateClient(IHttpClientFactory)> to create the <xref:System.Net.Http.HttpClient>. | |
| > - On .NET Core and .NET 5.0 and later, use static instances and set <xref:System.Net.Http.SocketsHttpHandler.PooledConnectionLifetime>. | |
| > - On .NET Core 2.1 and later, and .NET 5.0 and later, use static instances and set <xref:System.Net.Http.SocketsHttpHandler.PooledConnectionLifetime>. |
| @@ -28,6 +28,14 @@ | |||
| <format type="text/markdown"><![CDATA[ | |||
|
|
|||
| ## Remarks | |||
|
|
|||
| > [!WARNING] | |||
| > - <xref:System.Net.Http.HttpClient> is intended to be instantiated once and re-used throughout the life of an application. Instantiating an <xref:System.Net.Http.HttpClient> class for every request will exhaust the number of sockets available under heavy loads and result in a <xref:System.Net.Sockets.SocketException>. However, having long-lived <xref:System.Net.Http.HttpClient> instances can lead to stale DNS usage and non-functional connections. Use one of the following options to avoid these problems: | |||
There was a problem hiding this comment.
| > - <xref:System.Net.Http.HttpClient> is intended to be instantiated once and re-used throughout the life of an application. Instantiating an <xref:System.Net.Http.HttpClient> class for every request will exhaust the number of sockets available under heavy loads and result in a <xref:System.Net.Sockets.SocketException>. However, having long-lived <xref:System.Net.Http.HttpClient> instances can lead to stale DNS usage and non-functional connections. Use one of the following options to avoid these problems: | |
| > - <xref:System.Net.Http.HttpClient> is intended to be instantiated once and re-used throughout the life of an application, unless it was created by `HttpClientFactory`. Instantiating an <xref:System.Net.Http.HttpClient> class directly for every request will exhaust the number of sockets available under heavy loads and result in a <xref:System.Net.Sockets.SocketException>. However, having long-lived <xref:System.Net.Http.HttpClient> instances can lead to stale DNS usage and non-functional connections. Use one of the following options to avoid these problems: |
|
I'm ok with the changes proposed by @karelz, I can approve the PR as soon as the changes are commited. |
|
I'll make them today |
| @@ -28,6 +28,14 @@ | |||
| <format type="text/markdown"><![CDATA[ | |||
|
|
|||
| ## Remarks | |||
|
|
|||
| > [!WARNING] | |||
| > - <xref:System.Net.Http.HttpClient> is intended to be instantiated once and re-used throughout the life of an application. Instantiating an <xref:System.Net.Http.HttpClient> class for every request will exhaust the number of sockets available under heavy loads and result in a <xref:System.Net.Sockets.SocketException>. However, having long-lived <xref:System.Net.Http.HttpClient> instances can lead to stale DNS usage and non-functional connections. Use one of the following options to avoid these problems: | |||
There was a problem hiding this comment.
The problem with saying "instantiated once" is that it makes users think they literally should not create more than one of these, which is incorrect.
An instance of HttpClient basically corresponds to a connection pool. So you want to reuse the same HttpClient instance because it will reuse connections from that pool. But there are cases where you don't care about sharing/reusing connections, e.g. when you are talking to two different origin servers and thus wouldn't be sharing connections anyway. And if you want to have, e.g. different authentication settings or redirect settings or version policy or whatever, it it totally fine and, in fact, advisable to have a different HttpClient instance for each of these.
There was a problem hiding this comment.
@karelz @geoffkizer I'm sorry this fell by the wayside. The PR comments coincided with me getting covid last year, and the recovery/brain fog limited my mental capacity for "a good bit", as we say in the south.
We've learned more about
HttpClientFactoryand we should reword its part and not recommend it as option whenHttpClientis used directly. It is suited only for DI environments.
-- @karelz
This actually doesn't make total sense to me. Can you explain why?
An instance of HttpClient basically corresponds to a connection pool. So you want to reuse the same HttpClient instance because it will reuse connections from that pool.
-- @geoffkizer
That's all fine, except that "basically" is vague, too.
IHttpClientBuilder defines an explicit lifetime for re-use, here:
IHttpClientBuilder also mentions that the HttpMessageHandler is what gets pooled, so perhaps you mean to say "HttpClient provides a connection pool for HttpMessageHandler instances."
Here is another area that likely need improving:
ITypedHttpClientFactory confusingly doesn't use DI but relies on a static cache - and thus should not be confused with IHttpClientFactory
This is a really bizarre remark to me. Just reading it sounded incorrect. It reads to the cautionary reader that there is a lifetime scope mismatch, when you think about it!
So, I decided to dig deeper and found THIS which confirmed my worry - it's a cache that has no fault tolerance mechanisms and is basically a memory leak:
ITypedHttpClientFactory has a reference to an undefined variable.
_exampleClient is undefined
HttpClientFactoryExtensions - IHttpClientFactory has a "default configuration" 👀 ?
This also doesn't read well - how can an interface have a "default configuration" 👀 ?
There was a problem hiding this comment.
We've learned more about HttpClientFactory and we should reword its part and not recommend it as option when HttpClient is used directly. It is suited only for DI environments.
This actually doesn't make total sense to me. Can you explain why?
Not sure what is not clear. It is statement. HttpClientFactory is designed for DI system. It is cumbersome to use it outside of DI environment.
There was a problem hiding this comment.
Let me rephrase:
If the article Use IHttpClientFactory to implement resilient HTTP requests is correct guidance, and not using HttpClientFactory leads to non-resilient HTTP Requests, why would DI vs. non-DI be the controlling factor for its use?
I think saying it is only suited for DI environments is rather confusing in light of the problems highlighted with HttpClient. But perhaps HttpClient has been refactored in newer versions of .NET 5/6 and this no longer applies? See specifically this part of the article which addresses HttpClient shortcomings: https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests#issues-with-the-original-httpclient-class-available-in-net
There was a problem hiding this comment.
Actually, I think I see the problem, further down in that same article I linked, there is a caution about HttpClientFactory being somewhat tightly coupled to Microsoft.Extensions.DependencyInjection nuget package, with a GitHub discussion linked for more info: dotnet/aspnetcore#28385
There was a problem hiding this comment.
I think, if you scroll down to the bottom of the aforementioned GitHub discussion, you will see a comment by an end user that is fairly lucid (unfortunately, Eric Sampson then decided to close the thread because I guess it was too flamboyant): dotnet/aspnetcore#28385 (comment)
In particular, "Example 4: You're creating a redistributable web-service client library class:" points out that "Figuring out how to handle HttpClient when you don't own the entrypoint or host program that will run your code is very difficult."
Separtely, Eric's reply - go look at the Azure SDK for .NET - is not good. Not only was there no direct link to the code, but Guess what, I followed his suggestion and found production code commented out in ServiceClient.cs - how is this the example Microsoft wants us to follow? :)
and a TODO
...and, it looks like the actual pattern for using ServiceClient.cs is to rely on some code generation, and I can't seem to figure out where that code generator lives, but I can see the checked-in artifacts:
- https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingVisualSearch/src/Generated/VisualSearchClient.cs
- https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/hdinsight/Microsoft.Azure.HDInsight.Job/src/Generated/HDInsightJobClient.cs
- https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingNewsSearch/src/Generated/NewsSearch/NewsSearchClient.cs
- https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingImageSearch/src/Generated/ImageSearch/ImageSearchClient.cs
- https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingEntitySearch/src/Generated/EntitySearch/EntitySearchClient.cs
- https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingVideoSearch/src/Generated/VideoSearch/VideoSearchClient.cs
- etc... there's got to be tens if not hundreds of these auto-generated clients.
Point is: The "if you want to see how someone has handled these issues, you can take a look at the Azure SDK. IIRC they've dealt with this" guidance is not good.
I think this is what Joshua Bloch would call the empathy gene importance in good API design. https://www.youtube.com/watch?v=aAb7hSCtvGw
There was a problem hiding this comment.
I filed a ticket with the Azure SDK team and they elaborated on what their solution to Jehoel's Scenario 4 is, where you are a API author providing API services to a client. See: Azure/azure-sdk-for-net#29035 (comment)
|
@jzabroski do you plan to take the suggested changes above? |
|
I think this PR was superseded by #3986 and is no longer needed |
|
@MSDN-WhiteKnight I don't think it directly addresses it, actually. I'll need to carefully re-read the article now but it seems neither my initial PR nor Karel comments / idea to improve the documentation actually teach people "how to fish" but rather seems more focused on documentation of footguns that were mysteriously omitted |
ghost
removed
the
needs-author-action
|
Hi @jzabroski, Could you please
I'm closing this PR as what we were trying to update in this PR was already addressed by #8098 and dotnet/docs#29568 |
|
Agreed, thank you. |
Summary
In several places I've worked, people use
HttpClientincorrectly. While some of this can be chalked up to failure to "Read the ... Documentation", I also think the documentation could make use of WARNING Note Blocks to call-out the best way to use HttpClient.Even code bases I've looked at (using Reflector) written by quality software vendors frequently have many HttpClient objects created. See also this Reddit thread today where someone was hit by the same issue: https://www.reddit.com/r/csharp/comments/bdemhl/why_have_we_been_programmed_to_dispose_of_things/
While I can appreciate developers should read the whole remarks, this is probably the most frequent error I encounter with people using HttpClient. As such, I think it should be prominently displayed.