fix: Fix NRHttpClientFactory so that it creates only one client. (#1873)

Author: jaffinito

src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs

@@ -9,6 +9,7 @@
 using System.Threading.Tasks;
 using NewRelic.Agent.Configuration;
 using NewRelic.Agent.Core.DataTransport.Client.Interfaces;
+using NewRelic.Core.Logging;
 
 namespace NewRelic.Agent.Core.DataTransport.Client
 {
@@ -25,7 +26,9 @@ public NRHttpClient(IWebProxy proxy, IConfiguration configuration) : base(proxy)
             _configuration = configuration;
 
             // set the default timeout to "infinite", but specify the configured collector timeout as the actual timeout for SendAsync() calls
-            var httpClient = new HttpClient(new HttpClientHandler { Proxy = proxy }, true) {Timeout = System.Threading.Timeout.InfiniteTimeSpan};
+            var httpHandler = new HttpClientHandler { Proxy = proxy };
+            Log.InfoFormat("Current HttpClientHandler TLS Configuration (HttpClientHandler.SslProtocols): {0}", httpHandler.SslProtocols.ToString());
+            var httpClient = new HttpClient(httpHandler, true) {Timeout = System.Threading.Timeout.InfiniteTimeSpan};
             _httpClientWrapper = new HttpClientWrapper(httpClient, (int)configuration.CollectorTimeout);
         }
 

src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClientFactory.cs

@@ -17,18 +17,18 @@ public class NRHttpClientFactory : IHttpClientFactory
     {
         private IHttpClient _httpClient;
 
+        private bool? _hasProxy;
+
         public IHttpClient CreateClient(IWebProxy proxy, IConfiguration configuration)
         {
-            if (proxy != null)
-            {
-                Interlocked.CompareExchange(ref _httpClient, new NRHttpClient(proxy, configuration), null);
-            }
-            else
+            var proxyRequired = (proxy != null);
+            if (_httpClient != null && (_hasProxy == proxyRequired))
             {
-                Interlocked.CompareExchange(ref _httpClient, new NRHttpClient(null,configuration), null);
+                return _httpClient;
             }
 
-            return _httpClient;
+            _hasProxy = proxyRequired;
+            return _httpClient = new NRHttpClient(proxy, configuration);
         }
     }
 }

tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientFactoryTests.cs

@@ -0,0 +1,81 @@
+// Copyright 2020 New Relic, Inc. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+#if !NETFRAMEWORK
+using System;
+using System.Net;
+using System.Net.Http;
+using System.Threading.Tasks;
+using NewRelic.Agent.Configuration;
+using NewRelic.Agent.Core.DataTransport.Client.Interfaces;
+using NUnit.Framework;
+using Telerik.JustMock;
+using Telerik.JustMock.Helpers;
+
+namespace NewRelic.Agent.Core.DataTransport.Client
+{
+    internal class NRHttpClientFactoryTests
+    {
+        private IConfiguration _mockConfiguration;
+        private IWebProxy _mockProxy;
+        private IHttpClientFactory _httpClientFactory;
+
+        [SetUp]
+        public void SetUp()
+        {
+            _mockConfiguration = Mock.Create<IConfiguration>();
+            Mock.Arrange(() => _mockConfiguration.AgentLicenseKey).Returns("12345");
+            Mock.Arrange(() => _mockConfiguration.AgentRunId).Returns("123");
+            Mock.Arrange(() => _mockConfiguration.CollectorMaxPayloadSizeInBytes).Returns(int.MaxValue);
+
+            _mockProxy = Mock.Create<IWebProxy>();
+
+            _httpClientFactory = new NRHttpClientFactory();
+        }
+
+        [Test]
+        public void CreateClient_NotNull()
+        {
+            var client = _httpClientFactory.CreateClient(null, _mockConfiguration);
+
+            Assert.That(client, Is.Not.Null);
+        }
+
+        [Test]
+        public void CreateClient_NoProxy_ReturnsSameClient()
+        {
+            var clientA = _httpClientFactory.CreateClient(null, _mockConfiguration);
+            var clientB = _httpClientFactory.CreateClient(null, _mockConfiguration);
+
+            Assert.That(clientA == clientB);
+        }
+
+        [Test]
+        public void CreateClient_Proxy_ReturnsSameClient()
+        {
+            var clientA = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration);
+            var clientB = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration);
+
+            Assert.That(clientA == clientB);
+        }
+
+        [Test]
+        public void CreateClient_NoProxyToProxy_ReturnsNewClient()
+        {
+            var clientA = _httpClientFactory.CreateClient(null, _mockConfiguration);
+            var clientB = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration);
+
+            Assert.That(clientA != clientB);
+        }
+
+        [Test]
+        public void CreateClient_ProxyToNoProxy_ReturnsNewClient()
+        {
+            var clientA = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration);
+            var clientB = _httpClientFactory.CreateClient(null, _mockConfiguration);
+
+            Assert.That(clientA != clientB);
+        }
+    }
+}
+#endif