Support synchronous HttpClient.Send in SentryHttpMessageHandler (#…

…2336)

CHANGELOG.md

@@ -17,6 +17,7 @@
 - Restore `System.Reflection.Metadata` dependency for .NET Core 3 ([#2302](https://github.com/getsentry/sentry-dotnet/pull/2302))
 - Capture open transactions on disabled hubs ([#2319](https://github.com/getsentry/sentry-dotnet/pull/2319))
 - Remove session breadcrumbs ([#2333](https://github.com/getsentry/sentry-dotnet/pull/2333))
+- Support synchronous `HttpClient.Send` in `SentryHttpMessageHandler` ([#2336](https://github.com/getsentry/sentry-dotnet/pull/2336))
 - Fix ASP.NET Core issue with missing context when using capture methods that configure scope ([#2339](https://github.com/getsentry/sentry-dotnet/pull/2339))
 
 ### Dependencies

src/Sentry/SentryHttpMessageHandler.cs

@@ -13,62 +13,104 @@ public class SentryHttpMessageHandler : DelegatingHandler
     private readonly ISentryFailedRequestHandler? _failedRequestHandler;
 
     /// <summary>
-    /// Initializes an instance of <see cref="SentryHttpMessageHandler"/>.
+    /// Constructs an instance of <see cref="SentryHttpMessageHandler"/>.
     /// </summary>
-    public SentryHttpMessageHandler(IHub hub)
-    {
-        _hub = hub;
-        _options = hub.GetSentryOptions();
-        if (_options != null)
-        {
-            _failedRequestHandler = new SentryFailedRequestHandler(_hub, _options);
-        }
-    }
+    public SentryHttpMessageHandler()
+        : this(default, default, default) { }
+
+    /// <summary>
+    /// Constructs an instance of <see cref="SentryHttpMessageHandler"/>.
+    /// </summary>
+    /// <param name="innerHandler">An inner message handler to delegate calls to.</param>
+    public SentryHttpMessageHandler(HttpMessageHandler innerHandler)
+        : this(default, default, innerHandler) { }
 
-    internal SentryHttpMessageHandler(IHub hub, SentryOptions options, ISentryFailedRequestHandler? failedRequestHandler = null)
+    /// <summary>
+    /// Constructs an instance of <see cref="SentryHttpMessageHandler"/>.
+    /// </summary>
+    /// <param name="hub">The Sentry hub.</param>
+    public SentryHttpMessageHandler(IHub hub)
+        : this(hub, default)
     {
-        _hub = hub;
-        _options = options;
-        _failedRequestHandler = failedRequestHandler;
     }
 
     /// <summary>
-    /// Initializes an instance of <see cref="SentryHttpMessageHandler"/>.
+    /// Constructs an instance of <see cref="SentryHttpMessageHandler"/>.
     /// </summary>
+    /// <param name="innerHandler">An inner message handler to delegate calls to.</param>
+    /// <param name="hub">The Sentry hub.</param>
     public SentryHttpMessageHandler(HttpMessageHandler innerHandler, IHub hub)
-        : this(hub)
+        : this(hub, default, innerHandler)
     {
-        InnerHandler = innerHandler;
     }
 
-    internal SentryHttpMessageHandler(HttpMessageHandler innerHandler, IHub hub, SentryOptions options, ISentryFailedRequestHandler? failedRequestHandler = null)
-        : this(hub, options, failedRequestHandler)
+    internal SentryHttpMessageHandler(IHub? hub, SentryOptions? options, HttpMessageHandler? innerHandler = default, ISentryFailedRequestHandler? failedRequestHandler = null)
     {
-        InnerHandler = innerHandler;
-    }
+        _hub = hub ?? HubAdapter.Instance;
+        _options = options ?? _hub.GetSentryOptions();
+        _failedRequestHandler = failedRequestHandler;
 
-    /// <summary>
-    /// Initializes an instance of <see cref="SentryHttpMessageHandler"/>.
-    /// </summary>
-    public SentryHttpMessageHandler(HttpMessageHandler innerHandler)
-        : this(innerHandler, HubAdapter.Instance) { }
+        // Only assign the inner handler if it is supplied.  We can't assign null or it will throw.
+        // We also cannot assign a default value here, or it will throw when used with HttpMessageHandlerBuilderFilter.
+        if (innerHandler is not null)
+        {
+            InnerHandler = innerHandler;
+        }
 
-    /// <summary>
-    /// Initializes an instance of <see cref="SentryHttpMessageHandler"/>.
-    /// </summary>
-    public SentryHttpMessageHandler()
-        : this(HubAdapter.Instance) { }
+        // Use the default failed request handler if none was supplied - but options is required.
+        if (_failedRequestHandler == null && _options != null)
+        {
+            _failedRequestHandler = new SentryFailedRequestHandler(_hub, _options);
+        }
+    }
 
     /// <inheritdoc />
     protected override async Task<HttpResponseMessage> SendAsync(
         HttpRequestMessage request,
         CancellationToken cancellationToken)
     {
-        // Prevent null reference exception in the following call
-        // in case the user didn't set an inner handler.
+        var (span, method, url) = ProcessRequest(request);
+
+        try
+        {
+            var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
+            HandleResponse(response, span, method, url);
+            return response;
+        }
+        catch (Exception ex)
+        {
+            span?.Finish(ex);
+            throw;
+        }
+    }
+
+#if NET5_0_OR_GREATER
+    /// <inheritdoc />
+    protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
+    {
+        var (span, method, url) = ProcessRequest(request);
+
+        try
+        {
+            var response = base.Send(request, cancellationToken);
+            HandleResponse(response, span, method, url);
+            return response;
+        }
+        catch (Exception ex)
+        {
+            span?.Finish(ex);
+            throw;
+        }
+    }
+#endif
+
+    private (ISpan? Span, string Method, string Url) ProcessRequest(HttpRequestMessage request)
+    {
+        // Assign a default inner handler for convenience the first time this is used.
+        // We can't do this in a constructor, or it will throw when used with HttpMessageHandlerBuilderFilter.
         InnerHandler ??= new HttpClientHandler();
 
-        var requestMethod = request.Method.Method.ToUpperInvariant();
+        var method = request.Method.Method.ToUpperInvariant();
         var url = request.RequestUri?.ToString() ?? string.Empty;
 
         if (_options?.TracePropagationTargets.ContainsMatch(url) is true or null)
@@ -79,36 +121,28 @@ protected override async Task<HttpResponseMessage> SendAsync(
 
         // Start a span that tracks this request
         // (may be null if transaction is not set on the scope)
-        var span = _hub.GetSpan()?.StartChild(
-            "http.client",
-            // e.g. "GET https://example.com"
-            $"{requestMethod} {url}");
-
-        try
-        {
-            var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
-
-            var breadcrumbData = new Dictionary<string, string>
-            {
-                { "url", url },
-                { "method", requestMethod },
-                { "status_code", ((int)response.StatusCode).ToString() }
-            };
-            _hub.AddBreadcrumb(string.Empty, "http", "http", breadcrumbData);
+        // e.g. "GET https://example.com"
+        var span = _hub.GetSpan()?.StartChild("http.client", $"{method} {url}");
 
-            // Create events for failed requests
-            _failedRequestHandler?.HandleResponse(response);
-
-            // This will handle unsuccessful status codes as well
-            span?.Finish(SpanStatusConverter.FromHttpStatusCode(response.StatusCode));
+        return (span, method, url);
+    }
 
-            return response;
-        }
-        catch (Exception ex)
+    private void HandleResponse(HttpResponseMessage response, ISpan? span, string method, string url)
+    {
+        var breadcrumbData = new Dictionary<string, string>
         {
-            span?.Finish(ex);
-            throw;
-        }
+            {"url", url},
+            {"method", method},
+            {"status_code", ((int) response.StatusCode).ToString()}
+        };
+        _hub.AddBreadcrumb(string.Empty, "http", "http", breadcrumbData);
+
+        // Create events for failed requests
+        _failedRequestHandler?.HandleResponse(response);
+
+        // This will handle unsuccessful status codes as well
+        var status = SpanStatusConverter.FromHttpStatusCode(response.StatusCode);
+        span?.Finish(status);
     }
 
     private void AddSentryTraceHeader(HttpRequestMessage request)

test/Sentry.AspNetCore.Tests/SentryHttpMessageHandlerBuilderFilterTests.cs

@@ -21,6 +21,8 @@ private class RecordingHandlerBuilderFilter : IHttpMessageHandlerBuilderFilter
         public Action<HttpMessageHandlerBuilder> Configure(Action<HttpMessageHandlerBuilder> next) =>
             handlerBuilder =>
             {
+                // Set the fake handler to prevent outgoing HTTP requests in this test.
+                handlerBuilder.PrimaryHandler = new FakeHttpMessageHandler();
                 handlerBuilder.AdditionalHandlers.Add(_handler);
                 next(handlerBuilder);
             };
@@ -34,7 +36,7 @@ public async Task Generated_client_sends_Sentry_trace_header_automatically()
         // Will use this to record outgoing requests
         using var recorder = new RecordingHttpMessageHandler();
 
-        var hub = new Internal.Hub(new SentryOptions
+        var hub = new Hub(new SentryOptions
         {
             Dsn = ValidDsn
         });
@@ -65,11 +67,6 @@ public async Task Generated_client_sends_Sentry_trace_header_automatically()
                             .GetRequiredService<IHttpClientFactory>()
                             .CreateClient();
 
-                        // The framework setup pipeline would end up adding an HttpClientHandler and this test
-                        // would require access to the Internet. So overriding it here
-                        // so the request stops at our stub:
-                        recorder.InnerHandler = new FakeHttpMessageHandler();
-
                         await httpClient.GetAsync("https://fake.tld");
                     });
                 });

test/Sentry.Testing/FakeHttpMessageHandler.cs

@@ -12,13 +12,11 @@ public FakeHttpMessageHandler(Func<HttpResponseMessage> getResponse)
 
     public FakeHttpMessageHandler() { }
 
-    protected override Task<HttpResponseMessage> SendAsync(
-        HttpRequestMessage request,
-        CancellationToken cancellationToken)
-    {
-        return Task.FromResult(
-            _getResponse is not null
-                ? _getResponse(request)
-                : new HttpResponseMessage(HttpStatusCode.OK));
-    }
+    protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) =>
+        Task.FromResult(_getResponse?.Invoke(request) ?? new HttpResponseMessage(HttpStatusCode.OK));
+
+#if NET5_0_OR_GREATER
+    protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken) =>
+        _getResponse?.Invoke(request) ?? new HttpResponseMessage(HttpStatusCode.OK);
+#endif
 }

test/Sentry.Testing/HttpClientExtensions.cs

@@ -23,7 +23,8 @@ public static IEnumerable<HttpMessageHandler> GetMessageHandlers(this HttpClient
         } while (handler != null);
     }
 
-    public static async Task<HttpRequestMessage> CloneAsync(this HttpRequestMessage source)
+    public static async Task<HttpRequestMessage> CloneAsync(this HttpRequestMessage source,
+        CancellationToken cancellationToken)
     {
         var clone = new HttpRequestMessage(source.Method, source.RequestUri) { Version = source.Version };
 
@@ -38,7 +39,11 @@ public static async Task<HttpRequestMessage> CloneAsync(this HttpRequestMessage
         {
             var cloneContentStream = new MemoryStream();
 
+#if NET5_0_OR_GREATER
+            await source.Content.CopyToAsync(cloneContentStream, cancellationToken).ConfigureAwait(false);
+#else
             await source.Content.CopyToAsync(cloneContentStream).ConfigureAwait(false);
+#endif
             cloneContentStream.Position = 0;
 
             clone.Content = new StreamContent(cloneContentStream);
@@ -52,4 +57,43 @@ public static async Task<HttpRequestMessage> CloneAsync(this HttpRequestMessage
 
         return clone;
     }
+
+#if NET5_0_OR_GREATER
+    public static HttpRequestMessage Clone(this HttpRequestMessage source, CancellationToken cancellationToken)
+    {
+        var clone = new HttpRequestMessage(source.Method, source.RequestUri) { Version = source.Version };
+
+        // Headers
+        foreach (var (key, value) in source.Headers)
+        {
+            clone.Headers.TryAddWithoutValidation(key, value);
+        }
+
+        // Content
+        if (source.Content != null)
+        {
+            var cloneContentStream = new MemoryStream();
+
+            source.Content.CopyTo(cloneContentStream, default, cancellationToken);
+            cloneContentStream.Position = 0;
+
+            clone.Content = new StreamContent(cloneContentStream);
+
+            // Content headers
+            foreach (var (key, value) in source.Content.Headers)
+            {
+                clone.Content.Headers.Add(key, value);
+            }
+        }
+
+        return clone;
+    }
+
+    public static HttpResponseMessage Get(this HttpClient client, string requestUri,
+        CancellationToken cancellationToken = default)
+    {
+        var message = new HttpRequestMessage(HttpMethod.Get, requestUri);
+        return client.Send(message, cancellationToken);
+    }
+#endif
 }

test/Sentry.Testing/RecordingHttpMessageHandler.cs

@@ -6,21 +6,22 @@ public class RecordingHttpMessageHandler : DelegatingHandler
 
     public RecordingHttpMessageHandler() { }
 
-    public RecordingHttpMessageHandler(HttpMessageHandler innerHandler) =>
-        InnerHandler = innerHandler;
+    public RecordingHttpMessageHandler(HttpMessageHandler innerHandler) => InnerHandler = innerHandler;
 
-    protected override async Task<HttpResponseMessage> SendAsync(
-        HttpRequestMessage request,
-        CancellationToken cancellationToken)
+    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
     {
-        // Clone the request to avoid ObjectDisposedException
-        _requests.Add(await request.CloneAsync());
-
-        InnerHandler ??= new FakeHttpMessageHandler();
-
+        _requests.Add(await request.CloneAsync(cancellationToken));
         return await base.SendAsync(request, cancellationToken);
     }
 
+#if NET5_0_OR_GREATER
+    protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
+    {
+        _requests.Add(request.Clone(cancellationToken));
+        return base.Send(request, cancellationToken);
+    }
+#endif
+
     public IReadOnlyList<HttpRequestMessage> GetRequests() => _requests.ToArray();
 
     protected override void Dispose(bool disposing)

test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt

@@ -514,6 +514,7 @@ namespace Sentry
         public SentryHttpMessageHandler(Sentry.IHub hub) { }
         public SentryHttpMessageHandler(System.Net.Http.HttpMessageHandler innerHandler) { }
         public SentryHttpMessageHandler(System.Net.Http.HttpMessageHandler innerHandler, Sentry.IHub hub) { }
+        protected override System.Net.Http.HttpResponseMessage Send(System.Net.Http.HttpRequestMessage request, System.Threading.CancellationToken cancellationToken) { }
         protected override System.Threading.Tasks.Task<System.Net.Http.HttpResponseMessage> SendAsync(System.Net.Http.HttpRequestMessage request, System.Threading.CancellationToken cancellationToken) { }
     }
     public readonly struct SentryId : Sentry.IJsonSerializable, System.IEquatable<Sentry.SentryId>

test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt

@@ -514,6 +514,7 @@ namespace Sentry
         public SentryHttpMessageHandler(Sentry.IHub hub) { }
         public SentryHttpMessageHandler(System.Net.Http.HttpMessageHandler innerHandler) { }
         public SentryHttpMessageHandler(System.Net.Http.HttpMessageHandler innerHandler, Sentry.IHub hub) { }
+        protected override System.Net.Http.HttpResponseMessage Send(System.Net.Http.HttpRequestMessage request, System.Threading.CancellationToken cancellationToken) { }
         protected override System.Threading.Tasks.Task<System.Net.Http.HttpResponseMessage> SendAsync(System.Net.Http.HttpRequestMessage request, System.Threading.CancellationToken cancellationToken) { }
     }
     public readonly struct SentryId : Sentry.IJsonSerializable, System.IEquatable<Sentry.SentryId>