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

Fix using HttpClient as a singleton #41

Open
timothy-b opened this issue Dec 2, 2020 · 4 comments
Open

Fix using HttpClient as a singleton #41

timothy-b opened this issue Dec 2, 2020 · 4 comments

Comments

@timothy-b
Copy link

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
Copy link
Contributor

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
Copy link

@timothy-b Please refer to this twilio/twilio-csharp#529

@mattisking
Copy link

mattisking commented Jun 27, 2022

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
Copy link

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

No branches or pull requests

5 participants