Remove HttpRequestTracer.LastRequest since it's not thread-safe; simplify formatting APIs

Author: C0nquistadore

src/Dibix.Http.Client/Client/DefaultHttpClientFactory.cs

@@ -335,26 +335,23 @@ public IHttpClientBuilder ConfigurePrimaryHttpMessageHandler<THandler>(Func<THan
 
             public void ConfigureDefaults()
             {
-                // Ensure the tracing handler is ran before all other handlers
-                ICollection<Action<HttpMessageHandlerBuilder>> httpMessageHandlerBuilderActions = this.HttpMessageHandlerBuilderActions.ToArray();
-                this.HttpMessageHandlerBuilderActions.Clear();
-
-                if (this.Tracer != null)
-                    this.AddHttpMessageHandler(() => new TracingHttpMessageHandler(this.Tracer));
-
-                this.HttpMessageHandlerBuilderActions.AddRange(httpMessageHandlerBuilderActions);
-
-                if (this.WrapTimeoutsInException)
-                    this.AddHttpMessageHandler<TimeoutHttpMessageHandler>();
-
                 if (this.FollowRedirectsGetRequests)
                     this.AddHttpMessageHandler<FollowRedirectHttpMessageHandler>();
 
                 if (this.TraceProxy)
                     this.AddHttpMessageHandler<TraceProxyHttpMessageHandler>();
 
+                // Run tracing after all other handlers, that potentially modified the request, to ensure the trace includes the actual request that is sent.
+                if (this.Tracer != null)
+                    this.AddHttpMessageHandler(() => new TracingHttpMessageHandler(this.Tracer));
+
+                // Add this after each handler, that needs to access the response, before an exception is thrown. (i.E. TracingHttpMessageHandler)
                 if (this.EnsureSuccessStatusCode)
                     this.AddHttpMessageHandler<EnsureSuccessStatusCodeHttpMessageHandler>();
+
+                // This should be as close to the primary handler as possible to avoid timeouts caused by other handlers.
+                if (this.WrapTimeoutsInException)
+                    this.AddHttpMessageHandler<TimeoutHttpMessageHandler>();
             }
         }
 

src/Dibix.Http.Client/Client/Handlers/TracingHttpMessageHandler.cs

@@ -47,9 +47,9 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
         #endregion
 
         #region Private Methods
-        private Task TraceRequest(HttpRequestMessage request) => this._tracer.TraceRequestMessageAsync(request);
+        private Task TraceRequest(HttpRequestMessage request) => this._tracer.TraceRequestAsync(request);
 
-        private Task TraceResponse(HttpResponseMessage response) => this._tracer.TraceResponseMessageAsync(response, this._requestDurationTracker.Elapsed);
+        private Task TraceResponse(HttpResponseMessage response) => this._tracer.TraceResponseAsync(response, this._requestDurationTracker.Elapsed);
 
         private void StartTracking() => this._requestDurationTracker.Restart();
 

src/Dibix.Http.Client/Client/HttpException.cs

@@ -13,7 +13,7 @@ public class HttpException : Exception
         public HttpResponseMessage Response { get; }
         public string ResponseContentText { get; }
 
-        protected internal HttpException(HttpRequestMessage request, string requestContentText, HttpResponseMessage response, string responseContentText) : base(CreateMessage(response))
+        private protected HttpException(HttpRequestMessage request, string requestContentText, HttpResponseMessage response, string responseContentText) : base(CreateMessage(response))
         {
             this.Request = request;
             this.RequestContentText = requestContentText;

…Client/HttpRequestExceptionExtensions.cs …Client/Client/HttpExceptionExtensions.cssrc/Dibix.Http.Client/Client/HttpRequestExceptionExtensions.cs renamed to src/Dibix.Http.Client/Client/HttpExceptionExtensions.cs src/Dibix.Http.Client/Client/HttpRequestExceptionExtensions.cs renamed to src/Dibix.Http.Client/Client/HttpExceptionExtensions.cs

@@ -1,20 +1,28 @@
-using Newtonsoft.Json;
+using System;
+using Newtonsoft.Json;
 
 namespace Dibix.Http.Client
 {
-    public static class HttpRequestExceptionExtensions
+    public static class HttpExceptionExtensions
     {
         public static T ReadContentAsJson<T>(this HttpException exception)
         {
             Guard.IsNotNull(exception, nameof(exception));
             return JsonConvert.DeserializeObject<T>(exception.ResponseContentText);
         }
 
-        public static string Format(this HttpException exception)
+        public static string GetFormattedText(this HttpException exception, int? maxContentLength = null, bool maskSensitiveData = true)
         {
-            string formattedRequest = HttpMessageFormatter.Format(exception.Request, exception.RequestContentText, maskSensitiveData: true);
-            string formattedResponse = HttpMessageFormatter.Format(exception.Response, exception.ResponseContentText);
-            return HttpMessageFormatter.Format(formattedRequest, formattedResponse);
+            string formattedRequest = HttpMessageFormatter.Format(exception.Request, exception.RequestContentText, maskSensitiveData, maxContentLength);
+            string formattedResponse = HttpMessageFormatter.Format(exception.Response, exception.ResponseContentText, maxContentLength);
+            string text = $@"Request
+-------
+{formattedRequest}
+
+Response
+--------
+{formattedResponse}";
+            return text;
         }
     }
-}
+}

src/Dibix.Http.Client/Client/HttpMessageFormatter.cs

@@ -10,36 +10,14 @@
 
 namespace Dibix.Http.Client
 {
-    public static class HttpMessageFormatter
+    internal static class HttpMessageFormatter
     {
         #region Fields
         private static readonly Func<HttpHeaders, IEnumerable<KeyValuePair<string, string>>> GetHeaderStringsAccessor = CompileGetHeaderStrings();
         #endregion
 
         #region Public Methods
-        public static string Format(string formattedRequestText, string formattedResponseText, int? maxLength = null)
-        {
-            string formattedRequestTextTrimmed = formattedRequestText;
-            string formattedResponseTextTrimmed = formattedResponseText;
-
-            if (maxLength.HasValue)
-            {
-                formattedRequestTextTrimmed = formattedRequestTextTrimmed.TrimIfNecessary(maxLength.Value);
-                formattedResponseTextTrimmed = formattedResponseTextTrimmed.TrimIfNecessary(maxLength.Value);
-            }
-
-            return $@"Request
--------
-{formattedRequestTextTrimmed}
-
-Response
---------
-{formattedResponseTextTrimmed}";
-        }
-        #endregion
-
-        #region Internal Methods
-        internal static string Format(HttpRequestMessage requestMessage, string requestContentText, bool maskSensitiveData)
+        public static string Format(HttpRequestMessage requestMessage, string requestContentText, bool maskSensitiveData, int? maxContentLength)
         {
             Guard.IsNotNull(requestMessage, nameof(requestMessage));
 
@@ -59,20 +37,23 @@ internal static string Format(HttpRequestMessage requestMessage, string requestC
 
             if (requestContentText != null)
             {
-                string secureRequestContentText = requestContentText;
+                string normalizedRequestContentText = requestContentText;
                 if (maskSensitiveData)
-                    secureRequestContentText = Regex.Replace(secureRequestContentText, "password=[^&]+", "password=*****");
+                    normalizedRequestContentText = Regex.Replace(normalizedRequestContentText, "password=[^&]+", "password=*****");
+
+                if (maxContentLength.HasValue)
+                    normalizedRequestContentText = normalizedRequestContentText.TrimIfNecessary(maxContentLength.Value);
 
                 sb.AppendLine()
                   .AppendLine()
-                  .Append(secureRequestContentText);
+                  .Append(normalizedRequestContentText);
             }
 
             string formattedRequest = sb.ToString();
             return formattedRequest;
         }
 
-        internal static string Format(HttpResponseMessage responseMessage, string responseContentText)
+        public static string Format(HttpResponseMessage responseMessage, string responseContentText, int? maxContentLength)
         {
             Guard.IsNotNull(responseMessage, nameof(responseMessage));
 
@@ -92,9 +73,13 @@ internal static string Format(HttpResponseMessage responseMessage, string respon
 
             if (responseContentText.Length > 0)
             {
+                string normalizedResponseContentText = responseContentText;
+                if (maxContentLength.HasValue) 
+                    normalizedResponseContentText = normalizedResponseContentText.TrimIfNecessary(maxContentLength.Value);
+
                 sb.AppendLine()
                   .AppendLine()
-                  .Append(responseContentText);
+                  .Append(normalizedResponseContentText);
             }
 
             string formattedResponse = sb.ToString();

src/Dibix.Http.Client/Client/HttpRequestMessageExtensions.cs

@@ -0,0 +1,17 @@
+using System.Net.Http;
+
+namespace Dibix.Http.Client
+{
+    public static class HttpRequestMessageExtensions
+    {
+        public static string GetFormattedText(this HttpRequestMessage requestMessage, string requestContentText, int? maxContentLength = null, bool maskSensitiveData = true)
+        {
+            return HttpMessageFormatter.Format(requestMessage, requestContentText, maskSensitiveData, maxContentLength);
+        }
+
+        public static string GetFormattedText(this HttpResponseMessage responseMessage, string responseContentText, int? maxContentLength = null)
+        {
+            return HttpMessageFormatter.Format(responseMessage, responseContentText, maxContentLength);
+        }
+    }
+}

src/Dibix.Http.Client/Client/HttpRequestPropertyExtensions.cs

@@ -0,0 +1,48 @@
+using System;
+using System.Net.Http;
+
+namespace Dibix.Http.Client
+{
+    internal static class HttpRequestPropertyExtensions
+    {
+        private const string PropertyKeyHttpRequestTrace = "Dibix.Http.Client.HttpRequestTrace";
+
+        public static bool TryGetHttpRequestTrace(this HttpRequestMessage requestMessage, out HttpRequestTrace requestTrace) => HttpRequestPropertyExtensions.TryGetPropertyValue(requestMessage, PropertyKeyHttpRequestTrace, out requestTrace);
+
+        public static HttpRequestTrace GetHttpRequestTrace(this HttpRequestMessage requestMessage) => HttpRequestPropertyExtensions.GetRequestProperty<HttpRequestTrace>(requestMessage, PropertyKeyHttpRequestTrace);
+
+        public static void SetHttpRequestTrace(this HttpRequestMessage requestMessage, HttpRequestTrace requestTrace) => HttpRequestPropertyExtensions.SetRequestProperty(requestMessage, PropertyKeyHttpRequestTrace, requestTrace);
+
+        private static T GetRequestProperty<T>(HttpRequestMessage requestMessage, string propertyName)
+        {
+            if (!TryGetPropertyValue(requestMessage, propertyName, out T value))
+                throw new InvalidOperationException(FormattableString.Invariant($"Missing property '{propertyName}' on HTTP request message"));
+
+            return value;
+        }
+
+        private static void SetRequestProperty<T>(HttpRequestMessage requestMessage, string propertyName, T value)
+        {
+#if NETCOREAPP
+            requestMessage.Options.Set(new HttpRequestOptionsKey<T>(propertyName), value);
+#else
+            requestMessage.Properties[propertyName] = value;
+#endif
+        }
+
+        private static bool TryGetPropertyValue<T>(HttpRequestMessage requestMessage, string propertyName, out T value)
+        {
+#if NETCOREAPP
+            return requestMessage.Options.TryGetValue(new HttpRequestOptionsKey<T>(propertyName), out value);
+#else
+            if (requestMessage.Properties.TryGetValue(propertyName, out object rawValue))
+            {
+                value = (T)rawValue;
+                return true;
+            }
+            value = default;
+            return false;
+#endif
+        }
+    }
+}

src/Dibix.Http.Client/Client/HttpRequestTrace.cs

@@ -1,20 +1,9 @@
 using System;
-using System.Net.Http;
 
 namespace Dibix.Http.Client
 {
-    public sealed class HttpRequestTrace
+    public class HttpRequestTrace
     {
-        public HttpRequestMessage RequestMessage { get; }
-        public string FormattedRequestText { get; set; }
-        public HttpResponseMessage ResponseMessage { get; set; }
-        public string FormattedResponseText { get; set; }
         public TimeSpan Duration { get; set; }
-
-        internal HttpRequestTrace(HttpRequestMessage requestMessage, string formattedRequestText)
-        {
-            this.RequestMessage = requestMessage;
-            this.FormattedRequestText = formattedRequestText;
-        }
     }
 }

src/Dibix.Http.Client/Client/HttpRequestTracer.cs

@@ -7,98 +7,49 @@ namespace Dibix.Http.Client
     public class HttpRequestTracer
     {
         public bool MaskSensitiveContent { get; } = true;
-        public HttpRequestTrace LastRequest { get; private set; }
 
         public HttpRequestTracer() { }
         public HttpRequestTracer(bool maskSensitiveContent)
         {
             this.MaskSensitiveContent = maskSensitiveContent;
         }
 
-        internal async Task TraceRequestMessageAsync(HttpRequestMessage requestMessage)
+        internal async Task TraceRequestAsync(HttpRequestMessage requestMessage)
         {
-            await this.CollectLastRequest(requestMessage).ConfigureAwait(false);
-            await this.TraceRequestAsync(requestMessage).ConfigureAwait(false);
+            HttpRequestTrace requestTrace = CreateRequestTrace();
+            requestMessage.SetHttpRequestTrace(requestTrace);
+            await this.TraceRequestAsync(requestMessage, requestTrace).ConfigureAwait(false);
         }
 
-        internal async Task TraceResponseMessageAsync(HttpResponseMessage responseMessage, TimeSpan duration)
+        internal async Task TraceResponseAsync(HttpResponseMessage responseMessage, TimeSpan duration)
         {
-            await CompleteLastRequest(responseMessage, duration).ConfigureAwait(false);
-            await this.TraceResponseAsync(responseMessage, duration);
+            HttpRequestTrace requestTrace = responseMessage.RequestMessage.GetHttpRequestTrace();
+            CompleteLastRequest(requestTrace, duration);
+            await this.TraceResponseAsync(responseMessage, requestTrace);
         }
 
-        protected virtual Task TraceRequestAsync(HttpRequestMessage requestMessage) => Task.CompletedTask;
+        protected virtual Task TraceRequestAsync(HttpRequestMessage requestMessage, HttpRequestTrace requestTrace) => Task.CompletedTask;
 
-        protected virtual Task TraceResponseAsync(HttpResponseMessage responseMessage, TimeSpan duration) => Task.CompletedTask;
+        protected virtual Task TraceResponseAsync(HttpResponseMessage responseMessage, HttpRequestTrace requestTrace) => Task.CompletedTask;
 
-        protected virtual bool ShouldBufferRequestContent(HttpRequestMessage requestMessage) => false;
+        private protected virtual HttpRequestTrace CreateRequestTrace() => new HttpRequestTrace();
 
-        protected virtual bool ShouldBufferResponseContent(HttpRequestMessage requestMessage) => false;
-
-        private async Task CollectLastRequest(HttpRequestMessage requestMessage)
-        {
-            string formattedRequestText = await this.GetFormattedRequestContent(requestMessage).ConfigureAwait(false);
-            this.LastRequest = new HttpRequestTrace(requestMessage, formattedRequestText);
-        }
-
-        private async Task CompleteLastRequest(HttpResponseMessage responseMessage, TimeSpan duration)
-        {
-            if (this.LastRequest == null)
-                throw new InvalidOperationException("Request not initialized");
-
-            string formattedResponseText = await this.GetFormattedResponseContent(responseMessage).ConfigureAwait(false);
-            this.LastRequest.ResponseMessage = responseMessage;
-            this.LastRequest.FormattedResponseText = formattedResponseText;
-            this.LastRequest.Duration = duration;
-
-            // Since non successful status code will throw an exception,
-            // it is now safe to restore the request content, that was not previously captured
-            HttpRequestMessage requestMessage = responseMessage.RequestMessage;
-            if (!this.ShouldBufferRequestContent(requestMessage) && !responseMessage.IsSuccessStatusCode)
-            {
-                string requestContentText = await GetRequestContentTextAsync(requestMessage, bufferRequestContent: true).ConfigureAwait(false);
-                this.LastRequest.FormattedRequestText = this.FormatRequest(requestMessage, requestContentText);
-            }
-        }
-
-        private async Task<string> GetFormattedRequestContent(HttpRequestMessage requestMessage)
-        {
-            bool bufferRequestContent = this.ShouldBufferRequestContent(requestMessage);
-            string requestContentText = await GetRequestContentTextAsync(requestMessage, bufferRequestContent).ConfigureAwait(false);
-            string formattedRequestText = this.FormatRequest(requestMessage, requestContentText);
-            return formattedRequestText;
-        }
-
-        private async Task<string> GetFormattedResponseContent(HttpResponseMessage responseMessage)
+        private static void CompleteLastRequest(HttpRequestTrace requestTrace, TimeSpan duration)
         {
-            bool bufferResponseContent = this.ShouldBufferResponseContent(responseMessage.RequestMessage) || !responseMessage.IsSuccessStatusCode;
-            string responseContentTest = await GetResponseContentTextAsync(responseMessage, bufferResponseContent).ConfigureAwait(false);
-            string formattedResponseText = HttpMessageFormatter.Format(responseMessage, responseContentTest);
-            return formattedResponseText;
+            requestTrace.Duration = duration;
         }
+    }
 
-        private string FormatRequest(HttpRequestMessage requestMessage, string requestContentText) => HttpMessageFormatter.Format(requestMessage, requestContentText, this.MaskSensitiveContent);
-
-        private static async Task<string> GetRequestContentTextAsync(HttpRequestMessage requestMessage, bool bufferRequestContent)
-        {
-            if (requestMessage.Content == null)
-                return null;
-
-            if (!bufferRequestContent)
-                return "<Request content unavailable>";
+    public class HttpRequestTracer<T> : HttpRequestTracer where T : HttpRequestTrace, new()
+    {
+        protected sealed override Task TraceRequestAsync(HttpRequestMessage requestMessage, HttpRequestTrace requestTrace) => this.TraceRequestAsync(requestMessage, (T)requestTrace);
 
-            return await requestMessage.Content.ReadAsStringAsync().ConfigureAwait(false);
-        }
+        protected sealed override Task TraceResponseAsync(HttpResponseMessage responseMessage, HttpRequestTrace requestTrace) => this.TraceResponseAsync(responseMessage, (T)requestTrace);
 
-        private static async Task<string> GetResponseContentTextAsync(HttpResponseMessage responseMessage, bool bufferResponseContent)
-        {
-            if (responseMessage.Content == null)
-                return null;
+        private protected sealed override HttpRequestTrace CreateRequestTrace() => new T();
 
-            if (!bufferResponseContent)
-                return "<Response content unavailable>";
+        protected virtual Task TraceRequestAsync(HttpRequestMessage requestMessage, T requestTrace) => Task.CompletedTask;
 
-            return await responseMessage.Content.ReadAsStringAsync().ConfigureAwait(false);
-        }
+        protected virtual Task TraceResponseAsync(HttpResponseMessage responseMessage, T requestTrace) => Task.CompletedTask;
     }
 }