CyberSource / cybersource-rest-client-dotnetstandard

.NET Standard client library for the CyberSource REST API
7 stars 20 forks source link

Update RestSharp to v107 and follow recommended usage #46

Open sebastian-sawicki opened 2 years ago

sebastian-sawicki commented 2 years ago

With the new version implemented, RestSharp solves a couple of issues:

Upgrade RestSharp to to v107 https://restsharp.dev/v107/#restsharp-v107 and follow best practices https://restsharp.dev/v107/#recommended-usage

https://code-maze.com/httpclient-vs-restsharp/

sebastian-sawicki commented 1 year ago

Please provide option to configure HttpClient in merchant’s source code and inject HttpClient into CyberSource Client SDK. The latest RestSharp.RestClient has constructor which accepts HttpClient. https://github.com/restsharp/RestSharp/blob/dev/src/RestSharp/RestClient.cs#L99

WHY:

HttpClient has problems. Create an instance for every request and you will run into socket exhaustion. Make it a singleton and it will not respect DNS changes. Microsoft explains it in its documentation and provides 2 solutions – either HttpClient + SocketsHttpHandler settings or HttpClientFactory with its extension methods – for connection pooling and connection lifetime management.

  1. https://learn.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests
  2. https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines
  3. https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-6.0
  4. https://www.stevejgordon.co.uk/httpclient-connection-pooling-in-dotnet-core

Please see that RestSharp’s contributors have already noticed that RestSharp’s recommendation to always create a singleton RestClient https://restsharp.dev/v107/#restclient-lifecycle doesn’t work for all use cases. There is active issue on RestSharp GitHub https://github.com/restsharp/RestSharp/issues/1791 which mentions: “The biggest problem I foresee with the way v107 is currently implemented is while it does fix the issued with connection exhaustion, it does not fix the issues with DNS resolution problems. So anyone using RestSharp v107 and singleton instances, or anyone relying on a library that does the same (like EasyPost if they accept my patches) can potentially run into DNS related problems if their application needs to respect DNS changes.” and last comment https://github.com/restsharp/RestSharp/issues/1791#issuecomment-1163659767 is also what we ask for.