Update HttpClientFactory to use singleton client

Don't create a new `HttpClient` on each `GetClient` call; we run the
risk of exhausting all available sockets because a socket is assigned
per client instance, and the socket does not become free immediately
after disposal of the client.

In light of this change to use a singleton instance, don't set the
default headers on that singleton client otherwise we will pollute
the client setup for other callers. Instead we set the headers per
request message that is sent.

Tests have been updated to cover the above changes.

Additional minor change: move the HTTP-y constants to a nested static
class `Constants.Http.*` for better grouping.

Author: mjcheetham

src/Microsoft.Git.CredentialManager/Authentication/WindowsIntegratedAuthentication.cs

@@ -32,21 +32,21 @@ public async Task<bool> GetIsSupportedAsync(Uri uri)
 
             bool supported = false;
 
-            _context.Trace.WriteLine($"HTTP HEAD {uri}");
+            _context.Trace.WriteLine($"HTTP: HEAD {uri}");
             using (HttpClient client = _httpFactory.GetClient())
-            using (HttpResponseMessage response = await client.HeadAsync(uri))
+            using (HttpResponseMessage response = await client.SendAsync(HttpMethod.Head, uri))
             {
-                _context.Trace.WriteLine("HTTP Response - Ignoring response code");
+                _context.Trace.WriteLine("HTTP: Response code ignored.");
 
                 _context.Trace.WriteLine("Inspecting WWW-Authenticate headers...");
                 foreach (AuthenticationHeaderValue wwwHeader in response.Headers.WwwAuthenticate)
                 {
-                    if (StringComparer.OrdinalIgnoreCase.Equals(wwwHeader.Scheme, Constants.WwwAuthenticateNegotiateScheme))
+                    if (StringComparer.OrdinalIgnoreCase.Equals(wwwHeader.Scheme, Constants.Http.WwwAuthenticateNegotiateScheme))
                     {
                         _context.Trace.WriteLine("Found WWW-Authenticate header for Negotiate");
                         supported = true;
                     }
-                    else if (StringComparer.OrdinalIgnoreCase.Equals(wwwHeader.Scheme, Constants.WwwAuthenticateNtlmScheme))
+                    else if (StringComparer.OrdinalIgnoreCase.Equals(wwwHeader.Scheme, Constants.Http.WwwAuthenticateNtlmScheme))
                     {
                         _context.Trace.WriteLine("Found WWW-Authenticate header for NTLM");
                         supported = true;

src/Microsoft.Git.CredentialManager/Constants.cs

@@ -1,6 +1,5 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
-using System.Text;
 
 namespace Microsoft.Git.CredentialManager
 {
@@ -9,9 +8,6 @@ public static class Constants
         public const string GcmVersion = "1.0";
         public const string PersonalAccessTokenUserName = "PersonalAccessToken";
 
-        public const string WwwAuthenticateNegotiateScheme = "Negotiate";
-        public const string WwwAuthenticateNtlmScheme      = "NTLM";
-
         public static class EnvironmentVariables
         {
             public const string GcmTrace           = "GCM_TRACE";
@@ -20,6 +16,12 @@ public static class EnvironmentVariables
             public const string GitTerminalPrompts = "GIT_TERMINAL_PROMPT";
         }
 
+        public static class Http
+        {
+            public const string WwwAuthenticateNegotiateScheme = "Negotiate";
+            public const string WwwAuthenticateNtlmScheme      = "NTLM";
+        }
+
         /// <summary>
         /// Get standard program header title for Git Credential Manager, including the current version and OS information.
         /// </summary>

src/Microsoft.Git.CredentialManager/HttpClientExtensions.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
 using System;
+using System.Collections.Generic;
 using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
@@ -52,5 +53,31 @@ public static Task<HttpResponseMessage> HeadAsync(this HttpClient client, Uri re
         }
 
         #endregion
+
+        #region SendAsync with content and headers
+
+        public static Task<HttpResponseMessage> SendAsync(this HttpClient client,
+            HttpMethod method,
+            Uri requestUri,
+            IEnumerable<KeyValuePair<string, IEnumerable<string>>> headers = null,
+            HttpContent content = null)
+        {
+            var request = new HttpRequestMessage(method, requestUri)
+            {
+                Content = content
+            };
+
+            if (!(headers is null))
+            {
+                foreach (var kvp in headers)
+                {
+                    request.Headers.Add(kvp.Key, kvp.Value);
+                }
+            }
+
+            return client.SendAsync(request);
+        }
+
+        #endregion
     }
 }

src/Microsoft.Git.CredentialManager/HttpClientFactory.cs

@@ -1,6 +1,5 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
-using System.Collections.Generic;
 using System.Net.Http;
 using System.Net.Http.Headers;
 
@@ -20,57 +19,24 @@ public interface IHttpClientFactory
 
     public class HttpClientFactory : IHttpClientFactory
     {
-        public HttpClient GetClient()
-        {
-            var client = new HttpClient();
-
-            // Add default headers
-            client.DefaultRequestHeaders.UserAgent.ParseAdd(Constants.GetHttpUserAgent());
-            client.DefaultRequestHeaders.CacheControl = new CacheControlHeaderValue
-            {
-                NoCache = true
-            };
+        private HttpClient _client;
 
-            return client;
-        }
-    }
-
-    public static class HttpClientFactoryExtensions
-    {
-        /// <summary>
-        /// Get an instance of <see cref="HttpClient"/> with default headers and a bearer token based authorization header.
-        /// </summary>
-        /// <param name="factory"><see cref="IHttpClientFactory"/></param>
-        /// <param name="bearerToken">Bearer token value</param>
-        /// <returns>Client with default and bearer token headers.</returns>
-        public static HttpClient GetClient(this IHttpClientFactory factory, string bearerToken)
-        {
-            var client = factory.GetClient();
-
-            if (bearerToken != null)
-            {
-                client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", bearerToken);
-            }
-
-            return client;
-        }
-
-        /// <summary>
-        /// Get an instance of <see cref="HttpClient"/> with default headers and additional custom headers.
-        /// </summary>
-        /// <param name="factory"><see cref="IHttpClientFactory"/></param>
-        /// <param name="headers">Custom HTTP headers to set on the client.</param>
-        /// <returns>Client with default and custom headers.</returns>
-        public static HttpClient GetClient(this IHttpClientFactory factory, IDictionary<string, IEnumerable<string>> headers)
+        public HttpClient GetClient()
         {
-            var client = factory.GetClient();
-
-            foreach (var header in headers)
+            if (_client is null)
             {
-                client.DefaultRequestHeaders.Add(header.Key, header.Value);
+                // Initialize a new HttpClient
+                _client = new HttpClient();
+
+                // Add default headers
+                _client.DefaultRequestHeaders.UserAgent.ParseAdd(Constants.GetHttpUserAgent());
+                _client.DefaultRequestHeaders.CacheControl = new CacheControlHeaderValue
+                {
+                    NoCache = true
+                };
             }
 
-            return client;
+            return _client;
         }
     }
 }

tests/Microsoft.Git.CredentialManager.Tests/HttpClientExtensionsTests.cs

@@ -0,0 +1,86 @@
+using System;
+using System.Collections.Generic;
+using System.Net.Http;
+using System.Threading.Tasks;
+using Microsoft.Git.CredentialManager.Tests.Objects;
+using Xunit;
+
+namespace Microsoft.Git.CredentialManager.Tests
+{
+    public class HttpClientExtensionsTests
+    {
+        [Fact]
+        public async Task HttpClientExtensions_SendAsync_SendsRequestMessage()
+        {
+            var method = HttpMethod.Get;
+            var uri = new Uri("http://example.com");
+
+            var httpHandler = new TestHttpMessageHandler{ThrowOnUnexpectedRequest = true};
+            httpHandler.Setup(method, uri, request =>
+            {
+                Assert.Equal(method, request.Method);
+                Assert.Equal(uri, request.RequestUri);
+
+                return new HttpResponseMessage();
+            });
+
+            var httpClient = new HttpClient(httpHandler);
+
+            await HttpClientExtensions.SendAsync(httpClient, method, uri);
+        }
+
+        [Fact]
+        public async Task HttpClientExtensions_SendAsync_Content_SetsContent()
+        {
+            var method = HttpMethod.Get;
+            var uri = new Uri("http://example.com");
+
+            var expectedContent = new StringContent("foobar");
+
+            var httpHandler = new TestHttpMessageHandler{ThrowOnUnexpectedRequest = true};
+            httpHandler.Setup(method, uri, request =>
+            {
+                Assert.Same(expectedContent,request.Content);
+
+                return new HttpResponseMessage();
+            });
+
+            var httpClient = new HttpClient(httpHandler);
+
+            await HttpClientExtensions.SendAsync(httpClient, method, uri, null, expectedContent);
+        }
+
+        [Fact]
+        public async Task HttpClientExtensions_SendAsync_Headers_SetsHeaders()
+        {
+            var method = HttpMethod.Get;
+            var uri = new Uri("http://example.com");
+
+            var customHeaders = new Dictionary<string, IEnumerable<string>>
+            {
+                ["header0"] = new string[0],
+                ["header1"] = new []{ "first-value" },
+                ["header2"] = new []{ "first-value", "second-value"},
+                ["header3"] = new []{ "first-value", "second-value", "third-value"},
+            };
+
+            var httpHandler = new TestHttpMessageHandler{ThrowOnUnexpectedRequest = true};
+            httpHandler.Setup(method, uri, request =>
+            {
+                Assert.False(request.Headers.Contains("header0"));
+                Assert.True(request.Headers.Contains("header1"));
+                Assert.True(request.Headers.Contains("header2"));
+                Assert.True(request.Headers.Contains("header3"));
+                Assert.Equal(customHeaders["header1"], request.Headers.GetValues("header1"));
+                Assert.Equal(customHeaders["header2"], request.Headers.GetValues("header2"));
+                Assert.Equal(customHeaders["header3"], request.Headers.GetValues("header3"));
+
+                return new HttpResponseMessage();
+            });
+
+            var httpClient = new HttpClient(httpHandler);
+
+            await HttpClientExtensions.SendAsync(httpClient, method, uri, customHeaders);
+        }
+    }
+}

tests/Microsoft.Git.CredentialManager.Tests/HttpClientFactoryTests.cs

@@ -1,6 +1,5 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
-using System.Collections.Generic;
 using System.Net.Http;
 using Xunit;
 
@@ -21,43 +20,14 @@ public void HttpClientFactory_GetClient_SetsDefaultHeaders()
         }
 
         [Fact]
-        public void HttpClientFactory_GetClient_BearerToken_SetsAuthorizationBearerTokenHeader()
+        public void HttpClientFactory_GetClient_MultipleCalls_ReturnsSameInstance()
         {
-            const string bearerToken = "letmein123";
-
             var factory = new HttpClientFactory();
 
-            HttpClient client = factory.GetClient(bearerToken);
-
-            Assert.NotNull(client);
-            Assert.NotNull(client.DefaultRequestHeaders.Authorization);
-            Assert.Equal("Bearer", client.DefaultRequestHeaders.Authorization.Scheme);
-            Assert.Equal(bearerToken, client.DefaultRequestHeaders.Authorization.Parameter);
-        }
+            HttpClient client1 = factory.GetClient();
+            HttpClient client2 = factory.GetClient();
 
-        [Fact]
-        public void HttpClientFactory_GetClient_CustomHeaders_SetsHeaders()
-        {
-            var customHeaders = new Dictionary<string, IEnumerable<string>>
-            {
-                ["header0"] = new string[0],
-                ["header1"] = new []{ "first-value" },
-                ["header2"] = new []{ "first-value", "second-value"},
-                ["header3"] = new []{ "first-value", "second-value", "third-value"},
-            };
-
-            var factory = new HttpClientFactory();
-
-            HttpClient client = factory.GetClient(customHeaders);
-
-            Assert.NotNull(client);
-            Assert.False(client.DefaultRequestHeaders.Contains("header0"));
-            Assert.True(client.DefaultRequestHeaders.Contains("header1"));
-            Assert.True(client.DefaultRequestHeaders.Contains("header2"));
-            Assert.True(client.DefaultRequestHeaders.Contains("header3"));
-            Assert.Equal(customHeaders["header1"], client.DefaultRequestHeaders.GetValues("header1"));
-            Assert.Equal(customHeaders["header2"], client.DefaultRequestHeaders.GetValues("header2"));
-            Assert.Equal(customHeaders["header3"], client.DefaultRequestHeaders.GetValues("header3"));
+            Assert.Same(client1, client2);
         }
     }
 }

tests/Microsoft.Git.CredentialManager.Tests/Objects/TestHttpMessageHandler.cs

@@ -6,28 +6,42 @@
 using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.Git.CredentialManager.Authentication;
 using Xunit;
 
 namespace Microsoft.Git.CredentialManager.Tests.Objects
 {
     public class TestHttpMessageHandler : HttpMessageHandler
     {
-        private readonly IDictionary<(HttpMethod method, Uri uri), HttpResponseMessage> _handlers =
-                      new Dictionary<(HttpMethod, Uri), HttpResponseMessage>();
+        public delegate HttpResponseMessage RequestHandler(HttpRequestMessage request);
+
+        private readonly IDictionary<(HttpMethod method, Uri uri), RequestHandler> _handlers =
+                      new Dictionary<(HttpMethod, Uri), RequestHandler>();
 
         private readonly IDictionary<(HttpMethod method, Uri uri), int> _requestCounts =
-                      new Dictionary<(HttpMethod method, Uri uri), int>();
+                      new Dictionary<(HttpMethod, Uri), int>();
 
         public bool ThrowOnUnexpectedRequest { get; set; }
+        public bool SimulateNoNetwork { get; set; }
+
+        public void Setup(HttpMethod method, Uri uri, RequestHandler handler)
+        {
+            _handlers[(method, uri)] = handler;
+        }
 
         public void Setup(HttpMethod method, Uri uri, HttpResponseMessage responseMessage)
         {
-            _handlers[(method, uri)] = responseMessage;
+            _handlers[(method, uri)] = _ => responseMessage;
         }
 
         public void Setup(HttpMethod method, Uri uri, HttpStatusCode responseCode)
         {
-            Setup(method, uri, new HttpResponseMessage(responseCode));
+            Setup(method, uri, responseCode, string.Empty);
+        }
+
+        public void Setup(HttpMethod method, Uri uri, HttpStatusCode responseCode, string content)
+        {
+            Setup(method, uri, new HttpResponseMessage(responseCode){Content = new StringContent(content)});
         }
 
         public void AssertRequest(HttpMethod method, Uri uri, int expectedNumberOfCalls)
@@ -49,12 +63,16 @@ protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage reques
 
             IncrementRequestCount(requestKey);
 
+            if (SimulateNoNetwork)
+            {
+                throw new HttpRequestException("Simulated no network");
+            }
+
             foreach (var kvp in _handlers)
             {
                 if (kvp.Key == requestKey)
                 {
-                    HttpResponseMessage response = kvp.Value;
-                    return Task.FromResult(response);
+                    return Task.FromResult(kvp.Value(request));
                 }
             }