[csharp][netcore-httpclient] Reuse HttpClient, Allow use of external HttpClient, Fix Socket Exhaustion, Alternative With Constructor Injection

[Blackclaws]

Fixed an existing issue that made the HttpClient open new sockets for each connection even if being reused as the UserAgent was being set on the Client and not the request. Also makes HttpClient settable from external.
Fixes: #9060

Alternative to #9080

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh
  ./bin/utils/export_docs_generators.sh
  

  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master, 5.1.x, 6.0.x
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@mandrean (2017/08) @frankyjuang (2019/09) @shibayan (2020/02)

[wing328]

From the code sample in the readme:

HttpClientHandler yourHandler = new HttpClientHandler();
HttpClient yourHttpClient = new HttpClient(yourHandler);

Looks like if the HttpClient is provided, one can obtain its HttpClientHandler, right?

If that's the case, I would suggest simply ask for HttpClient in the constructor to make it easier for the user.

[Blackclaws]

That is unfortunately not the case. You can not acquire the handler from the HttpClient once it is constructed. The handler is used for setting/retrieving cookies as well as configuring the proxy and client certificates. We could say that in case a user passed in their own HttpClient that they would be handling all of that themselves or simply have to live with the fact that it doesn't work. Hence why I was wondering whether we should at least give some kind of warning.

[wing328]

Just saw this: https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient?view=net-5.0#httpclient-and-net-core

Shall we simply use HttpClient = new HttpClient(); as it should pick the best available transport based on the platform? In other words, ApiClient will not store the HttpClientHandler.

[Blackclaws]

Again, we are no longer able to set proxy options and client certificates anymore if we do that. Instantiating HttpClientHandler creates an underlying actual handler which is SocketsHttpHandler.

[Blackclaws]

Just to be clear, what we are doing by default is exactly what HttpClient and its default constructor are doing:

        public HttpClient()
            : this(new HttpClientHandler())
        {
        }

[lucamazzanti]

I also thinked about the validation of both parameters as you.
But actually i think that I won't to get a complex DI for these few not so useful parameters.
I test it in a real net core application.
I see more things:

• The disposable client now must be manage in the API template.
• The api must have a better ctor to inject the client, the double interface is problematic to manage in the DI
  What do you think will be the usage, the client is prepared that way for example:

 services.AddHttpClient<IProductClient, ProductClient>()
 .SetHandlerLifetime(TimeSpan.FromMinutes(5))
.AddTransientHttpErrorPolicy(p => p.RetryAsync(3))
.AddTransientHttpErrorPolicy(p => p.CircuitBreakerAsync(5, TimeSpan.FromSeconds(30)));

Now i need to register the Api passing some related options like the basePath or configuration and the client or the api client.

[Blackclaws]

Yeah you need customize the DI injection by running a lambda function that then sets the correct configuration at that point.

Right now adding just the HttpClient is not a supported UseCase. If you say this is relevant for you as well, and you want to use the client from IHttpClientFactory then we'll figure out a way to do that.

I talked about this also with @wing328 and we actually wanted to add a way to do this later if people request it. It appears you may have a usecase already?

[Blackclaws]

I've added a new version that includes a switch to disable the exception and the features that don't work if handler isn't present.

[lucamazzanti]

I don't understand why in those 3+ years of net core, no one had this issue using a swagger codegen.
Maybe because in the official startup documentation they promote NSwag, effectively the only one actually support the injection of an HttpClient. They also underline very well the issues\tradeoffs in the HttpClient and how they manage it with the factory in the DI.

We moved to net core with a few projects in this years, those services consume others using rest api, with a Swagger codgen. In this days I monitored a strange pressure on the production environment, and I found the socket exsaustion, done by ourself with a chatty client. I compared the different behavior of the vanilla codegen in both framework, monitoring with wireshark:

• one uses 1 socket, 1 TCP handshake, 1 TLS handshale, N calls
• the other one open each times the pipeline consuming sockets

The breaking change is inside RestSharp, it has an issue open but the project seems aware to this difficult change to support net core and I think it has no future plans to fix it. The real change is inside the HttpWebRequest that actually in Net Core no more has a static lifecycle and create a new HttpClient.

So RestSharp is not a valid production library for net core, at all.
This means also Swagger is not a valid choice for net core.
Swagger seems no more active, the main author, William, is here now on OpenApi, reviewing you now (William thank you for the projects, they are great).

So the choice for us are:

• create new contracts with NSwag for the net core projects, leaving Swagger.
• cooperate on OpenApi to have a net-core client fully integrated in the core DI. OpenApi c# codegen is 99% the same as swagger so moving to OpenApi it seems the right way to continue using Swagger.

Your fix is just enough to avoid the exsaustion now.
But having a client that not fit easily in the DI can move us to choose another more solid way for the future.
But I also see a lot of interest in this PR and on this project to move to a working client for net core so I will await and cooperate with you if I can give some support.

[wing328]

I've different suggestions but let's go with what you've so far. We can resume the discussion after the merge.

[wing328]

I've added a new version that includes a switch to disable the exception and the features that don't work if handler isn't present.

Instead of adding a switch, what about adding a constructor that takes just one argument (HttpClient) as follows instead?

        public ApiClient(HttpClient client = null) :

        public ApiClient(HttpClient client = null, HttpClientHandler handler) :

(the handler cannot be null)

[lucamazzanti]

I've added a new version that includes a switch to disable the exception and the features that don't work if handler isn't present.

Instead of adding a switch, what about adding a constructor that takes just one argument (HttpClient) as follows instead?

        public ApiClient(HttpClient client = null) :

        public ApiClient(HttpClient client = null, HttpClientHandler handler) :

(the handler cannot be null)

I created a PR #9109 with that change, if you and @Blackclaws find that changes useful.

[wing328]

@lucamazzanti thanks for the PR 👍 . Will try to review later today.