Vantiv / cnp-sdk-for-dotnet

.NET SDK to ease XML integrations with the Vantiv eCommerce platform
Other
3 stars 39 forks source link

Fix using HttpClient as a singleton #41

Open timothy-b opened 3 years ago

timothy-b commented 3 years ago

I appreciate the transition to using HttpClient in this commit (thanks @GregoryConrad!) , but there's a known problem with using a singleton HttpClient: it won't respect DNS changes made over its lifetime. According to the following documentation, the current best-practice for managing HttpClients over the lifetime of long-running apps is by using an IHttpClientFactory.

So, rather than constructing a HttpClient here, I suspect we should perhaps pass in a Func<HttpMessageHandler, HttpClient> to the class's constructor, and store the resulting client as an instance member.

This concern is blocking me from upgrading Vantiv.CnpSdkForNet in my team's project. So, I'd be happy to open a PR and implement my suggested changes, but since this would be my first time contributing to this repo, I'd appreciate it if someone would give me the thumbs-up before I got started.

VantivSDK commented 3 years ago

Hi timothy, thanks for helping us with your suggestion. Please feel free to apply your suggestion in PR. Once you are ready with PR, we will perform a few additional tests at our end. And then we will accept and release a new version with your changes.

andrebts commented 3 years ago

@timothy-b Please refer to this https://github.com/twilio/twilio-csharp/issues/529

mattisking commented 2 years ago

So, I'm not planning any kind of immediate PR because my branch intentionally breaks some functionality (mostly around logging to just use ILogger) but this is basically one of a couple options for what you want to do:

I'm moving to my version internally on a project but the lifting is done here if you are interested, including updating all the tests, etc. I added a sample for using Dependency Injection. My sample is still .Net Core 3.1 and could be moved to .Net 6 but we haven't made that move yet.

https://github.com/mattisking/cnp-sdk-for-dotnet

I am willing to do some more work on it, it might just take a little time as I'm swamped at work.

mgungorchamp commented 1 year ago

Use IHttpClientFactory to implement resilient HTTP requests

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