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 1 commit
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
8 changes: 6 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,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.
Copy link
Member

Choose a reason for hiding this comment

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

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 HttpClient instances as statics, which are recycled on regular basis, or
  • Use HttpClientFactor which works around this problem, or
  • Use static instances on .NET Core while setting 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 +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:
Copy link
Member

Choose a reason for hiding this comment

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

The below example is technically also incomplete and needs update

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@karelz karelz Apr 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks so much. It will be great help!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that sounds like a great link

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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:

  1. HttpClient is intended to be instantiated once and reused throughout the life of an application. Despite HttpClient implementing IDisposable, constantly constructing and disposing of HttpClient instances can cause serious system-wide issues. <insert explanation of socket exhaustion here />
  2. However, a singleton or static HttpClient doesn'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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@jzabroski jzabroski Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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


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