diff --git a/src/Agent/NewRelic/Agent/Core/AgentShim.cs b/src/Agent/NewRelic/Agent/Core/AgentShim.cs index 34c269c045..bd64aa3dc7 100644 --- a/src/Agent/NewRelic/Agent/Core/AgentShim.cs +++ b/src/Agent/NewRelic/Agent/Core/AgentShim.cs @@ -21,14 +21,15 @@ static void Initialize() } private static bool _initialized = false; - private static object _initLock = new object(); + private static SemaphoreSlim _lockSemaphore = new SemaphoreSlim(1, 1); static bool TryInitialize(string method) { - if (Monitor.IsEntered(_initLock)) return false; + if (_lockSemaphore.CurrentCount == 0) return false; if (DeferInitialization(method)) return false; - lock (_initLock) + _lockSemaphore.Wait(); + try { if (!_initialized) { @@ -38,6 +39,10 @@ static bool TryInitialize(string method) return true; } + finally + { + _lockSemaphore.Release(); + } } private static HashSet _deferInitializationOnTheseMethods = new HashSet diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/CustomEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/CustomEventAggregator.cs index b974b6a017..1044d3c295 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/CustomEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/CustomEventAggregator.cs @@ -89,14 +89,16 @@ protected void InternalHarvest(string transactionId = null) var customEvents = originalCustomEvents.Where(node => node != null).Select(node => node.Data).ToList(); // if we don't have any events to publish then don't - if (customEvents.Count <= 0) - return; + var eventCount = customEvents.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(customEvents, transactionId); - var responseStatus = DataTransportService.Send(customEvents, transactionId); + HandleResponse(responseStatus, customEvents); + } - HandleResponse(responseStatus, customEvents); + Log.Finest($"Custom Event harvest finished. {eventCount} event(s) sent."); - Log.Finest("Custom Event harvest finished."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorEventAggregator.cs index 3a5d6de467..e4500a98d6 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorEventAggregator.cs @@ -99,14 +99,15 @@ protected void InternalHarvest(string transactionId = null) var eventHarvestData = new EventHarvestData(originalErrorEvents.Size, originalErrorEvents.GetAddAttemptsCount()); // if we don't have any events to publish then don't - if (aggregatedEvents.Count <= 0) - return; - - var responseStatus = DataTransportService.Send(eventHarvestData, aggregatedEvents, transactionId); + var eventCount = aggregatedEvents.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(eventHarvestData, aggregatedEvents, transactionId); - HandleResponse(responseStatus, aggregatedEvents); + HandleResponse(responseStatus, aggregatedEvents); + } - Log.Finest("Error Event harvest finished."); + Log.Finest($"Error Event harvest finished. {eventCount} event(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs index ef7c49ae9b..8384ce2363 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs @@ -79,14 +79,16 @@ protected void InternalHarvest(string transactionId = null) _readerWriterLock.ExitWriteLock(); } - if (errorTraceWireModels.Count <= 0) - return; - - var responseStatus = DataTransportService.Send(errorTraceWireModels, transactionId); + // if we don't have any events to publish then don't + var traceCount = errorTraceWireModels.Count; + if (traceCount > 0) + { + var responseStatus = DataTransportService.Send(errorTraceWireModels, transactionId); - HandleResponse(responseStatus, errorTraceWireModels); + HandleResponse(responseStatus, errorTraceWireModels); + } - Log.Finest("Error Trace harvest finished."); + Log.Finest($"Error Trace harvest finished. {traceCount} trace(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/LogEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/LogEventAggregator.cs index 6ec943ebc6..01520d6cea 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/LogEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/LogEventAggregator.cs @@ -88,28 +88,27 @@ protected void InternalHarvest(string transactionId = null) Interlocked.Add(ref _logsDroppedCount, originalLogEvents.GetAndResetDroppedItemCount()); // if we don't have any events to publish then don't - if (aggregatedEvents.Count <= 0) + var eventCount = aggregatedEvents.Count; + if (eventCount > 0) { - return; - } + // matches metadata so that utilization and this match + var hostname = !string.IsNullOrEmpty(_configuration.UtilizationFullHostName) + ? _configuration.UtilizationFullHostName + : _configuration.UtilizationHostName; - // matches metadata so that utilization and this match - var hostname = !string.IsNullOrEmpty(_configuration.UtilizationFullHostName) - ? _configuration.UtilizationFullHostName - : _configuration.UtilizationHostName; + var modelsCollection = new LogEventWireModelCollection( + _configuration.ApplicationNames.ElementAt(0), + _configuration.EntityGuid, + hostname, + _configuration.LabelsEnabled ? _labelsService.GetFilteredLabels(_configuration.LabelsExclude) : [], + aggregatedEvents); - var modelsCollection = new LogEventWireModelCollection( - _configuration.ApplicationNames.ElementAt(0), - _configuration.EntityGuid, - hostname, - _configuration.LabelsEnabled ? _labelsService.GetFilteredLabels(_configuration.LabelsExclude) : [], - aggregatedEvents); + var responseStatus = DataTransportService.Send(modelsCollection, transactionId); - var responseStatus = DataTransportService.Send(modelsCollection, transactionId); - - HandleResponse(responseStatus, aggregatedEvents); + HandleResponse(responseStatus, aggregatedEvents); + } - Log.Finest("Log Event harvest finished."); + Log.Finest($"Log Event harvest finished. {eventCount} event(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs index fd4c7d9581..f82d53ba5c 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using NewRelic.Agent.Core.DataTransport; using NewRelic.Agent.Core.Events; using NewRelic.Agent.Core.Metrics; @@ -75,12 +76,16 @@ protected void InternalHarvest(string transactionId = null) var oldMetrics = GetStatsCollectionForHarvest(); oldMetrics.MergeUnscopedStats(MetricNames.SupportabilityMetricHarvestTransmit, MetricDataWireModel.BuildCountData()); - var metricsToSend = oldMetrics.ConvertToJsonForSending(_metricNameService); + var metricsToSend = oldMetrics.ConvertToJsonForSending(_metricNameService).ToList(); - var responseStatus = DataTransportService.Send(metricsToSend, transactionId); - HandleResponse(responseStatus, metricsToSend); + var metricCount = metricsToSend.Count; + if (metricCount > 0) + { + var responseStatus = DataTransportService.Send(metricsToSend, transactionId); + HandleResponse(responseStatus, metricsToSend); + } - Log.Debug("Metric harvest finished."); + Log.Finest($"Metric harvest finished. {metricCount} metric(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/SpanEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/SpanEventAggregator.cs index 45813b29bc..6f050dcec6 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/SpanEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/SpanEventAggregator.cs @@ -123,16 +123,16 @@ protected void InternalHarvest(string transactionId = null) var eventHarvestData = new EventHarvestData(spanEventsPriorityQueue.Size, spanEventsPriorityQueue.GetAddAttemptsCount()); var wireModels = spanEventsPriorityQueue.Where(node => null != node).Select(node => node.Data).ToList(); - - // if we don't have any events to publish then don't - if (wireModels.Count <= 0) - return; - - var responseStatus = DataTransportService.Send(eventHarvestData, wireModels, transactionId); - HandleResponse(responseStatus, wireModels); + // if we don't have any events to publish then don't + var eventCount = wireModels.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(eventHarvestData, wireModels, transactionId); + HandleResponse(responseStatus, wireModels); + } - Log.Finest("Span Event harvest finished."); + Log.Finest($"Span Event harvest finished. {eventCount} event(s) sent."); } private void ReduceReservoirSize(int newSize) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/SqlTraceAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/SqlTraceAggregator.cs index f090294a65..df77a6f567 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/SqlTraceAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/SqlTraceAggregator.cs @@ -66,14 +66,15 @@ protected void InternalHarvest(string transactionId = null) .Take(_configuration.SqlTracesPerPeriod) .ToList(); - if (!slowestTraces.Any()) - return; - - var responseStatus = DataTransportService.Send(slowestTraces, transactionId); - - HandleResponse(responseStatus, slowestTraces); + // if we don't have any traces to publish then don't + var traceCount = slowestTraces.Count; + if (traceCount > 0) + { + var responseStatus = DataTransportService.Send(slowestTraces, transactionId); + HandleResponse(responseStatus, slowestTraces); + } - Log.Finest("SQL Trace harvest finished."); + Log.Finest($"SQL Trace harvest finished. {traceCount} trace(s) sent."); } private void HandleResponse(DataTransportResponseStatus responseStatus, ICollection traces) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionEventAggregator.cs index b6af92a475..3b1e65af7d 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionEventAggregator.cs @@ -96,14 +96,16 @@ protected void InternalHarvest(string transactionId = null) var eventHarvestData = new EventHarvestData(originalTransactionEvents.Size, originalTransactionEvents.GetAddAttemptsCount()); // if we don't have any events to publish then don't - if (aggregatedEvents.Count <= 0) - return; + var eventCount = aggregatedEvents.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(eventHarvestData, aggregatedEvents, transactionId); - var responseStatus = DataTransportService.Send(eventHarvestData, aggregatedEvents, transactionId); + HandleResponse(responseStatus, aggregatedEvents); + } - HandleResponse(responseStatus, aggregatedEvents); + Log.Finest($"Transaction Event harvest finished. {eventCount} event(s) sent."); - Log.Finest("Transaction Event harvest finished."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs index 7846c017c2..e338fed3cd 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs @@ -49,6 +49,8 @@ public override void Collect(TransactionTraceWireModelComponents transactionTrac protected void InternalHarvest(string transactionId = null) { + Log.Finest("Transaction Trace harvest starting."); + var traceSamples = _transactionCollectors .Where(t => t != null) .SelectMany(t => t.GetCollectedSamples()) @@ -59,13 +61,17 @@ protected void InternalHarvest(string transactionId = null) .Select(t => t.CreateWireModel()) .ToList(); - if (!traceWireModels.Any()) - return; + // if we don't have any traces to publish then don't + var traceCount = traceWireModels.Count; + if (traceCount > 0) + { + LogUnencodedTraceData(traceWireModels); - LogUnencodedTraceData(traceWireModels); + var responseStatus = DataTransportService.Send(traceWireModels, transactionId); + HandleResponse(responseStatus, traceSamples); + } - var responseStatus = DataTransportService.Send(traceWireModels, transactionId); - HandleResponse(responseStatus, traceSamples); + Log.Finest($"Transaction Trace harvest finished. {traceCount} trace(s) sent."); } private void HandleResponse(DataTransportResponseStatus responseStatus, ICollection traceSamples) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientBase.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientBase.cs index c619e8f9eb..2f8eef0ed0 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientBase.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientBase.cs @@ -66,7 +66,7 @@ protected void TestConnection() wc.DownloadString(testAddress); } #else - _lazyHttpClient.Value.GetAsync(testAddress).GetAwaiter().GetResult(); + _lazyHttpClient.Value.GetAsync(testAddress).ConfigureAwait(false).GetAwaiter().GetResult(); #endif Log.Info("Connection test to \"{0}\" succeeded", testAddress); } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs index 1779a985a5..8b17d983e6 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs @@ -1,4 +1,4 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #if !NETFRAMEWORK @@ -6,9 +6,8 @@ using System.Net.Http; using System.Threading; using System.Threading.Tasks; -using NewRelic.Agent.Configuration; -using NewRelic.Agent.Core.Config; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; +using NewRelic.Agent.Extensions.Logging; namespace NewRelic.Agent.Core.DataTransport.Client { @@ -31,19 +30,20 @@ public void Dispose() public async Task SendAsync(HttpRequestMessage message) { - var cts = new CancellationTokenSource(_timeoutMilliseconds); - return new HttpResponseMessageWrapper(await _httpClient.SendAsync(message, cts.Token)); - } - - public TimeSpan Timeout - { - get + using var cts = new CancellationTokenSource(_timeoutMilliseconds); + try { - return _httpClient.Timeout; + // .ConfigureAwait(false) is used to avoid deadlocks. + var httpResponseMessage = await _httpClient.SendAsync(message, cts.Token).ConfigureAwait(false); + return new HttpResponseMessageWrapper(httpResponseMessage); } - set + catch (Exception e) { - _httpClient.Timeout = value; + Log.Debug(cts.IsCancellationRequested + ? $"HttpClient.SendAsync() timed out after {_timeoutMilliseconds}ms." + : $"HttpClient.SendAsync() threw an unexpected exception: {e}"); + + throw; } } } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpContentWrapper.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpContentWrapper.cs index 57f55359dc..80392fbb03 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpContentWrapper.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpContentWrapper.cs @@ -22,7 +22,7 @@ public HttpContentWrapper(HttpContent httpContent) public Stream ReadAsStream() { - return _httpContent.ReadAsStreamAsync().GetAwaiter().GetResult(); + return _httpContent.ReadAsStreamAsync().ConfigureAwait(false).GetAwaiter().GetResult(); } public IHttpContentHeadersWrapper Headers => new HttpContentHeadersWrapper(_httpContent.Headers); diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpResponse.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpResponse.cs index d4632f0c0e..1d4da062a1 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpResponse.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpResponse.cs @@ -48,7 +48,7 @@ public string GetContent() using (responseStream) using (var reader = new StreamReader(responseStream, Encoding.UTF8)) { - var responseBody = reader.ReadLineAsync().GetAwaiter().GetResult(); + var responseBody = reader.ReadLineAsync().ConfigureAwait(false).GetAwaiter().GetResult(); if (responseBody != null) { diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientFactory.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientFactory.cs index 921446e134..f8509f840c 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientFactory.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientFactory.cs @@ -1,4 +1,4 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 using System.Net; @@ -8,6 +8,6 @@ namespace NewRelic.Agent.Core.DataTransport.Client.Interfaces { public interface IHttpClientFactory { - public IHttpClient CreateClient(IWebProxy proxy, IConfiguration configuration); + public IHttpClient GetOrCreateClient(IWebProxy proxy, IConfiguration configuration); } } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientWrapper.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientWrapper.cs index 708180dd9e..bbc234b7cd 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientWrapper.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientWrapper.cs @@ -11,8 +11,6 @@ namespace NewRelic.Agent.Core.DataTransport.Client.Interfaces public interface IHttpClientWrapper : IDisposable { Task SendAsync(HttpRequestMessage message); - - TimeSpan Timeout { get; set; } } } #endif diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs index ad0b445ae3..02e9f7cb20 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs @@ -6,7 +6,7 @@ using System.Net; using System.Net.Http; using System.Net.Http.Headers; -using System.Threading.Tasks; +using System.Reflection; using NewRelic.Agent.Configuration; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; using NewRelic.Agent.Extensions.Logging; @@ -26,22 +26,58 @@ 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 httpHandler = new HttpClientHandler { Proxy = proxy }; - Log.Info("Current HttpClientHandler TLS Configuration (HttpClientHandler.SslProtocols): {0}", httpHandler.SslProtocols.ToString()); - var httpClient = new HttpClient(httpHandler, true) {Timeout = System.Threading.Timeout.InfiniteTimeSpan}; + var httpHandler = GetHttpHandler(proxy); + + var httpClient = new HttpClient(httpHandler, true) { Timeout = System.Threading.Timeout.InfiniteTimeSpan }; _httpClientWrapper = new HttpClientWrapper(httpClient, (int)configuration.CollectorTimeout); } - public override IHttpResponse Send(IHttpRequest request) + private dynamic GetHttpHandler(IWebProxy proxy) { - try + // check whether the application is running .NET 6 or later + if (System.Environment.Version.Major >= 6) { - var req = new HttpRequestMessage + try + { + var pooledConnectionLifetime = TimeSpan.FromMinutes(5); // an in-use connection will be closed and recycled after 5 minutes + var pooledConnectionIdleTimeout = TimeSpan.FromMinutes(1); // a connection that is idle for 1 minute will be closed and recycled + + Log.Info($"Creating a SocketsHttpHandler with PooledConnectionLifetime set to {pooledConnectionLifetime} and PooledConnectionIdleTimeout set to {pooledConnectionIdleTimeout}"); + + // use reflection to create a SocketsHttpHandler instance and set the PooledConnectionLifetime to 1 minute + var assembly = Assembly.Load("System.Net.Http"); + var handlerType = assembly.GetType("System.Net.Http.SocketsHttpHandler"); + dynamic handler = Activator.CreateInstance(handlerType); + + handler.PooledConnectionLifetime = pooledConnectionLifetime; + handler.PooledConnectionIdleTimeout = pooledConnectionIdleTimeout; + + handler.Proxy = proxy; + + Log.Info("Current SocketsHttpHandler TLS Configuration (SocketsHttpHandler.SslOptions): {0}", handler.SslOptions.EnabledSslProtocols); + return handler; + } + catch (Exception e) { - RequestUri = request.Uri, - Method = _configuration.PutForDataSend ? HttpMethod.Put : HttpMethod.Post - }; + Log.Info(e, "Application is running .NET 6+ but an exception occurred trying to create SocketsHttpHandler. Falling back to HttpHandler."); + } + } + + // if the application is not running .NET 6 or later, use the default HttpClientHandler + var httpClientHandler = new HttpClientHandler { Proxy = proxy }; + Log.Info("Current HttpClientHandler TLS Configuration (HttpClientHandler.SslProtocols): {0}", httpClientHandler.SslProtocols.ToString()); + + return httpClientHandler; + } + + public override IHttpResponse Send(IHttpRequest request) + { + try + { + using var req = new HttpRequestMessage(); + req.RequestUri = request.Uri; + req.Method = _configuration.PutForDataSend ? HttpMethod.Put : HttpMethod.Post; req.Headers.Add("User-Agent", $"NewRelic-DotNetAgent/{AgentInstallConfiguration.AgentVersion}"); req.Headers.Add("Connection", "keep-alive"); req.Headers.Add("Keep-Alive", "true"); @@ -52,7 +88,7 @@ public override IHttpResponse Send(IHttpRequest request) req.Headers.Add(header.Key, header.Value); } - var content = new ByteArrayContent(request.Content.PayloadBytes); + using var content = new ByteArrayContent(request.Content.PayloadBytes); var encoding = request.Content.IsCompressed ? request.Content.CompressionType.ToLower() : "identity"; content.Headers.ContentType = new MediaTypeHeaderValue(request.Content.ContentType); content.Headers.Add("Content-Encoding", encoding); @@ -65,7 +101,10 @@ public override IHttpResponse Send(IHttpRequest request) req.Content.Headers.Add(contentHeader.Key, contentHeader.Value); } - var response = _httpClientWrapper.SendAsync(req).GetAwaiter().GetResult(); + Log.Finest($"Request({request.RequestGuid}: Sending"); + // .ConfigureAwait(false) is used to avoid deadlocks. + var response = _httpClientWrapper.SendAsync(req).ConfigureAwait(false).GetAwaiter().GetResult(); + Log.Finest($"Request({request.RequestGuid}: Sent"); var httpResponse = new HttpResponse(request.RequestGuid, response); return httpResponse; diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClientFactory.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClientFactory.cs index a44c48ba6f..59f5035f0f 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClientFactory.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClientFactory.cs @@ -1,9 +1,8 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #if !NETFRAMEWORK using System.Net; -using System.Threading; using NewRelic.Agent.Configuration; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; @@ -19,7 +18,7 @@ public class NRHttpClientFactory : IHttpClientFactory private bool? _hasProxy; - public IHttpClient CreateClient(IWebProxy proxy, IConfiguration configuration) + public IHttpClient GetOrCreateClient(IWebProxy proxy, IConfiguration configuration) { var proxyRequired = (proxy != null); if (_httpClient != null && (_hasProxy == proxyRequired)) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/WebRequestHttpClientFactory.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/WebRequestHttpClientFactory.cs index 803ac67b5c..146b3d5fb2 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/WebRequestHttpClientFactory.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/WebRequestHttpClientFactory.cs @@ -1,4 +1,4 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #if NETFRAMEWORK using System.Net; @@ -18,7 +18,7 @@ public WebRequestHttpClientFactory() { } - public IHttpClient CreateClient(IWebProxy proxy, IConfiguration configuration) + public IHttpClient GetOrCreateClient(IWebProxy proxy, IConfiguration configuration) { return new NRWebRequestClient(proxy, configuration); } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs index eb67f8c641..e92f15b655 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs @@ -17,7 +17,10 @@ using System.IO; using System.Linq; using System.Net; +using Newtonsoft.Json; +#if NETFRAMEWORK using System.Web; +#endif namespace NewRelic.Agent.Core.DataTransport { @@ -323,7 +326,7 @@ private string GetIdentifier() var appNames = string.Join(":", _configuration.ApplicationNames.ToArray()); #if NETSTANDARD2_0 - return $"{Path.GetFileName(_processStatic.GetCurrentProcess().MainModuleFileName)}{appNames}"; + return $"{Path.GetFileName(_processStatic.GetCurrentProcess().MainModuleFileName)}{appNames}"; #else return HttpRuntime.AppDomainAppId != null @@ -420,34 +423,36 @@ private T SendNonDataRequest(string method, params object[] data) private T SendDataOverWire(ICollectorWire wire, string method, params object[] data) { var requestGuid = Guid.NewGuid(); + string serializedData; try { - var serializedData = _serializer.Serialize(data); - try - { - var responseBody = wire.SendData(method, _connectionInfo, serializedData, requestGuid); - return ParseResponse(responseBody); - } - catch (DataTransport.HttpException ex) - { - Log.Debug(ex, "Request({0}): Received a {1} {2} response invoking method \"{3}\" with payload \"{4}\"", requestGuid, (int)ex.StatusCode, ex.StatusCode, method, serializedData); + serializedData = _serializer.Serialize(data); + } + catch (JsonSerializationException ex) + { + Log.Debug("Request({0}): Exception occurred serializing request data: {1}", requestGuid, ex); // log message only since exception is rethrown + throw; + } - if (ex.StatusCode == HttpStatusCode.Gone) - { - Log.Info(ex, "Request({0}): The server has requested that the agent disconnect. The agent is shutting down.", requestGuid); - } + try + { + var responseBody = wire.SendData(method, _connectionInfo, serializedData, requestGuid); + return ParseResponse(responseBody); + } + catch (DataTransport.HttpException ex) + { + Log.Debug("Request({0}): Received a {1} {2} response invoking method \"{3}\" with payload \"{4}\"", requestGuid, (int)ex.StatusCode, ex.StatusCode, method, serializedData); - throw; - } - catch (Exception ex) + if (ex.StatusCode == HttpStatusCode.Gone) { - Log.Debug(ex, "Request({0}): An error occurred invoking method \"{1}\" with payload \"{2}\": {3}", requestGuid, method, serializedData, ex); - throw; + Log.Info("Request({0}): The server has requested that the agent disconnect. The agent is shutting down.", requestGuid); } + + throw; } catch (Exception ex) { - Log.Debug(ex, "Request({0}): Exception occurred serializing request data: {1}", requestGuid, ex); + Log.Debug("Request({0}): An error occurred invoking method \"{1}\" with payload \"{2}\": {3}", requestGuid, method, serializedData, ex); // log message only since exception is rethrown throw; } } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs index e31d2c5a34..11f0b1e2e8 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs @@ -2,14 +2,14 @@ // SPDX-License-Identifier: Apache-2.0 using NewRelic.Agent.Core.Events; -using NewRelic.Agent.Core.DataTransport; using NewRelic.Agent.Core.Time; using NewRelic.Agent.Core.Utilities; using NewRelic.Agent.Extensions.Logging; using System; using System.IO; -using System.Linq; using System.Net; +using System.Threading; + #if !NETFRAMEWORK using System.Net.Http; #endif @@ -38,7 +38,7 @@ public class ConnectionManager : ConfigurationBasedService, IConnectionManager private readonly IScheduler _scheduler; private int _connectionAttempt = 0; private bool _started; - private readonly object _syncObject = new object(); + private readonly SemaphoreSlim _lockSemaphore = new SemaphoreSlim(1, 1); private bool _runtimeConfigurationUpdated = false; public ConnectionManager(IConnectionHandler connectionHandler, IScheduler scheduler) @@ -70,7 +70,8 @@ private void Start() if (_started) return; - lock (_syncObject) + _lockSemaphore.Wait(); + try { // Second, a thread-safe check inside the blocking code block that ensures we'll never start more than once if (_started) @@ -79,21 +80,36 @@ private void Start() if (_configuration.CollectorSyncStartup || _configuration.CollectorSendDataOnExit) Connect(); else - _scheduler.ExecuteOnce(Connect, TimeSpan.Zero); + _scheduler.ExecuteOnce(LockAndConnect, TimeSpan.Zero); _started = true; } + finally + { + _lockSemaphore.Release(); + } } + private void LockAndConnect() + { + _lockSemaphore.Wait(); + try + { + Connect(); + } + finally + { + _lockSemaphore.Release(); + } + } + + // This method does not acquire the semaphore. Be certain it is only called from within a semaphore block. private void Connect() { try { - lock (_syncObject) - { - _runtimeConfigurationUpdated = false; - _connectionHandler.Connect(); - } + _runtimeConfigurationUpdated = false; + _connectionHandler.Connect(); // If the runtime configuration has changed, the app names have updated, so we schedule a restart // This uses the existing ScheduleRestart logic so the current Connect can finish and we follow the backoff pattern and don't spam reconnect attempts. @@ -127,6 +143,7 @@ private void HandleConnectionException(Exception ex) // Occurs when no network connection is available, DNS unavailable, etc. case WebException: // Usually occurs when a request times out but did not get far enough along to trigger a timeout exception + // OperationCanceledException is a base class for TaskCanceledException, which can be thrown by HttpClient.SendAsync in .NET 6+ case OperationCanceledException: Log.Info("Connection failed: {0}", ex.Message); break; @@ -157,31 +174,53 @@ private void HandleConnectionException(Exception ex) ImmediateShutdown(); } + // This method does not acquire the semaphore. Be certain it is only called from within a semaphore block. private void Disconnect() { - lock (_syncObject) + _connectionHandler.Disconnect(); + } + + private void LockAndDisconnect() + { + _lockSemaphore.Wait(); + try { - _connectionHandler.Disconnect(); + Disconnect(); + } + finally + { + _lockSemaphore.Release(); } } + private void Reconnect() { EventBus.Publish(new StopHarvestEvent()); - lock (_syncObject) + _lockSemaphore.Wait(); + try { Disconnect(); Connect(); } + finally + { + _lockSemaphore.Release(); + } } public T SendDataRequest(string method, params object[] data) { - lock (_syncObject) + _lockSemaphore.Wait(); + try { return _connectionHandler.SendDataRequest(method, data); } + finally + { + _lockSemaphore.Release(); + } } #endregion Synchronized methods @@ -198,7 +237,7 @@ private void ScheduleRestart() { var retryTime = ConnectionRetryBackoffSequence[_connectionAttempt]; Log.Info("Will attempt to reconnect in {0} seconds", retryTime.TotalSeconds); - _scheduler.ExecuteOnce(Connect, retryTime); + _scheduler.ExecuteOnce(LockAndConnect, retryTime); _connectionAttempt = Math.Min(_connectionAttempt + 1, ConnectionRetryBackoffSequence.Length - 1); } @@ -248,9 +287,8 @@ private void OnRestartAgent(RestartAgentEvent eventData) private void OnCleanShutdown(CleanShutdownEvent eventData) { - Disconnect(); + LockAndDisconnect(); } - #endregion Event handlers } } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/DataStreamingService.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/DataStreamingService.cs index 975fb4fb9d..ff1ef5bfe6 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/DataStreamingService.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/DataStreamingService.cs @@ -49,7 +49,7 @@ private async Task WaitForResponse() var success = false; try { - success = await _responseStream.MoveNext(_streamCancellationToken); + success = await _responseStream.MoveNext(_streamCancellationToken).ConfigureAwait(false); } catch (RpcException rpcEx) { diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs index 62da5474d7..ae81fda0d8 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs @@ -5,7 +5,6 @@ using NewRelic.Agent.Core.Aggregators; using NewRelic.Agent.Core.Commands; using NewRelic.Agent.Core.Events; -using NewRelic.Agent.Core.DataTransport; using NewRelic.Agent.Core.Segments; using NewRelic.Agent.Core.ThreadProfiling; using NewRelic.Agent.Core.Utilities; @@ -17,7 +16,6 @@ using System.Linq; using System.Net; using System.Net.Sockets; - namespace NewRelic.Agent.Core.DataTransport { public interface IDataTransportService @@ -176,7 +174,8 @@ private DataTransportResponse TrySendDataRequest(string method, params obj var errorStatus = GetDataTransportResponseStatusByHttpStatusCode(ex.StatusCode); return new DataTransportResponse(errorStatus); } - catch (Exception ex) when (ex is SocketException || ex is WebException || ex is OperationCanceledException) + // OperationCanceledException is a base class for TaskCanceledException, which can be thrown by HttpClient.SendAsync in .NET 6+ + catch (Exception ex) when (ex is SocketException or WebException or OperationCanceledException) { LogErrorResponse(ex, method, startTime, null); return new DataTransportResponse(DataTransportResponseStatus.Retain); @@ -225,7 +224,7 @@ private void LogErrorResponse(Exception exception, string method, DateTime start { var endTime = DateTime.UtcNow; _agentHealthReporter.ReportSupportabilityCollectorErrorException(method, endTime - startTime, httpStatusCode); - Log.Error(exception, ""); + Log.Error(exception, "Exception in TrySendDataRequest():"); } private DataTransportResponseStatus GetDataTransportResponseStatusByHttpStatusCode(HttpStatusCode httpStatusCode) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs index 0c34f280f2..e5b352c519 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs @@ -45,7 +45,8 @@ public string SendData(string method, ConnectionInfo connectionInfo, string seri try { - var httpClient = _httpClientFactory.CreateClient(connectionInfo.Proxy, _configuration); + Log.Finest("Request({0}): Invoking \"{1}\"", requestGuid, method); + var httpClient = _httpClientFactory.GetOrCreateClient(connectionInfo.Proxy, _configuration); request = new HttpRequest(_configuration) { diff --git a/src/Agent/NewRelic/Agent/Core/Time/Scheduler.cs b/src/Agent/NewRelic/Agent/Core/Time/Scheduler.cs index ce03e33044..434929feec 100644 --- a/src/Agent/NewRelic/Agent/Core/Time/Scheduler.cs +++ b/src/Agent/NewRelic/Agent/Core/Time/Scheduler.cs @@ -16,7 +16,7 @@ public class Scheduler : IScheduler, IDisposable // The System.Threading.Timer class uses -1 milliseconds as a magic "off" value private static readonly TimeSpan DisablePeriodicExecution = TimeSpan.FromMilliseconds(-1); - private readonly object _lock = new object(); + private readonly SemaphoreSlim _lockSemaphore = new SemaphoreSlim(1, 1); private readonly DisposableCollection _oneTimeTimers = new DisposableCollection(); @@ -29,7 +29,8 @@ public void ExecuteOnce(Action action, TimeSpan timeUntilExecution) if (timeUntilExecution < TimeSpan.Zero) throw new ArgumentException("Must be non-negative", "timeUntilExecution"); - lock (_lock) + _lockSemaphore.Wait(); + try { // Removes processed oneTimeTimers var timersToRemove = new List(); @@ -58,6 +59,10 @@ public void ExecuteOnce(Action action, TimeSpan timeUntilExecution) _oneTimeTimers.Add(newExecution); } + finally + { + _lockSemaphore.Release(); + } } public void ExecuteEvery(Action action, TimeSpan timeBetweenExecutions, TimeSpan? optionalInitialDelay = null) @@ -65,7 +70,8 @@ public void ExecuteEvery(Action action, TimeSpan timeBetweenExecutions, TimeSpan if (timeBetweenExecutions < TimeSpan.Zero) throw new ArgumentException("Must be non-negative", "timeBetweenExecutions"); - lock (_lock) + _lockSemaphore.Wait(); + try { var existingTimer = _recurringTimers.GetValueOrDefault(action); if (existingTimer != null) @@ -77,6 +83,10 @@ public void ExecuteEvery(Action action, TimeSpan timeBetweenExecutions, TimeSpan var timer = CreateExecuteEveryTimer(action, timeBetweenExecutions, optionalInitialDelay); _recurringTimers[action] = timer; } + finally + { + _lockSemaphore.Release(); + } } public static Timer CreateExecuteOnceTimer(Action action) @@ -169,7 +179,8 @@ public static Timer CreateExecuteEveryTimer(Action action, TimeSpan timeBetweenE public void StopExecuting(Action action, TimeSpan? timeToWaitForInProgressAction = null) { - lock (_lock) + _lockSemaphore.Wait(); + try { var existingTimer = _recurringTimers.GetValueOrDefault(action); if (existingTimer == null) @@ -190,13 +201,18 @@ public void StopExecuting(Action action, TimeSpan? timeToWaitForInProgressAction waitHandle.WaitOne(timeToWaitForInProgressAction.Value); } } + finally + { + _lockSemaphore.Release(); + } } #endregion Public API public void Dispose() { - lock (_lock) + _lockSemaphore.Wait(); + try { _oneTimeTimers.Dispose(); @@ -206,6 +222,10 @@ public void Dispose() } _recurringTimers.Clear(); } + finally + { + _lockSemaphore.Release(); + } } private class TimerStatus : IDisposable diff --git a/src/Agent/NewRelic/Agent/Core/Utilization/VendorHttpApiRequestor.cs b/src/Agent/NewRelic/Agent/Core/Utilization/VendorHttpApiRequestor.cs index d14f492a7c..2bf7a7607c 100644 --- a/src/Agent/NewRelic/Agent/Core/Utilization/VendorHttpApiRequestor.cs +++ b/src/Agent/NewRelic/Agent/Core/Utilization/VendorHttpApiRequestor.cs @@ -113,7 +113,7 @@ private string CallWithHttpClient(Uri uri, string method, string vendorName, IEn } } - var response = httpClient.Value.SendAsync(request).GetAwaiter().GetResult(); + var response = httpClient.Value.SendAsync(request).ConfigureAwait(false).GetAwaiter().GetResult(); if (!response.IsSuccessStatusCode) { var statusCode = response.StatusCode; @@ -122,7 +122,7 @@ private string CallWithHttpClient(Uri uri, string method, string vendorName, IEn Log.Debug("CallVendorApi ({0}) failed with WebException with status: {1}; message: {2}", vendorName, statusCode, statusDescription); } - return response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); + return response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult(); } catch (Exception ex) { diff --git a/tests/Agent/UnitTests/Core.UnitTest/Core.UnitTest.csproj b/tests/Agent/UnitTests/Core.UnitTest/Core.UnitTest.csproj index 8e9b829e65..1d62b59dd1 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/Core.UnitTest.csproj +++ b/tests/Agent/UnitTests/Core.UnitTest/Core.UnitTest.csproj @@ -15,6 +15,7 @@ + all diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/HttpClientWrapperTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/HttpClientWrapperTests.cs new file mode 100644 index 0000000000..57d21716b4 --- /dev/null +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/HttpClientWrapperTests.cs @@ -0,0 +1,81 @@ +// Copyright 2020 New Relic, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#if !NETFRAMEWORK +using System.Net; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; +using Telerik.JustMock; + + +namespace NewRelic.Agent.Core.DataTransport.Client +{ + [TestFixture] + public class HttpClientWrapperTests + { + private HttpClientWrapper _httpClientWrapper; + private HttpClient _mockHttpClient; + private int _timeoutMilliseconds = 1000; + + [SetUp] + public void SetUp() + { + _mockHttpClient = Mock.Create(); + _httpClientWrapper = new HttpClientWrapper(_mockHttpClient, _timeoutMilliseconds); + } + + [TearDown] + public void TearDown() + { + _httpClientWrapper.Dispose(); + _mockHttpClient.Dispose(); + } + + [Test] + public async Task SendAsync_ShouldReturnHttpResponseMessageWrapper_OnSuccess() + { + // Arrange + var requestMessage = new HttpRequestMessage(HttpMethod.Get, "http://example.com"); + var responseMessage = new HttpResponseMessage(HttpStatusCode.OK); + Mock.Arrange(() => _mockHttpClient.SendAsync(Arg.IsAny(), Arg.IsAny())) + .Returns(Task.FromResult(responseMessage)); + + // Act + var result = await _httpClientWrapper.SendAsync(requestMessage); + + // Assert + Assert.That(result.IsSuccessStatusCode, Is.True); + } + + [Test] + public void SendAsync_ShouldThrowException_OnFailure() + { + // Arrange + var requestMessage = new HttpRequestMessage(HttpMethod.Get, "http://example.com"); + Mock.Arrange(() => _mockHttpClient.SendAsync(Arg.IsAny(), Arg.IsAny())) + .Throws(new HttpRequestException("Request failed")); + + // Act & Assert + Assert.ThrowsAsync(async () => await _httpClientWrapper.SendAsync(requestMessage)); + } + + [Test] + public void SendAsync_ShouldThrowException_OnTimeout() + { + // Arrange + var requestMessage = new HttpRequestMessage(HttpMethod.Get, "http://example.com"); + Mock.Arrange(() => _mockHttpClient.SendAsync(Arg.IsAny(), Arg.IsAny())) + .Returns(async (HttpRequestMessage msg, CancellationToken token) => + { + await Task.Delay(_timeoutMilliseconds + 1000, token); + return new HttpResponseMessage(HttpStatusCode.RequestTimeout); + }); + + // Act & Assert + Assert.ThrowsAsync(async () => await _httpClientWrapper.SendAsync(requestMessage)); + } + } +} +#endif diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientFactoryTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientFactoryTests.cs index d5776005b9..64666a86f9 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientFactoryTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientFactoryTests.cs @@ -1,4 +1,4 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #if !NETFRAMEWORK @@ -36,7 +36,7 @@ public void SetUp() [Test] public void CreateClient_NotNull() { - var client = _httpClientFactory.CreateClient(null, _mockConfiguration); + var client = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); Assert.That(client, Is.Not.Null); } @@ -44,8 +44,8 @@ public void CreateClient_NotNull() [Test] public void CreateClient_NoProxy_ReturnsSameClient() { - var clientA = _httpClientFactory.CreateClient(null, _mockConfiguration); - var clientB = _httpClientFactory.CreateClient(null, _mockConfiguration); + var clientA = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); + var clientB = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); Assert.That(clientA, Is.EqualTo(clientB)); } @@ -53,8 +53,8 @@ public void CreateClient_NoProxy_ReturnsSameClient() [Test] public void CreateClient_Proxy_ReturnsSameClient() { - var clientA = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration); - var clientB = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration); + var clientA = _httpClientFactory.GetOrCreateClient(_mockProxy, _mockConfiguration); + var clientB = _httpClientFactory.GetOrCreateClient(_mockProxy, _mockConfiguration); Assert.That(clientA, Is.EqualTo(clientB)); } @@ -62,8 +62,8 @@ public void CreateClient_Proxy_ReturnsSameClient() [Test] public void CreateClient_NoProxyToProxy_ReturnsNewClient() { - var clientA = _httpClientFactory.CreateClient(null, _mockConfiguration); - var clientB = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration); + var clientA = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); + var clientB = _httpClientFactory.GetOrCreateClient(_mockProxy, _mockConfiguration); Assert.That(clientA, Is.Not.EqualTo(clientB)); } @@ -71,8 +71,8 @@ public void CreateClient_NoProxyToProxy_ReturnsNewClient() [Test] public void CreateClient_ProxyToNoProxy_ReturnsNewClient() { - var clientA = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration); - var clientB = _httpClientFactory.CreateClient(null, _mockConfiguration); + var clientA = _httpClientFactory.GetOrCreateClient(_mockProxy, _mockConfiguration); + var clientB = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); Assert.That(clientA, Is.Not.EqualTo(clientB)); } diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs index 23ad71b34c..be0232d0b7 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs @@ -3,14 +3,16 @@ #if !NETFRAMEWORK using System; +using System.Linq; 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; +using Nito.AsyncEx; +using System.Threading; +using System.Threading.Tasks; namespace NewRelic.Agent.Core.DataTransport.Client { @@ -27,16 +29,17 @@ public class NRHttpClientTests public void SetUp() { _mockConfiguration = Mock.Create(); - Mock.Arrange(() => _mockConfiguration.AgentLicenseKey).Returns( "12345"); + Mock.Arrange(() => _mockConfiguration.AgentLicenseKey).Returns("12345"); Mock.Arrange(() => _mockConfiguration.AgentRunId).Returns("123"); Mock.Arrange(() => _mockConfiguration.CollectorMaxPayloadSizeInBytes).Returns(int.MaxValue); + Mock.Arrange(() => _mockConfiguration.CollectorTimeout).Returns(60000); // 60 seconds + Mock.Arrange(() => _mockConfiguration.PutForDataSend).Returns(false); _mockConnectionInfo = Mock.Create(); Mock.Arrange(() => _mockConnectionInfo.Host).Returns("testhost.com"); Mock.Arrange(() => _mockConnectionInfo.HttpProtocol).Returns("https"); Mock.Arrange(() => _mockConnectionInfo.Port).Returns(123); - _mockProxy = Mock.Create(); _mockHttpClientWrapper = Mock.Create(); @@ -50,6 +53,7 @@ public void TearDown() _client.Dispose(); _mockHttpClientWrapper.Dispose(); } + [Test] public void Send_ReturnsResponse_WhenSendAsyncSucceeds() { @@ -68,10 +72,11 @@ public void Send_ReturnsResponse_WhenSendAsyncSucceeds() // Assert Assert.That(response, Is.Not.Null); + Assert.That(response.StatusCode, Is.EqualTo(HttpStatusCode.OK)); } [Test] - public void SendAsync_ThrowsHttpRequestException_WhenSendAsyncThrows() + public void Send_ThrowsHttpRequestException_WhenSendAsyncThrows() { // Arrange var request = CreateHttpRequest(); @@ -83,6 +88,100 @@ public void SendAsync_ThrowsHttpRequestException_WhenSendAsyncThrows() Assert.Throws(() => _client.Send(request)); } + [Test] + public void Send_AddsCustomHeaders() + { + // Arrange + var request = CreateHttpRequest(); + request.Headers.Add("Custom-Header", "HeaderValue"); + + var mockHttpResponseMessage = Mock.Create(); + Mock.Arrange(() => mockHttpResponseMessage.StatusCode).Returns(HttpStatusCode.OK); + Mock.Arrange(() => mockHttpResponseMessage.IsSuccessStatusCode).Returns(true); + + Mock.Arrange(() => _mockHttpClientWrapper.SendAsync(Arg.IsAny())) + .ReturnsAsync(mockHttpResponseMessage); + + // Act + var response = _client.Send(request); + + // Assert + Mock.Assert(() => _mockHttpClientWrapper.SendAsync(Arg.Matches(req => + req.Headers.Contains("Custom-Header") && req.Headers.GetValues("Custom-Header").First() == "HeaderValue")), Occurs.Once()); + } + + [Test] + public void Send_SetsContentHeaders() + { + // Arrange + var request = CreateHttpRequest(); + + var mockHttpResponseMessage = Mock.Create(); + Mock.Arrange(() => mockHttpResponseMessage.StatusCode).Returns(HttpStatusCode.OK); + Mock.Arrange(() => mockHttpResponseMessage.IsSuccessStatusCode).Returns(true); + + Mock.Arrange(() => _mockHttpClientWrapper.SendAsync(Arg.IsAny())) + .ReturnsAsync(mockHttpResponseMessage); + + // Act + var response = _client.Send(request); + + // Assert + Mock.Assert(() => _mockHttpClientWrapper.SendAsync(Arg.Matches(req => + req.Content.Headers.ContentType.MediaType == "application/json" && + req.Content.Headers.ContentLength == request.Content.PayloadBytes.Length)), Occurs.Once()); + } + + [Test] + public void Dispose_DisposesHttpClientWrapper() + { + // Act + _client.Dispose(); + + // Assert + Mock.Assert(() => _mockHttpClientWrapper.Dispose(), Occurs.Once()); + } + + [Test] + public void Send_Does_Not_Deadlock() + { + using var client = new NRHttpClient(_mockProxy, _mockConfiguration); + + client.SetHttpClientWrapper(new HttpClientWrapper(new HttpClient(), 500)); + + var request = new HttpRequest(_mockConfiguration) + { + Endpoint = "connect", + ConnectionInfo = _mockConnectionInfo, + RequestGuid = Guid.NewGuid(), + Content = { SerializedData = "{\"Test\"}", ContentType = "application/json" } + }; + + Assert.DoesNotThrow(() => + { + // start a thread that might deadlock - if the thread doesn't complete within 5 seconds, then we have a deadlock + var thread = new Thread(() => SendWithAsyncContext(client, request)); + thread.Start(); + if (!thread.Join(5000)) + throw new Exception("Deadlock detected."); + }, "Deadlock detected. Check logic in NRHttpClient.Send() and HttpClientWrapper.Send() for correct usage of .ConfigureAwait(false)"); + } + + private static void SendWithAsyncContext(NRHttpClient client, HttpRequest request) + { + AsyncContext.Run(() => + { + try + { + var response = client.Send(request); + } + catch (TaskCanceledException) + { + // expected exception + } + }); + } + private IHttpRequest CreateHttpRequest() { var request = new HttpRequest(_mockConfiguration) @@ -90,7 +189,7 @@ private IHttpRequest CreateHttpRequest() Endpoint = "Test", ConnectionInfo = _mockConnectionInfo, RequestGuid = Guid.NewGuid(), - Content = { SerializedData = "{\"Test\"}", ContentType = "application/json"} + Content = { SerializedData = "{\"Test\"}", ContentType = "application/json" } }; return request; diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/HttpCollectorWireTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/HttpCollectorWireTests.cs index b7508f69b6..fd88b6aad7 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/HttpCollectorWireTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/HttpCollectorWireTests.cs @@ -64,7 +64,7 @@ public void SendData_ShouldSendRequestWithValidParameters(bool usePutForSend) var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Returns(mockHttpResponse); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); @@ -73,7 +73,7 @@ public void SendData_ShouldSendRequestWithValidParameters(bool usePutForSend) // Assert Assert.That(response, Is.EqualTo(expected)); - Mock.Assert(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); + Mock.Assert(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); } [Test] @@ -96,14 +96,14 @@ public void SendData_ShouldThrowHttpRequestException_WhenRequestThrows() var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Throws(new HttpRequestException()); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); // Act and Assert Assert.Throws(() => collectorWire.SendData("test_method", connectionInfo, serializedData, Guid.NewGuid())); - Mock.Assert(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); + Mock.Assert(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); } [Test] @@ -126,14 +126,14 @@ public void SendData_ShouldThrowHttpException_WhenResponse_IsNotSuccessful() var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Returns(mockHttpResponse); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); // Act and Assert Assert.Throws(() => collectorWire.SendData("test_method", connectionInfo, serializedData, Guid.NewGuid())); - Mock.Assert(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); + Mock.Assert(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); } [Test] @@ -158,7 +158,7 @@ public void SendData_ShouldDropPayload_WhenPayloadSizeExceedsMaxSize() var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Returns(mockHttpResponse); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); @@ -192,7 +192,7 @@ public void SendData_ShouldNotCallAuditLog_UnlessAuditLogIsEnabled(bool isEnable var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Returns(mockHttpResponse); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); diff --git a/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/ConvertMetricDataToJsonTests.cs b/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/ConvertMetricDataToJsonTests.cs index 23f7c0b55e..59ba644df9 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/ConvertMetricDataToJsonTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/ConvertMetricDataToJsonTests.cs @@ -10,8 +10,10 @@ using NewRelic.Agent.Core.Utilities; using NewRelic.Agent.Core.WireModels; using NewRelic.Agent.Core.SharedInterfaces; +using Newtonsoft.Json; using NUnit.Framework; using Telerik.JustMock; +using JsonSerializer = NewRelic.Agent.Core.DataTransport.JsonSerializer; namespace NewRelic.Agent.Core.JsonConverters { @@ -24,7 +26,6 @@ public class ConvertMetricDataToJsonTests private MetricWireModelCollection _wellformedMetricData; private string _wellformedJson = "[\"440491846668652\",1450462672.0,1450462710.0,[[{\"name\":\"DotNet/name\",\"scope\":\"WebTransaction/DotNet/name\"},[1,3.0,2.0,3.0,3.0,9.0]],[{\"name\":\"Custom/name\"},[1,4.0,3.0,4.0,4.0,16.0]]]]"; - [SetUp] public void Setup() { @@ -55,6 +56,16 @@ public void Serialize_NoErrors() Assert.DoesNotThrow(() => _connectionHandler.SendDataRequest("metric_data", _wellformedMetricData)); } + [Test] + public void Serialize_ThrowsOnInvalidObject() + { + var node1 = new Node { Name = "Node1" }; + var node2 = new Node { Name = "Node2", Child = node1 }; + node1.Child = node2; // circular reference, should throw + + Assert.Throws(() => _connectionHandler.SendDataRequest("metric_data", node1)); + } + [Test] public void Serialize_MatchesExpectedOutput() { @@ -63,5 +74,12 @@ public void Serialize_MatchesExpectedOutput() var serializedMetrics = Newtonsoft.Json.JsonConvert.SerializeObject(model); Assert.That(serializedMetrics, Is.EqualTo(_wellformedJson)); } + + public class Node + { + public string Name { get; set; } + public Node Child { get; set; } + } + } }