[release/8.5] Detect connection timeouts in Hedging (#5149)

dotnet · May 9, 2024 · de1f323 · de1f323
2 parents a8a6e03 + 80d518b
commit de1f323
Show file tree

Hide file tree

Showing 11 changed files with 185 additions and 66 deletions.
diff --git a/...ies/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs b/...ies/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs
@@ -2,7 +2,10 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Diagnostics.CodeAnalysis;
 using System.Net.Http;
+using System.Threading;
+using Microsoft.Shared.DiagnosticIds;
 using Microsoft.Shared.Diagnostics;
 using Polly;
 using Polly.CircuitBreaker;
@@ -17,13 +20,26 @@ public static class HttpClientHedgingResiliencePredicates
     /// <summary>
     /// Determines whether an outcome should be treated by hedging as a transient failure.
     /// </summary>
+    /// <param name="outcome">The outcome of the user-specified callback.</param>
     /// <returns><see langword="true"/> if outcome is transient, <see langword="false"/> if not.</returns>
-    public static bool IsTransient(Outcome<HttpResponseMessage> outcome) => outcome switch
-    {
-        { Result: { } response } when HttpClientResiliencePredicates.IsTransientHttpFailure(response) => true,
-        { Exception: { } exception } when IsTransientHttpException(exception) => true,
-        _ => false,
-    };
+    public static bool IsTransient(Outcome<HttpResponseMessage> outcome)
+        => outcome switch
+        {
+            { Result: { } response } when HttpClientResiliencePredicates.IsTransientHttpFailure(response) => true,
+            { Exception: { } exception } when IsTransientHttpException(exception) => true,
+            _ => false,
+        };
+
+    /// <summary>
+    /// Determines whether an <see cref="HttpResponseMessage"/> should be treated by hedging as a transient failure.
+    /// </summary>
+    /// <param name="outcome">The outcome of the user-specified callback.</param>
+    /// <param name="cancellationToken">The <see cref="CancellationToken"/> associated with the execution.</param>
+    /// <returns><see langword="true"/> if outcome is transient, <see langword="false"/> if not.</returns>
+    [Experimental(diagnosticId: DiagnosticIds.Experiments.Resilience, UrlFormat = DiagnosticIds.UrlFormat)]
+    public static bool IsTransient(Outcome<HttpResponseMessage> outcome, CancellationToken cancellationToken)
+        => HttpClientResiliencePredicates.IsHttpConnectionTimeout(outcome, cancellationToken)
+           || IsTransient(outcome);
 
     /// <summary>
     /// Determines whether an exception should be treated by hedging as a transient failure.
@@ -35,8 +51,7 @@ internal static bool IsTransientHttpException(Exception exception)
         return exception switch
         {
             BrokenCircuitException => true,
-            _ when HttpClientResiliencePredicates.IsTransientHttpException(exception) => true,
-            _ => false,
+            _ => HttpClientResiliencePredicates.IsTransientHttpException(exception),
         };
     }
 }
diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs
@@ -16,11 +16,11 @@ public class HttpHedgingStrategyOptions : HedgingStrategyOptions<HttpResponseMes
     /// Initializes a new instance of the <see cref="HttpHedgingStrategyOptions"/> class.
     /// </summary>
     /// <remarks>
-    /// By default the options is set to handle only transient failures,
+    /// By default, the options is set to handle only transient failures,
     /// i.e. timeouts, 5xx responses and <see cref="HttpRequestException"/> exceptions.
     /// </remarks>
     public HttpHedgingStrategyOptions()
     {
-        ShouldHandle = args => new ValueTask<bool>(HttpClientHedgingResiliencePredicates.IsTransient(args.Outcome));
+        ShouldHandle = args => new ValueTask<bool>(HttpClientHedgingResiliencePredicates.IsTransient(args.Outcome, args.Context.CancellationToken));
     }
 }
diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpClientResiliencePredicates.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpClientResiliencePredicates.cs
@@ -2,8 +2,11 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Diagnostics.CodeAnalysis;
 using System.Net;
 using System.Net.Http;
+using System.Threading;
+using Microsoft.Shared.DiagnosticIds;
 using Microsoft.Shared.Diagnostics;
 using Polly;
 using Polly.Timeout;
@@ -26,17 +29,32 @@ public static class HttpClientResiliencePredicates
         _ => false
     };
 
+    /// <summary>
+    /// Determines whether an <see cref="HttpResponseMessage"/> should be treated by resilience strategies as a transient failure.
+    /// </summary>
+    /// <param name="outcome">The outcome of the user-specified callback.</param>
+    /// <param name="cancellationToken">The <see cref="CancellationToken"/> associated with the execution.</param>
+    /// <returns><see langword="true"/> if outcome is transient, <see langword="false"/> if not.</returns>
+    [Experimental(diagnosticId: DiagnosticIds.Experiments.Resilience, UrlFormat = DiagnosticIds.UrlFormat)]
+    public static bool IsTransient(Outcome<HttpResponseMessage> outcome, CancellationToken cancellationToken)
+        => IsHttpConnectionTimeout(outcome, cancellationToken)
+           || IsTransient(outcome);
+
     /// <summary>
     /// Determines whether an exception should be treated by resilience strategies as a transient failure.
     /// </summary>
     internal static bool IsTransientHttpException(Exception exception)
     {
         _ = Throw.IfNull(exception);
 
-        return exception is HttpRequestException ||
-               exception is TimeoutRejectedException;
+        return exception is HttpRequestException or TimeoutRejectedException;
     }
 
+    internal static bool IsHttpConnectionTimeout(in Outcome<HttpResponseMessage> outcome, in CancellationToken cancellationToken)
+        => !cancellationToken.IsCancellationRequested
+           && outcome.Exception is OperationCanceledException { Source: "System.Private.CoreLib" }
+           && outcome.Exception.InnerException is TimeoutException;
+
     /// <summary>
     /// Determines whether a response contains a transient failure.
     /// </summary>
@@ -52,7 +70,6 @@ internal static bool IsTransientHttpFailure(HttpResponseMessage response)
         return statusCode >= InternalServerErrorCode ||
             response.StatusCode == HttpStatusCode.RequestTimeout ||
             statusCode == TooManyRequests;
-
     }
 
     private const int InternalServerErrorCode = (int)HttpStatusCode.InternalServerError;

diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptions.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptions.cs
@@ -26,7 +26,7 @@ public class HttpRetryStrategyOptions : RetryStrategyOptions<HttpResponseMessage
     /// </remarks>
     public HttpRetryStrategyOptions()
     {
-        ShouldHandle = args => new ValueTask<bool>(HttpClientResiliencePredicates.IsTransient(args.Outcome));
+        ShouldHandle = args => new ValueTask<bool>(HttpClientResiliencePredicates.IsTransient(args.Outcome, args.Context.CancellationToken));
         BackoffType = DelayBackoffType.Exponential;
         ShouldRetryAfterHeader = true;
         UseJitter = true;

diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/HedgingTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/HedgingTests.cs
@@ -35,6 +35,7 @@ public abstract class HedgingTests<TBuilder> : IDisposable
     private readonly Func<RequestRoutingStrategy> _requestRoutingStrategyFactory;
     private readonly IServiceCollection _services;
     private readonly Queue<HttpResponseMessage> _responses = new();
+    private ServiceProvider? _serviceProvider;
     private bool _failure;
 
     private protected HedgingTests(Func<IHttpClientBuilder, Func<RequestRoutingStrategy>, TBuilder> createDefaultBuilder)
@@ -63,6 +64,11 @@ public void Dispose()
         _requestRoutingStrategyMock.VerifyAll();
         _cancellationTokenSource.Cancel();
         _cancellationTokenSource.Dispose();
+        _serviceProvider?.Dispose();
+        foreach (var response in _responses)
+        {
+            response.Dispose();
+        }
     }
 
     [Fact]
@@ -93,7 +99,7 @@ public async Task SendAsync_EnsureContextFlows()
 
         using var client = CreateClientWithHandler();
 
-        await client.SendAsync(request, _cancellationTokenSource.Token);
+        using var _ = await client.SendAsync(request, _cancellationTokenSource.Token);
 
         Assert.Equal(2, calls);
     }
@@ -108,7 +114,7 @@ public async Task SendAsync_NoErrors_ShouldReturnSingleResponse()
 
         AddResponse(HttpStatusCode.OK);
 
-        var response = await client.SendAsync(request, _cancellationTokenSource.Token);
+        using var _ = await client.SendAsync(request, _cancellationTokenSource.Token);
         AssertNoResponse();
 
         Assert.Single(Requests);
@@ -164,7 +170,7 @@ public async Task SendAsync_NoRoutesLeftAndSomeResultPresent_ShouldReturn()
 
         using var client = CreateClientWithHandler();
 
-        var result = await client.SendAsync(request, _cancellationTokenSource.Token);
+        using var result = await client.SendAsync(request, _cancellationTokenSource.Token);
         Assert.Equal(DefaultHedgingAttempts + 1, Requests.Count);
         Assert.Equal(HttpStatusCode.ServiceUnavailable, result.StatusCode);
     }
@@ -183,7 +189,7 @@ public async Task SendAsync_EnsureDistinctContextForEachAttempt()
 
         using var client = CreateClientWithHandler();
 
-        await client.SendAsync(request, _cancellationTokenSource.Token);
+        using var _ = await client.SendAsync(request, _cancellationTokenSource.Token);
 
         RequestContexts.Distinct().OfType<ResilienceContext>().Should().HaveCount(3);
     }
@@ -204,7 +210,7 @@ public async Task SendAsync_EnsureContextReplacedInRequestMessage()
 
         using var client = CreateClientWithHandler();
 
-        await client.SendAsync(request, _cancellationTokenSource.Token);
+        using var _ = await client.SendAsync(request, _cancellationTokenSource.Token);
 
         RequestContexts.Distinct().OfType<ResilienceContext>().Should().HaveCount(3);
 
@@ -226,7 +232,7 @@ public async Task SendAsync_NoRoutesLeft_EnsureLessThanMaxHedgedAttempts()
 
         using var client = CreateClientWithHandler();
 
-        var result = await client.SendAsync(request, _cancellationTokenSource.Token);
+        using var _ = await client.SendAsync(request, _cancellationTokenSource.Token);
         Assert.Equal(2, Requests.Count);
     }
 
@@ -244,7 +250,7 @@ public async Task SendAsync_FailedExecution_ShouldReturnResponseFromHedging()
 
         using var client = CreateClientWithHandler();
 
-        var result = await client.SendAsync(request, _cancellationTokenSource.Token);
+        using var result = await client.SendAsync(request, _cancellationTokenSource.Token);
         Assert.Equal(3, Requests.Count);
         Assert.Equal(HttpStatusCode.OK, result.StatusCode);
         Assert.Equal("https://enpoint-1:80/some-path?query", Requests[0]);
@@ -268,7 +274,12 @@ protected void AddResponse(HttpStatusCode statusCode, int count)
 
     protected abstract void ConfigureHedgingOptions(Action<HttpHedgingStrategyOptions> configure);
 
-    protected HttpClient CreateClientWithHandler() => _services.BuildServiceProvider().GetRequiredService<IHttpClientFactory>().CreateClient(ClientId);
+    protected HttpClient CreateClientWithHandler()
+    {
+        _serviceProvider?.Dispose();
+        _serviceProvider = _services.BuildServiceProvider();
+        return _serviceProvider.GetRequiredService<IHttpClientFactory>().CreateClient(ClientId);
+    }
 
     private Task<HttpResponseMessage> InnerHandlerFunction(HttpRequestMessage request, CancellationToken cancellationToken)
     {

diff --git a/...ries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/OperationCanceledExceptionMock.cs b/...ries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/OperationCanceledExceptionMock.cs
@@ -0,0 +1,16 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+
+namespace Microsoft.Extensions.Http.Resilience.Test.Hedging;
+
+internal sealed class OperationCanceledExceptionMock : OperationCanceledException
+{
+    public OperationCanceledExceptionMock(Exception innerException)
+        : base(null, innerException)
+    {
+    }
+
+    public override string? Source { get => "System.Private.CoreLib"; set => base.Source = value; }
+}