braintree / braintree_dotnet

Braintree .NET library
https://developer.paypal.com/braintree/docs/start/overview
MIT License
136 stars 73 forks source link

Socket exhaustion from reuse of HttpClient #79

Closed hyphmongo closed 5 years ago

hyphmongo commented 6 years ago

General information

Issue description

Under high traffic we are reaching the point where no more connections can be opened to braintree with netstat showing a lot of connections in TIME_WAIT state.

Looking at HttpService.cs a new instance of HttpClient is being created per request. From the docs the recommendation is to instantiate HttpClient once as a static on the class.

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.

crookedneighbor commented 5 years ago

Thanks for the report. We're working on a fix for this and doing some testing internally.

Here's what we have so far:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c44aa5ef..4a9933f7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,6 @@
+## unreleased
+* Fix issue where HttpClient could throw exceptions under heavy traffic (closes #79)
+
 ## 4.7.0
 * Add `AuthorizationExpiresAt` to `Transaction`
 * Add `ProcessorResponseType` to `Transaction`, `AuthorizationAdjustment`, and `CreditCardVerification`
diff --git a/src/Braintree/HttpService.cs b/src/Braintree/HttpService.cs
index d1d326a8..25d85090 100644
--- a/src/Braintree/HttpService.cs
+++ b/src/Braintree/HttpService.cs
@@ -22,6 +22,10 @@ namespace Braintree
         protected static readonly Encoding encoding = Encoding.UTF8;

         protected Configuration Configuration;
+#if netcore
+        protected HttpClient HttpClientWithoutProxy;
+        protected HttpClient HttpClientWithProxy;
+#endif

         public Environment Environment
         {
@@ -61,6 +65,25 @@ namespace Braintree
         public HttpService(Configuration configuration)
         {
             Configuration = configuration;
+
+#if netcore
+            HttpClientWithoutProxy = new HttpClient(new HttpClientHandler {
+                AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
+                UseProxy = false,
+            }, false);
+            HttpClientWithoutProxy.Timeout = TimeSpan.FromMilliseconds(Configuration.Timeout);
+
+            var proxy = GetWebProxy();
+
+            if (proxy != null) {
+                HttpClientWithProxy = new HttpClient(new HttpClientHandler {
+                    AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
+                    UseProxy = true,
+                    Proxy = proxy,
+                }, false);
+                HttpClientWithProxy.Timeout = TimeSpan.FromMilliseconds(Configuration.Timeout);
+            }
+#endif
         }

 #if netcore
@@ -89,45 +112,27 @@ namespace Braintree
         }

         public string GetHttpResponse(HttpRequestMessage request) {
-            var httpClientHandler = new HttpClientHandler
-            {
-                AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
-            };
+            HttpClient client = GetHttpClient(request.RequestUri);

-            SetWebProxy(httpClientHandler, request.RequestUri);
-
-            using (var client = new HttpClient(httpClientHandler))
+            var response = client.SendAsync(request).GetAwaiter().GetResult();
+            if (response.StatusCode != (HttpStatusCode) 422)
             {
-                client.Timeout = TimeSpan.FromMilliseconds(Configuration.Timeout);
-                var response = client.SendAsync(request).GetAwaiter().GetResult();
-                if (response.StatusCode != (HttpStatusCode) 422)
-                {
-                    ThrowExceptionIfErrorStatusCode(response.StatusCode, null);
-                }
-
-                return ParseResponseStream(response.Content.ReadAsStreamAsync().GetAwaiter().GetResult());
+                ThrowExceptionIfErrorStatusCode(response.StatusCode, null);
             }
+
+            return ParseResponseStream(response.Content.ReadAsStreamAsync().GetAwaiter().GetResult());
         }

         public async Task<string> GetHttpResponseAsync(HttpRequestMessage request) {
-            var httpClientHandler = new HttpClientHandler
-            {
-                AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
-            };
-
-            SetWebProxy(httpClientHandler, request.RequestUri);
+            HttpClient client = GetHttpClient(request.RequestUri);

-            using (var client = new HttpClient(httpClientHandler))
+            var response = client.SendAsync(request).GetAwaiter().GetResult();
+            if (response.StatusCode != (HttpStatusCode) 422)
             {
-                client.Timeout = TimeSpan.FromMilliseconds(Configuration.Timeout);
-                var response = client.SendAsync(request).GetAwaiter().GetResult();
-                if (response.StatusCode != (HttpStatusCode) 422)
-                {
-                    ThrowExceptionIfErrorStatusCode(response.StatusCode, null);
-                }
-
-                return await ParseResponseStreamAsync(response.Content.ReadAsStreamAsync().GetAwaiter().GetResult());
+                ThrowExceptionIfErrorStatusCode(response.StatusCode, null);
             }
+
+            return await ParseResponseStreamAsync(response.Content.ReadAsStreamAsync().GetAwaiter().GetResult());
         }
 #else
         public HttpWebRequest GetHttpRequest(string URL, string method) {
@@ -197,18 +202,12 @@ namespace Braintree
 #endif

 #if netcore
-        protected void SetWebProxy(HttpClientHandler httpClientHandler, Uri URL)
-        {
-            var proxy = GetWebProxy();
-            bool useProxy = false;
-
-            if (proxy != null && !proxy.IsBypassed(URL))
-            {
-                useProxy = true;
-                httpClientHandler.Proxy = proxy;
+        protected HttpClient GetHttpClient (Uri URL) {
+            if (HttpClientWithProxy != null && !GetWebProxy().IsBypassed(URL)) {
+                return HttpClientWithProxy;
             }

-            httpClientHandler.UseProxy = useProxy;
+            return HttpClientWithoutProxy;
         }
 #else
         protected void SetRequestProxy(WebRequest request)

If you'd like to give it a try, you can save that to a file named httpclient.diff and apply it with: git apply httpclient.diff

crookedneighbor commented 5 years ago

Upon further examination, it looks like a new braintree service is being created in each request, which I didn't realize. 😬

Here's a new diff that uses a static client if configured.

diff --git a/src/Braintree/Configuration.cs b/src/Braintree/Configuration.cs
index 4384f3ba..32b5c560 100644
--- a/src/Braintree/Configuration.cs
+++ b/src/Braintree/Configuration.cs
@@ -59,6 +59,11 @@ namespace Braintree
             get { return timeout == 0 ? 60000 : timeout; }
             set { timeout = value; }
         }
+        private bool useStaticHttpClient;
+        public bool UseStaticHttpClient {
+            get { return useStaticHttpClient ? true : false; }
+            set { useStaticHttpClient = value; }
+        }

         public Configuration()
         {
diff --git a/src/Braintree/HttpService.cs b/src/Braintree/HttpService.cs
index d1d326a8..9d8d6d04 100644
--- a/src/Braintree/HttpService.cs
+++ b/src/Braintree/HttpService.cs
@@ -20,6 +20,9 @@ namespace Braintree
     public class HttpService
     {
         protected static readonly Encoding encoding = Encoding.UTF8;
+        protected static HttpClient staticClient = new HttpClient(new HttpClientHandler {
+                AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
+        }, false);

         protected Configuration Configuration;

@@ -88,45 +91,61 @@ namespace Braintree
             return request;
         }

-        public string GetHttpResponse(HttpRequestMessage request) {
-            var httpClientHandler = new HttpClientHandler
+        public string GetHttpResponseWithClient(HttpClient client, HttpRequestMessage request) {
+            var response = client.SendAsync(request).GetAwaiter().GetResult();
+            if (response.StatusCode != (HttpStatusCode) 422)
             {
-                AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
-            };
+                ThrowExceptionIfErrorStatusCode(response.StatusCode, null);
+            }

-            SetWebProxy(httpClientHandler, request.RequestUri);
+            return ParseResponseStream(response.Content.ReadAsStreamAsync().GetAwaiter().GetResult());
+        }

-            using (var client = new HttpClient(httpClientHandler))
+        public async Task<string> GetHttpResponseWithClientAsync(HttpClient client, HttpRequestMessage request) {
+            var response = client.SendAsync(request).GetAwaiter().GetResult();
+            if (response.StatusCode != (HttpStatusCode) 422)
             {
-                client.Timeout = TimeSpan.FromMilliseconds(Configuration.Timeout);
-                var response = client.SendAsync(request).GetAwaiter().GetResult();
-                if (response.StatusCode != (HttpStatusCode) 422)
+                ThrowExceptionIfErrorStatusCode(response.StatusCode, null);
+            }
+
+            return await ParseResponseStreamAsync(response.Content.ReadAsStreamAsync().GetAwaiter().GetResult());
+        }
+
+        public string GetHttpResponse(HttpRequestMessage request) {
+            if (Configuration.UseStaticHttpClient == true) {
+                return GetHttpResponseWithClient(staticClient, request);
+            } else {
+                var httpClientHandler = new HttpClientHandler
                 {
-                    ThrowExceptionIfErrorStatusCode(response.StatusCode, null);
-                }
+                    AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
+                };
+
+                SetWebProxy(httpClientHandler, request.RequestUri);

-                return ParseResponseStream(response.Content.ReadAsStreamAsync().GetAwaiter().GetResult());
+                using (var client = new HttpClient(httpClientHandler))
+                {
+                    client.Timeout = TimeSpan.FromMilliseconds(Configuration.Timeout);
+                    return GetHttpResponseWithClient(client, request);
+                }
             }
         }

         public async Task<string> GetHttpResponseAsync(HttpRequestMessage request) {
-            var httpClientHandler = new HttpClientHandler
-            {
-                AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
-            };
+            if (Configuration.UseStaticHttpClient == true) {
+                return await GetHttpResponseWithClientAsync(staticClient, request);
+            } else {
+                var httpClientHandler = new HttpClientHandler
+                {
+                    AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
+                };

-            SetWebProxy(httpClientHandler, request.RequestUri);
+                SetWebProxy(httpClientHandler, request.RequestUri);

-            using (var client = new HttpClient(httpClientHandler))
-            {
-                client.Timeout = TimeSpan.FromMilliseconds(Configuration.Timeout);
-                var response = client.SendAsync(request).GetAwaiter().GetResult();
-                if (response.StatusCode != (HttpStatusCode) 422)
+                using (var client = new HttpClient(httpClientHandler))
                 {
-                    ThrowExceptionIfErrorStatusCode(response.StatusCode, null);
+                    client.Timeout = TimeSpan.FromMilliseconds(Configuration.Timeout);
+                    return await GetHttpResponseWithClientAsync(client, request);
                 }
-
-                return await ParseResponseStreamAsync(response.Content.ReadAsStreamAsync().GetAwaiter().GetResult());
             }
         }
 #else

If you'd like, you can try this diff out.

hyphmongo commented 5 years ago

Thanks! Looks like the static HttpClient is working in my test scenarios

Should there also be an #if netcore check when creating staticClient?

crookedneighbor commented 5 years ago

Yes, I think you are right.

crookedneighbor commented 5 years ago

This is out now in 4.8.0