From b43ad94d7b0eef87f8d3e1ac432ffa85bf56f525 Mon Sep 17 00:00:00 2001 From: Tim Seaward Date: Mon, 17 Jun 2019 10:39:36 +0100 Subject: [PATCH] Add usings to HttpResponses --- CondenserDotNet.sln | 1 + releasenotes/4.1.13.props | 7 ++ .../Leadership/LeaderWatcher.cs | 52 ++++++++------- .../Leadership/LeaderWatcherNew.cs | 50 +++++++------- .../Services/ServiceRegistry.cs | 14 ++-- .../Services/ServiceWatcher.cs | 38 ++++++----- .../Consul/ConsulConfigSource.cs | 66 ++++++++++--------- src/CondenserDotNet.Core/HttpUtils.cs | 28 +++----- .../ConsulRouteSource.cs | 30 +++++---- .../AuthenticationMiddlewareFacts.cs | 8 ++- version.props | 2 +- 11 files changed, 156 insertions(+), 140 deletions(-) create mode 100644 releasenotes/4.1.13.props diff --git a/CondenserDotNet.sln b/CondenserDotNet.sln index f4f3ee9..8903f4b 100644 --- a/CondenserDotNet.sln +++ b/CondenserDotNet.sln @@ -87,6 +87,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Release Notes", "Release No releasenotes\4.1.1.props = releasenotes\4.1.1.props releasenotes\4.1.10.props = releasenotes\4.1.10.props releasenotes\4.1.11.props = releasenotes\4.1.11.props + releasenotes\4.1.13.props = releasenotes\4.1.13.props releasenotes\4.1.3.props = releasenotes\4.1.3.props releasenotes\4.1.4.props = releasenotes\4.1.4.props releasenotes\4.1.9.props = releasenotes\4.1.9.props diff --git a/releasenotes/4.1.13.props b/releasenotes/4.1.13.props new file mode 100644 index 0000000..5d47f2b --- /dev/null +++ b/releasenotes/4.1.13.props @@ -0,0 +1,7 @@ + + + +* Fix potential leaks by ensuring HttpResponses are disposed correctly + + + diff --git a/src/CondenserDotNet.Client/Leadership/LeaderWatcher.cs b/src/CondenserDotNet.Client/Leadership/LeaderWatcher.cs index 107ee1f..6953633 100644 --- a/src/CondenserDotNet.Client/Leadership/LeaderWatcher.cs +++ b/src/CondenserDotNet.Client/Leadership/LeaderWatcher.cs @@ -69,33 +69,35 @@ private async Task TryForElection() for (var i = 0; i < 10; i++) { CondenserEventSource.Log.LeadershipSessionGetStatus(_keyToWatch); - leaderResult = await _serviceManager.Client.GetAsync($"{KeyPath}{_keyToWatch}?index={_consulIndex}"); - if (!leaderResult.IsSuccessStatusCode) + using (leaderResult = await _serviceManager.Client.GetAsync($"{KeyPath}{_keyToWatch}?index={_consulIndex}")) { - _currentLeaderEvent.Reset(); - _electedLeaderEvent.Reset(); - //error so return to create session - return; + if (!leaderResult.IsSuccessStatusCode) + { + _currentLeaderEvent.Reset(); + _electedLeaderEvent.Reset(); + //error so return to create session + return; + } + var kv = JsonConvert.DeserializeObject(await leaderResult.Content.ReadAsStringAsync()); + if (string.IsNullOrWhiteSpace(kv[0].Session)) + { + _currentLeaderEvent.Reset(); + _electedLeaderEvent.Reset(); + break; + } + var infoService = JsonConvert.DeserializeObject(kv[0].ValueFromBase64()); + _currentLeaderEvent.Set(infoService); + _callback?.Invoke(infoService); + if (Guid.Parse(kv[0].Session) == _sessionId) + { + _electedLeaderEvent.Set(true); + } + else + { + _electedLeaderEvent.Reset(); + } + _consulIndex = leaderResult.GetConsulIndex(); } - var kv = JsonConvert.DeserializeObject(await leaderResult.Content.ReadAsStringAsync()); - if (string.IsNullOrWhiteSpace(kv[0].Session)) - { - _currentLeaderEvent.Reset(); - _electedLeaderEvent.Reset(); - break; - } - var infoService = JsonConvert.DeserializeObject(kv[0].ValueFromBase64()); - _currentLeaderEvent.Set(infoService); - _callback?.Invoke(infoService); - if (Guid.Parse(kv[0].Session) == _sessionId) - { - _electedLeaderEvent.Set(true); - } - else - { - _electedLeaderEvent.Reset(); - } - _consulIndex = leaderResult.GetConsulIndex(); } } } diff --git a/src/CondenserDotNet.Client/Leadership/LeaderWatcherNew.cs b/src/CondenserDotNet.Client/Leadership/LeaderWatcherNew.cs index e2f9ccb..6f10e19 100644 --- a/src/CondenserDotNet.Client/Leadership/LeaderWatcherNew.cs +++ b/src/CondenserDotNet.Client/Leadership/LeaderWatcherNew.cs @@ -104,35 +104,37 @@ private async Task WaitForLeadershipChange() { while(true) { - var leaderResult = await _serviceManager.Client.GetAsync($"{KeyPath}{_keyToWatch}?index={_consulIndex}"); - if(!leaderResult.IsSuccessStatusCode) + using (var leaderResult = await _serviceManager.Client.GetAsync($"{KeyPath}{_keyToWatch}?index={_consulIndex}")) { - //Lock deleted - if(leaderResult.StatusCode == System.Net.HttpStatusCode.NotFound) + if (!leaderResult.IsSuccessStatusCode) { - _electedLeaderEvent.Reset(); + //Lock deleted + if (leaderResult.StatusCode == System.Net.HttpStatusCode.NotFound) + { + _electedLeaderEvent.Reset(); + _currentInfoService.Reset(); + return; + } + await Task.Delay(500); + continue; + } + var kv = JsonConvert.DeserializeObject(await leaderResult.Content.ReadAsStringAsync()); + if (string.IsNullOrWhiteSpace(kv[0].Session)) + { + //no one has leadership _currentInfoService.Reset(); + _electedLeaderEvent.Reset(); + return; + } + var infoService = JsonConvert.DeserializeObject(kv[0].ValueFromBase64()); + _currentInfoService.Set(infoService); + _consulIndex = leaderResult.GetConsulIndex(); + _callback?.Invoke(infoService); + if (await _sessionIdTask != new Guid(kv[0].Session)) + { + _electedLeaderEvent.Reset(); return; } - await Task.Delay(500); - continue; - } - var kv = JsonConvert.DeserializeObject(await leaderResult.Content.ReadAsStringAsync()); - if(string.IsNullOrWhiteSpace(kv[0].Session)) - { - //no one has leadership - _currentInfoService.Reset(); - _electedLeaderEvent.Reset(); - return; - } - var infoService = JsonConvert.DeserializeObject(kv[0].ValueFromBase64()); - _currentInfoService.Set(infoService); - _consulIndex = leaderResult.GetConsulIndex(); - _callback?.Invoke(infoService); - if(await _sessionIdTask != new Guid(kv[0].Session)) - { - _electedLeaderEvent.Reset(); - return; } } } diff --git a/src/CondenserDotNet.Client/Services/ServiceRegistry.cs b/src/CondenserDotNet.Client/Services/ServiceRegistry.cs index 3e7eff2..fb5b1e2 100644 --- a/src/CondenserDotNet.Client/Services/ServiceRegistry.cs +++ b/src/CondenserDotNet.Client/Services/ServiceRegistry.cs @@ -31,14 +31,16 @@ public async Task> GetAvailableServicesAsync() public async Task> GetAvailableServicesWithTagsAsync() { - var result = await _client.GetAsync(HttpUtils.ServiceCatalogUrl); - if (!result.IsSuccessStatusCode) + using (var result = await _client.GetAsync(HttpUtils.ServiceCatalogUrl)) { - return null; + if (!result.IsSuccessStatusCode) + { + return null; + } + var content = await result.Content.ReadAsStringAsync(); + var serviceList = JsonConvert.DeserializeObject>(content); + return serviceList; } - var content = await result.Content.ReadAsStringAsync(); - var serviceList = JsonConvert.DeserializeObject>(content); - return serviceList; } public Task GetServiceInstanceAsync(string serviceName) diff --git a/src/CondenserDotNet.Client/Services/ServiceWatcher.cs b/src/CondenserDotNet.Client/Services/ServiceWatcher.cs index b3a7225..6446884 100644 --- a/src/CondenserDotNet.Client/Services/ServiceWatcher.cs +++ b/src/CondenserDotNet.Client/Services/ServiceWatcher.cs @@ -89,29 +89,31 @@ private async Task WatcherLoop(HttpClient client) var consulIndex = "0"; while (!_cancelationToken.Token.IsCancellationRequested) { - var result = await client.GetAsync(_url + consulIndex, _cancelationToken.Token); - if (!result.IsSuccessStatusCode) + using (var result = await client.GetAsync(_url + consulIndex, _cancelationToken.Token)) { - if (_state == WatcherState.UsingLiveValues) + if (!result.IsSuccessStatusCode) { - _state = WatcherState.UsingCachedValues; + if (_state == WatcherState.UsingLiveValues) + { + _state = WatcherState.UsingCachedValues; + } + await Task.Delay(1000); + continue; } - await Task.Delay(1000); - continue; - } - var newConsulIndex = result.GetConsulIndex(); - if (newConsulIndex == consulIndex) - { + var newConsulIndex = result.GetConsulIndex(); + if (newConsulIndex == consulIndex) + { + continue; + } + consulIndex = newConsulIndex; + var content = await result.Content.ReadAsStringAsync(); + var instance = JsonConvert.DeserializeObject>(content); + Volatile.Write(ref _instances, instance); + _listCallback?.Invoke(instance); + _state = WatcherState.UsingLiveValues; + _completionSource.TrySetResult(true); continue; } - consulIndex = newConsulIndex; - var content = await result.Content.ReadAsStringAsync(); - var instance = JsonConvert.DeserializeObject>(content); - Volatile.Write(ref _instances, instance); - _listCallback?.Invoke(instance); - _state = WatcherState.UsingLiveValues; - _completionSource.TrySetResult(true); - continue; } } catch (Exception ex) diff --git a/src/CondenserDotNet.Configuration/Consul/ConsulConfigSource.cs b/src/CondenserDotNet.Configuration/Consul/ConsulConfigSource.cs index 167d1d9..9a1a317 100644 --- a/src/CondenserDotNet.Configuration/Consul/ConsulConfigSource.cs +++ b/src/CondenserDotNet.Configuration/Consul/ConsulConfigSource.cs @@ -63,15 +63,17 @@ public async Task GetKeysAsync(string keyPath) try { CondenserEventSource.Log.ConfigurationGetKeysRecursive(keyPath); - var response = await _httpClient.GetAsync($"{ConsulKeyPath}{keyPath}?recurse"); - if (!response.IsSuccessStatusCode) + using (var response = await _httpClient.GetAsync($"{ConsulKeyPath}{keyPath}?recurse")) { - _logger?.LogWarning("We didn't get a succesful response from consul code was {code}", response.StatusCode); - return new KeyOperationResult() { Success = false, Dictionary = null }; + if (!response.IsSuccessStatusCode) + { + _logger?.LogWarning("We didn't get a succesful response from consul code was {code}", response.StatusCode); + return new KeyOperationResult() { Success = false, Dictionary = null }; + } + + var dictionary = await BuildDictionaryAsync(keyPath, response); + return new KeyOperationResult() { Success = true, Dictionary = dictionary }; } - - var dictionary = await BuildDictionaryAsync(keyPath, response); - return new KeyOperationResult() { Success = true, Dictionary = dictionary }; } catch (Exception ex) { @@ -86,17 +88,19 @@ public async Task GetKeysAsync(string keyPath) try { CondenserEventSource.Log.ConfigurationGetKey(keyPath); - var response = await _httpClient.GetAsync($"{ConsulKeyPath}{keyPath}"); - if (!response.IsSuccessStatusCode) + using (var response = await _httpClient.GetAsync($"{ConsulKeyPath}{keyPath}")) { - _logger?.LogWarning("We didn't get a successful response from consul code was {code}", response.StatusCode); - return (false, null); + if (!response.IsSuccessStatusCode) + { + _logger?.LogWarning("We didn't get a successful response from consul code was {code}", response.StatusCode); + return (false, null); + } + + var content = await response.Content.ReadAsStringAsync(); + var keys = JsonConvert.DeserializeObject(content); + if (keys.Length != 1) return (false, null); + return (true, keys[0].Value); } - - var content = await response.Content.ReadAsStringAsync(); - var keys = JsonConvert.DeserializeObject(content); - if (keys.Length != 1) return (false, null); - return (true, keys[0].Value); } catch (Exception exception) { @@ -113,23 +117,25 @@ public async Task TryWatchKeysAsync(string keyPath, object s try { CondenserEventSource.Log.ConfigurationWatchKey(keyPath); - var response = await _httpClient.GetAsync(url + consulState.ConsulIndex, _disposed.Token); - var newConsulIndex = response.GetConsulIndex(); - - if (!response.IsSuccessStatusCode) - { - consulState.ConsulIndex = newConsulIndex; - return default; - } - - if (newConsulIndex == consulState.ConsulIndex) + using (var response = await _httpClient.GetAsync(url + consulState.ConsulIndex, _disposed.Token)) { + var newConsulIndex = response.GetConsulIndex(); + + if (!response.IsSuccessStatusCode) + { + consulState.ConsulIndex = newConsulIndex; + return default; + } + + if (newConsulIndex == consulState.ConsulIndex) + { + consulState.ConsulIndex = newConsulIndex; + return default; + } consulState.ConsulIndex = newConsulIndex; - return default; + var dictionary = await BuildDictionaryAsync(keyPath, response); + return new KeyOperationResult() { Success = true, Dictionary = dictionary }; } - consulState.ConsulIndex = newConsulIndex; - var dictionary = await BuildDictionaryAsync(keyPath, response); - return new KeyOperationResult() { Success = true, Dictionary = dictionary }; } catch (Exception ex) { diff --git a/src/CondenserDotNet.Core/HttpUtils.cs b/src/CondenserDotNet.Core/HttpUtils.cs index a5b316f..0c2484a 100644 --- a/src/CondenserDotNet.Core/HttpUtils.cs +++ b/src/CondenserDotNet.Core/HttpUtils.cs @@ -39,21 +39,11 @@ public static HttpClient CreateClient(IConsulAclProvider aclProvider, string age var port = agentPort ?? DefaultPort; var uri = new UriBuilder("http", host, port); - HttpClient client; -#if NET452 - System.Net.ServicePointManager.DefaultConnectionLimit = 50; - client = new HttpClient() + var client = new HttpClient(new HttpClientHandler() { MaxConnectionsPerServer = 50 }) { BaseAddress = uri.Uri, Timeout = DefaultTimeout }; -#else - client = new HttpClient(new HttpClientHandler() { MaxConnectionsPerServer = 50 }) - { - BaseAddress = uri.Uri, - Timeout = DefaultTimeout - }; -#endif var token = aclProvider?.GetAclToken(); if(!string.IsNullOrEmpty(token)) { @@ -62,17 +52,17 @@ public static HttpClient CreateClient(IConsulAclProvider aclProvider, string age return client; } - public static Task GetObject(this HttpContent content) => - content.ReadAsStringAsync().ContinueWith(sTask => + public static async Task GetObject(this HttpContent content) { - return JsonConvert.DeserializeObject(sTask.Result); - }); + var result = await content.ReadAsStringAsync(); + return JsonConvert.DeserializeObject(result); + } - public static Task GetAsync(this HttpClient client, string uri) => - client.GetStringAsync(uri).ContinueWith(resultTask => + public static async Task GetAsync(this HttpClient client, string uri) { - return JsonConvert.DeserializeObject(resultTask.Result); - }); + var result = await client.GetStringAsync(uri); + return JsonConvert.DeserializeObject(result); + } public static StringContent GetStringContent(string stringForContent) { diff --git a/src/CondenserDotNet.Server/ConsulRouteSource.cs b/src/CondenserDotNet.Server/ConsulRouteSource.cs index 14c1b58..b1c7203 100644 --- a/src/CondenserDotNet.Server/ConsulRouteSource.cs +++ b/src/CondenserDotNet.Server/ConsulRouteSource.cs @@ -34,25 +34,27 @@ public ConsulRouteSource(CondenserConfiguration config, public async Task TryGetHealthChecksAsync() { _logger?.LogInformation("Looking for health changes with index {index}", _lastConsulIndex); - var result = await _client.GetAsync(_healthCheckUri + _lastConsulIndex.ToString(), _cancel.Token); - if (!result.IsSuccessStatusCode) + using (var result = await _client.GetAsync(_healthCheckUri + _lastConsulIndex.ToString(), _cancel.Token)) { - _logger?.LogWarning("Retrieved a response that was not success when getting the health status code was {code}", result.StatusCode); - return new GetHealthCheckResult() { Checks = EmptyChecks, Success = false }; - } - var newConsulIndex = result.GetConsulIndex(); + if (!result.IsSuccessStatusCode) + { + _logger?.LogWarning("Retrieved a response that was not success when getting the health status code was {code}", result.StatusCode); + return new GetHealthCheckResult() { Checks = EmptyChecks, Success = false }; + } + var newConsulIndex = result.GetConsulIndex(); - if (newConsulIndex == _lastConsulIndex) - { - return new GetHealthCheckResult() { Success = false, Checks = EmptyChecks }; - } + if (newConsulIndex == _lastConsulIndex) + { + return new GetHealthCheckResult() { Success = false, Checks = EmptyChecks }; + } - _lastConsulIndex = newConsulIndex ; + _lastConsulIndex = newConsulIndex; - _logger?.LogInformation("Got new set of health information new index is {index}", _lastConsulIndex); + _logger?.LogInformation("Got new set of health information new index is {index}", _lastConsulIndex); - var checks = await result.Content.GetObject(); - return new GetHealthCheckResult() { Success = true, Checks = checks }; + var checks = await result.Content.GetObject(); + return new GetHealthCheckResult() { Success = true, Checks = checks }; + } } public Task GetServiceInstancesAsync(string serviceName) => _client.GetAsync(_serviceLookupUri + serviceName); diff --git a/test/Condenser.Tests.Integration/AuthenticationMiddlewareFacts.cs b/test/Condenser.Tests.Integration/AuthenticationMiddlewareFacts.cs index 77c2a22..9ef3039 100644 --- a/test/Condenser.Tests.Integration/AuthenticationMiddlewareFacts.cs +++ b/test/Condenser.Tests.Integration/AuthenticationMiddlewareFacts.cs @@ -33,9 +33,11 @@ public async Task CanAuthenticateWithNtlm() { UseDefaultCredentials = true }); - var result = await client.GetAsync($"http://localhost:55555"); - var name = await result.Content.ReadAsStringAsync(); - Assert.Equal(System.Security.Principal.WindowsIdentity.GetCurrent().Name, name); + using (var result = await client.GetAsync($"http://localhost:55555")) + { + var name = await result.Content.ReadAsStringAsync(); + Assert.Equal(System.Security.Principal.WindowsIdentity.GetCurrent().Name, name); + } } finally { diff --git a/version.props b/version.props index 3da23e0..4f7bccf 100644 --- a/version.props +++ b/version.props @@ -1,6 +1,6 @@ - 4.1.12 + 4.1.13 beta