Analyzer to warn against locally scoped HttpClient instance

[bretehlert]

From https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient?view=netcore-3.1:
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.
In .NET Core they have also introduced IHttpClientFactory: https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests

This is easy to forget as it goes against the normal best practice of creating and disposing an instance of a class when you need it.

Create a analyzer rule to warn against creating an HttpClient instance scoped to a single function or class instance.

[yaakov-h]

The docs are only partially accurate there.

The real problem comes from not reusing a HttpClientHandler. It just so happens that the default constructor for HttpClient will instantiate and own a HttpClientHandler.

My initial thinking is along these lines:

1. Warn if a HttpClientHandler is constructed and its lifetime does not exceed that of the enclosing function. This can happen when developers need to customize some features such as setting a proxy or cookie container, IIRC, before passing it to HttpClient's constructor. In this case we need to also examine the lifetime of the object it is passed to, and any object that is passed to etc. in the case of nested DelegatingHandler objects.
2. Warn if a HttpClient is constructed using its default constructor, and its lifetime does not exceed that of the enclosing function.

There are probably some special cases that we could ignore (such as if the enclosing function is the program's Main entrypoint), but it may just be simpler to suppress the warning using SuppressWarningAttribute in places where the use is valid, as having the suppression will educate readers that this is not a generally acceptable approach, rather it is a valid exception.

(I just hope people don't follow the common behavior of disabling the rule or suppressing the warning in cases where the warning is valid).

[bretehlert]

Can you please provide a reference to back your assertion that it is really the HttpClientHandler that is the problem and not the HttpClient itself. The normal .NET documentation does not explain this at all, it only says you should use a single HttpClient instance.

[yaakov-h]

The documentation kind of glosses over the fact that HttpClient is actually a series of pieces, and is probably simplified for the average developer that doesn't need to know how it works internally.

If you look at the implementation of HttpClient (despite having grown a bit in recent .NET versions), it is essentially a set of utility methods that all revolve around base.SendAsync(...), which internally is a wrapper around handler.SendAsync(...).

The actual connection and socket management, on all .NET platforms and runtimes that I've looked at, do it in HttpClientHandler. That has now shifted again into further sub-components - on .NET 5/Core there is a managed implementation, SocketsHttpHandler, whereas on Framework its a fairly thin wrapper around WinHTTP (which you can still use on modern runtimes with WinHttpHandler if you so desire).

Since the HttpClientHandler and its components are managing the resources that can be exhausted, that is where the problem lies.

To further validate this, .NET's own IHttpClientFactory implementation follows this same approach. It caches the handlers, but each request for a HttpClient returns a new HttpClient that wraps the existing handler.