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

Refactor httpClient to be reused #208

Open
jhorn70 opened this issue Aug 15, 2022 · 4 comments
Open

Refactor httpClient to be reused #208

jhorn70 opened this issue Aug 15, 2022 · 4 comments

Comments

@jhorn70
Copy link

jhorn70 commented Aug 15, 2022

I have been trying to troubleshoot an issue with task cancellation and stumbled on the below.

https://www.aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/

https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient?redirectedfrom=MSDN&view=net-6.0#Remarks

Looks like the best practice is to re-use HttpClient across multiple requests.

In looking at the code, I would probably implement the RequestHandler as static inside AbstractClient. It can be reused for all requests for the lifetime of an instance of OnlineClient. Inside that I would implement HttpClient as static in the same way.

@jhorn70 jhorn70 changed the title Refactor httpClient as singleton Refactor httpClient to be reused Aug 15, 2022
@davidathompson
Copy link

Yes, that was the first question I head when digging through the code. A new HttpClient with every request? Yikes.....this could destabilize an app with enough volume.

@sfwatanabe
Copy link

To add this issue - it would be nice to allow the option to pass our own HttpClient instance the Offline/OnlineClient constructors

@davidathompson
Copy link

To further qualify this, a new HttpClient is actually fine, and is typically lightweight. And the logging wireup is useful here. What is actually at issue is:

HttpClientHandler httpClientHandler = new HttpClientHandler()

done with each request. Allowing the client to supply the inner HttpMessageHandler could give you the best of all the worlds, allowing the same logging wireup as you have now. Or allow either/or to be supplied by the client if they want to simply supply the HttpClient.

We typically pull our HttpClinets / HttpMessageHandlers from:

https://learn.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests

Maybe add HttpMessageHandler and HttpClient properties to ClientConfig? That way you can fill in the HttpMessageHandler if you know what you are doing there or simply HttpClient if you want to simplify (or leave null to use the current methodology) ?

@davidathompson
Copy link

Create pull request to address:

#232

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

No branches or pull requests

3 participants