Merge pull request #14 from mjcheetham/fix-httpclient

Don't double dispose HttpClients and ensure all objects are correctly disposed on exit

Author: mjcheetham

src/Microsoft.AzureRepos/AzureDevOpsRestApi.cs

@@ -12,15 +12,14 @@
 
 namespace Microsoft.AzureRepos
 {
-    public interface IAzureDevOpsRestApi
+    public interface IAzureDevOpsRestApi : IDisposable
     {
         Task<string> GetAuthorityAsync(Uri organizationUri);
         Task<string> CreatePersonalAccessTokenAsync(Uri organizationUri, string accessToken, IEnumerable<string> scopes);
     }
 
     public class AzureDevOpsRestApi : IAzureDevOpsRestApi
     {
-
         private readonly ICommandContext _context;
         private readonly IHttpClientFactory _httpFactory;
 
@@ -44,11 +43,8 @@ public async Task<string> GetAuthorityAsync(Uri organizationUri)
             const string commonAuthority = authorityBase + "common";
             const string msaAuthority = authorityBase + "live.com";
 
-            var headers = new[] {Constants.Http.AcceptHeader(Constants.Http.MimeTypeJson)};
-
             _context.Trace.WriteLine($"HTTP: HEAD {organizationUri}");
-            using (var client = _httpFactory.GetClient())
-            using (var response = await client.SendAsync(HttpMethod.Head, organizationUri, headers))
+            using (HttpResponseMessage response = await HttpClient.HeadAsync(organizationUri))
             {
                 _context.Trace.WriteLine("HTTP: Response code ignored.");
                 _context.Trace.WriteLine("Inspecting headers...");
@@ -112,16 +108,10 @@ public async Task<string> CreatePersonalAccessTokenAsync(Uri organizationUri, st
 
             Uri requestUri = new Uri(identityServiceUri, sessionTokenUrl);
 
-            var headers = new[]
-            {
-                Constants.Http.AcceptHeader(Constants.Http.MimeTypeJson),
-                Constants.Http.AuthorizationBearerHeader(accessToken)
-            };
-
             _context.Trace.WriteLine($"HTTP: POST {requestUri}");
             using (StringContent content = CreateAccessTokenRequestJson(organizationUri, scopes))
-            using (var client = _httpFactory.GetClient())
-            using (var response = await client.SendAsync(HttpMethod.Post, requestUri, headers, content))
+            using (HttpRequestMessage request = CreateRequestMessage(HttpMethod.Post, requestUri, content, accessToken))
+            using (HttpResponseMessage response = await HttpClient.SendAsync(request))
             {
                 _context.Trace.WriteLine($"HTTP: Response {(int)response.StatusCode} [{response.StatusCode}]");
 
@@ -162,15 +152,9 @@ private async Task<Uri> GetIdentityServiceUriAsync(Uri organizationUri, string a
                 Query = locationServiceQuery,
             }.Uri;
 
-            var headers = new[]
-            {
-                Constants.Http.AcceptHeader(Constants.Http.MimeTypeJson),
-                Constants.Http.AuthorizationBearerHeader(accessToken)
-            };
-
             _context.Trace.WriteLine($"HTTP: GET {requestUri}");
-            using (var client = _httpFactory.GetClient())
-            using (var response = await client.SendAsync(HttpMethod.Get, requestUri, headers))
+            using (HttpRequestMessage request = CreateRequestMessage(HttpMethod.Get, requestUri, bearerToken: accessToken))
+            using (HttpResponseMessage response = await HttpClient.SendAsync(request))
             {
                 _context.Trace.WriteLine($"HTTP: Response {(int)response.StatusCode} [{response.StatusCode}]");
                 if (response.IsSuccessStatusCode)
@@ -194,6 +178,24 @@ private async Task<Uri> GetIdentityServiceUriAsync(Uri organizationUri, string a
 
         private const RegexOptions CommonRegexOptions = RegexOptions.Compiled | RegexOptions.CultureInvariant | RegexOptions.IgnoreCase;
 
+        private HttpClient _httpClient;
+
+        private HttpClient HttpClient
+        {
+            get
+            {
+                if (_httpClient is null)
+                {
+                    _httpClient = _httpFactory.CreateClient();
+
+                    // Configure the HTTP client with standard headers for Azure Repos API calls
+                    _httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue(Constants.Http.MimeTypeJson));
+                }
+
+                return _httpClient;
+            }
+        }
+
         /// <summary>
         /// Attempt to extract the authority from a Authorization Bearer header.
         /// </summary>
@@ -270,6 +272,40 @@ private static StringContent CreateAccessTokenRequestJson(Uri organizationUri, I
             return content;
         }
 
+        /// <summary>
+        /// Create an <see cref="HttpRequestMessage"/> with optional content and bearer-token authorization header.
+        /// </summary>
+        /// <param name="method">HTTP request method type.</param>
+        /// <param name="uri">Request URI.</param>
+        /// <param name="content">Optional request content.</param>
+        /// <param name="bearerToken">Optional bearer token for authorization.</param>
+        /// <returns>HTTP request message.</returns>
+        private static HttpRequestMessage CreateRequestMessage(HttpMethod method, Uri uri, HttpContent content = null, string bearerToken = null)
+        {
+            var request = new HttpRequestMessage(method, uri);
+
+            if (!(content is null))
+            {
+                request.Content = content;
+            }
+
+            if (!string.IsNullOrWhiteSpace(bearerToken))
+            {
+                request.Headers.Authorization = new AuthenticationHeaderValue(Constants.Http.WwwAuthenticateBearerScheme, bearerToken);
+            }
+
+            return request;
+        }
+
+        #endregion
+
+        #region IDisposable
+
+        public void Dispose()
+        {
+            _httpClient?.Dispose();
+        }
+
         #endregion
     }
 }

src/Microsoft.AzureRepos/AzureReposHostProvider.cs

@@ -85,6 +85,16 @@ public override async Task<GitCredential> CreateCredentialAsync(InputArguments i
             return new GitCredential(Constants.PersonalAccessTokenUserName, pat);
         }
 
+        protected override void Dispose(bool disposing)
+        {
+            if (disposing)
+            {
+                _azDevOps.Dispose();
+            }
+
+            base.Dispose(disposing);
+        }
+
         #endregion
     }
 }

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

@@ -7,7 +7,7 @@
 
 namespace Microsoft.Git.CredentialManager.Authentication
 {
-    public interface IWindowsIntegratedAuthentication
+    public interface IWindowsIntegratedAuthentication : IDisposable
     {
         Task<bool> GetIsSupportedAsync(Uri uri);
     }
@@ -33,8 +33,7 @@ public async Task<bool> GetIsSupportedAsync(Uri uri)
             bool supported = false;
 
             _context.Trace.WriteLine($"HTTP: HEAD {uri}");
-            using (HttpClient client = _httpFactory.GetClient())
-            using (HttpResponseMessage response = await client.SendAsync(HttpMethod.Head, uri))
+            using (HttpResponseMessage response = await HttpClient.HeadAsync(uri))
             {
                 _context.Trace.WriteLine("HTTP: Response code ignored.");
 
@@ -56,5 +55,17 @@ public async Task<bool> GetIsSupportedAsync(Uri uri)
 
             return supported;
         }
+
+        private HttpClient _httpClient;
+        private HttpClient HttpClient => _httpClient ?? (_httpClient = _httpFactory.CreateClient());
+
+        #region IDisposable
+
+        public void Dispose()
+        {
+            _httpClient?.Dispose();
+        }
+
+        #endregion
     }
 }

src/Microsoft.Git.CredentialManager/Constants.cs

@@ -20,21 +20,12 @@ public static class EnvironmentVariables
 
         public static class Http
         {
+            public const string WwwAuthenticateBasicScheme     = "Basic";
             public const string WwwAuthenticateBearerScheme    = "Bearer";
             public const string WwwAuthenticateNegotiateScheme = "Negotiate";
             public const string WwwAuthenticateNtlmScheme      = "NTLM";
 
             public const string MimeTypeJson = "application/json";
-
-            public static Header AcceptHeader(string value)
-            {
-                return new Header("Accept", new[] {value});
-            }
-
-            public static Header AuthorizationBearerHeader(string bearerToken)
-            {
-                return new Header("Authorization", new[] {$"{WwwAuthenticateBearerScheme} {bearerToken}"});
-            }
         }
 
         /// <summary>

src/Microsoft.Git.CredentialManager/GenericHostProvider.cs

@@ -71,6 +71,12 @@ public override async Task<GitCredential> CreateCredentialAsync(InputArguments i
             return _basicAuth.GetCredentials(uri.AbsoluteUri, uri.UserInfo);
         }
 
+        protected override void Dispose(bool disposing)
+        {
+            _winAuth.Dispose();
+            base.Dispose(disposing);
+        }
+
         #endregion
 
         #region Helpers

src/Microsoft.Git.CredentialManager/HostProvider.cs

@@ -1,13 +1,15 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
+
+using System;
 using System.Threading.Tasks;
 
 namespace Microsoft.Git.CredentialManager
 {
     /// <summary>
     /// Represents a particular Git hosting service and provides for the creation of credentials to access the remote.
     /// </summary>
-    public interface IHostProvider
+    public interface IHostProvider : IDisposable
     {
         /// <summary>
         /// Name of the hosting provider.
@@ -60,5 +62,22 @@ protected HostProvider(ICommandContext context)
         public abstract string GetCredentialKey(InputArguments input);
 
         public abstract Task<GitCredential> CreateCredentialAsync(InputArguments input);
+
+        /// <summary>
+        /// Called when the application is being terminated. Clean up and release any resources.
+        /// </summary>
+        /// <param name="disposing">True if the instance is being disposed, false if being finalized.</param>
+        protected virtual void Dispose(bool disposing) { }
+
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
+
+        ~HostProvider()
+        {
+            Dispose(false);
+        }
     }
 }

src/Microsoft.Git.CredentialManager/HostProviderRegistry.cs

@@ -10,12 +10,16 @@ namespace Microsoft.Git.CredentialManager
     /// Represents a collection of <see cref="IHostProvider"/>s which are selected based on Git credential query
     /// <see cref="InputArguments"/>.
     /// </summary>
-    public interface IHostProviderRegistry
+    /// <remarks>
+    /// All registered <see cref="IHostProvider"/>s will be disposed when this <see cref="IHostProviderRegistry"/> is disposed.
+    /// </remarks>
+    public interface IHostProviderRegistry : IDisposable
     {
         /// <summary>
         /// Add the given <see cref="IHostProvider"/>(s) to this registry.
         /// </summary>
         /// <param name="hostProviders">A collection of providers to register.</param>
+        /// <remarks>Providers will be disposed of when this registry instance is disposed itself.</remarks>
         void Register(params IHostProvider[] hostProviders);
 
         /// <summary>
@@ -56,5 +60,14 @@ public IHostProvider GetProvider(InputArguments input)
 
             return provider;
         }
+
+        public void Dispose()
+        {
+            // Dispose of all registered providers to give them a chance to clean up and release any resources
+            foreach (IHostProvider provider in _hostProviders)
+            {
+                provider.Dispose();
+            }
+        }
     }
 }

src/Microsoft.Git.CredentialManager/HttpClientFactory.cs

@@ -6,37 +6,40 @@
 namespace Microsoft.Git.CredentialManager
 {
     /// <summary>
-    /// Acquires <see cref="HttpClient"/>s that have been configured for use in Git Credential Manager.
+    /// Constructs <see cref="HttpClient"/>s that have been configured for use in Git Credential Manager.
     /// </summary>
     public interface IHttpClientFactory
     {
         /// <summary>
-        /// Get an instance of <see cref="HttpClient"/> with default request headers set.
+        /// Get a new instance of <see cref="HttpClient"/> with default request headers set.
         /// </summary>
-        /// <returns>Client with default headers.</returns>
-        HttpClient GetClient();
+        /// <remarks>
+        /// Callers should reuse instances of <see cref="HttpClient"/> returned from this method as long
+        /// as they are needed, rather than repeatably call this method to create new ones.
+        /// <para/>
+        /// Creating a new <see cref="HttpClient"/> consumes one free socket which may not be released
+        /// by the Operating System until sometime after the client is disposed, leading to possible free
+        /// socket exhaustion.
+        /// </remarks>
+        /// <returns>New client instance with default headers.</returns>
+        HttpClient CreateClient();
     }
 
     public class HttpClientFactory : IHttpClientFactory
     {
-        private HttpClient _client;
-
-        public HttpClient GetClient()
+        public HttpClient CreateClient()
         {
-            if (_client is null)
-            {
-                // Initialize a new HttpClient
-                _client = new HttpClient();
+            // Initialize a new HttpClient
+            var client = new HttpClient();
 
-                // Add default headers
-                _client.DefaultRequestHeaders.UserAgent.ParseAdd(Constants.GetHttpUserAgent());
-                _client.DefaultRequestHeaders.CacheControl = new CacheControlHeaderValue
-                {
-                    NoCache = true
-                };
-            }
+            // Add default headers
+            client.DefaultRequestHeaders.UserAgent.ParseAdd(Constants.GetHttpUserAgent());
+            client.DefaultRequestHeaders.CacheControl = new CacheControlHeaderValue
+            {
+                NoCache = true
+            };
 
-            return _client;
+            return client;
         }
     }
 }