Skip to content
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.

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

Re-use HttpClient per host. #55

Closed
wants to merge 0 commits into from

Conversation

epbensimpson
Copy link
Contributor

@epbensimpson epbensimpson commented Feb 15, 2022

Fix for #19

Instead of creating a new HttpClient every single time a new DefaultRESTClient is created, only create one per host for the lifetime of the application. Per the HttpClient documentation:

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.

It's not possible to use a single instance without changing it not to set BaseAddress on the HttpClient and reworking how the request URI is built, and I'm not confident I could do that without introducing bugs. So pooling the clients based on the host seems like a good step towards this and I would expect that the majority of users wouldn't be using more than one host per application anyway.

Had to change all request methods to use HttpRequestMessage to avoid setting default headers on the HttpClient.

Also did the following (sorry to clutter the PR but sometimes I just can't help myself):

  • Made goAsync() actually async rather than using Task.Result and Task.ContinueWith().
  • Made the fields private (and readonly where applicable), usually this would be considered a breaking change but DefaultRESTClient is internal anyway so imo it's fine 😛

@mooreds
Copy link
Contributor

mooreds commented Feb 16, 2022

Thanks for the PR @epbensimpson !

We'd need to roll this into the template file: https://github.com/FusionAuth/fusionauth-client-builder/blob/master/src/main/client/netcore.client.ftl or it would be lost the next time we build the client libraries.

If you want to take a pass at that, that'd be awesome. If not, I'll see if I can get to it sometime soon.

@mooreds
Copy link
Contributor

mooreds commented Feb 16, 2022

Actually, please ignore that. It appears the DefaultRestClient is actually defined here.

@epbensimpson
Copy link
Contributor Author

epbensimpson commented Feb 16, 2022

This is not my first DefaultRESTClient rodeo 😉

One thing that I've uncovered - this may actually introduce a different problem. Apparently using a "singleton" HttpClient can cause issues when the DNS changes: dotnet/runtime#18348

The best fix would be to use IHttpClientFactory but the default implementation is tied pretty tightly to Microsoft.Extensions.DependencyInjection: dotnet/aspnetcore#28385

I'm not sure which issue is worse, tbh.

Edit: did a bit more reading and it does seem like the DNS thing is less of an issue - it's only an issue when there's an open connection because it'll reuse that connection without doing a DNS lookup even if the TTL has expired. But as soon as the connection is closed it will do a DNS lookup in order to open the new connection. Per dotnet/runtime#18348 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants