Skip to content

Warning about incorrect lifecycle policy #2315

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Warning about incorrect lifecycle policy #2315

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
276f939
Warning about incorrect lifecycle policy
jzabroski Apr 16, 2019
2779740
Update xml/System.Net.Http/HttpClient.xml
Apr 16, 2019
fad9cca
Apply suggestions from code review
gewarren Aug 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions xml/System.Net.Http/HttpClient.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> - <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:

Copy link
Contributor

@geoffkizer geoffkizer Mar 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 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.
-- @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:

https://github.com/dotnet/runtime/blob/215b39abf947da7a40b0cb137eab4bceb24ad3e3/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientBuilderExtensions.cs#L504-L518

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

https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/Microsoft.Extensions.Http/src/ITypedHttpClientFactory.cs#L31-L32

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:

https://github.com/dotnet/runtime/blob/46871b8541fbc2d2f3ff27207597b3a38792a010/src/libraries/Microsoft.Extensions.Http/src/DefaultTypedHttpClientFactory.cs#L33-L36

ITypedHttpClientFactory has a reference to an undefined variable.

https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/Microsoft.Extensions.Http/src/ITypedHttpClientFactory.cs#L71

_exampleClient is undefined

HttpClientFactoryExtensions - IHttpClientFactory has a "default configuration" 👀 ?

This also doesn't read well - how can an interface have a "default configuration" 👀 ?

https://github.com/dotnet/runtime/blob/215b39abf947da7a40b0cb137eab4bceb24ad3e3/src/libraries/Microsoft.Extensions.Http/src/HttpClientFactoryExtensions.cs#L14

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@jzabroski jzabroski May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/mgmtcommon/ClientRuntime/ClientRuntime/ServiceClient.cs#L33

https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/mgmtcommon/ClientRuntime/ClientRuntime/ServiceClient.cs#L89

https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/mgmtcommon/ClientRuntime/ClientRuntime/ServiceClient.cs#L126-L179

https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/mgmtcommon/ClientRuntime/ClientRuntime/ServiceClient.cs#L584-L600

and a TODO

https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/mgmtcommon/ClientRuntime/ClientRuntime/ServiceClient.cs#L506

...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:

  1. https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingVisualSearch/src/Generated/VisualSearchClient.cs
  2. https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/hdinsight/Microsoft.Azure.HDInsight.Job/src/Generated/HDInsightJobClient.cs
  3. https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingNewsSearch/src/Generated/NewsSearch/NewsSearchClient.cs
  4. https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingImageSearch/src/Generated/ImageSearch/ImageSearchClient.cs
  5. https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingEntitySearch/src/Generated/EntitySearch/EntitySearchClient.cs
  6. https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/cognitiveservices/Search.BingVideoSearch/src/Generated/VideoSearch/VideoSearchClient.cs
  7. 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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

>
> - Use static <xref:System.Net.Http.HttpClient> instances, which are recycled on a regular basis.
> - 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>.
Comment on lines +36 to +37
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> - 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>.


The <xref:System.Net.Http.HttpClient> class instance acts as a session to send HTTP requests. An <xref:System.Net.Http.HttpClient> instance is a collection of settings applied to all requests executed by that instance. In addition, every <xref:System.Net.Http.HttpClient> instance uses its own connection pool, isolating its requests from requests executed by other <xref:System.Net.Http.HttpClient> instances.

The <xref:System.Net.Http.HttpClient> also acts as a base class for more specific HTTP clients. An example would be a FacebookHttpClient providing additional methods specific to a Facebook web service (a GetFriends method, for instance). Derived classes should not override the virtual methods on the class. Instead, use a constructor overload that accepts <xref:System.Net.Http.HttpMessageHandler> to configure any pre- or post-request processing instead.
Expand Down Expand Up @@ -56,7 +64,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.
The following code snippet shows how to use <xref:System.Net.Http.HttpClient>:

```csharp
public class GoodController : ApiController
Expand Down Expand Up @@ -2152,4 +2160,4 @@ public class GoodController : ApiController
</Docs>
</Member>
</Members>
</Type>
</Type>