From 26047a034799a701665841e647744346c1fa6836 Mon Sep 17 00:00:00 2001 From: Nikita Balabaev Date: Tue, 30 Apr 2024 18:04:06 +0200 Subject: [PATCH 1/6] Detect connection timeouts in Hedging --- .../HttpClientHedgingResiliencePredicates.cs | 26 ++++++--- .../Hedging/HttpHedgingStrategyOptions.cs | 2 +- .../Hedging/HedgingTests.cs | 58 +++++++++++++++++++ 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs index 5955510faab..585bd4b5a2f 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs @@ -6,6 +6,7 @@ using Microsoft.Shared.Diagnostics; using Polly; using Polly.CircuitBreaker; +using Polly.Hedging; namespace Microsoft.Extensions.Http.Resilience; @@ -18,12 +19,22 @@ public static class HttpClientHedgingResiliencePredicates /// Determines whether an outcome should be treated by hedging as a transient failure. /// /// if outcome is transient, if not. - public static bool IsTransient(Outcome outcome) => outcome switch - { - { Result: { } response } when HttpClientResiliencePredicates.IsTransientHttpFailure(response) => true, - { Exception: { } exception } when IsTransientHttpException(exception) => true, - _ => false, - }; + public static bool IsTransient(Outcome outcome) + => outcome switch + { + { Result: { } response } when HttpClientResiliencePredicates.IsTransientHttpFailure(response) => true, + { Exception: { } exception } when IsTransientHttpException(exception) => true, + _ => false, + }; + + internal static bool IsTransient(HedgingPredicateArguments args) + => IsConnectionTimeout(args) + || IsTransient(args.Outcome); + + internal static bool IsConnectionTimeout(HedgingPredicateArguments args) + => !args.Context.CancellationToken.IsCancellationRequested + && args.Outcome.Exception is OperationCanceledException { Source: "System.Private.CoreLib" } + && args.Outcome.Exception.InnerException is TimeoutException; /// /// Determines whether an exception should be treated by hedging as a transient failure. @@ -35,8 +46,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 index faa46375f65..f4fd9cef3f4 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs @@ -21,6 +21,6 @@ public class HttpHedgingStrategyOptions : HedgingStrategyOptions public HttpHedgingStrategyOptions() { - ShouldHandle = args => new ValueTask(HttpClientHedgingResiliencePredicates.IsTransient(args.Outcome)); + ShouldHandle = args => new ValueTask(HttpClientHedgingResiliencePredicates.IsTransient(args)); } } diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/HedgingTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/HedgingTests.cs index f3c132e7c67..59b68ff1841 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/HedgingTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/HedgingTests.cs @@ -252,6 +252,64 @@ public async Task SendAsync_FailedExecution_ShouldReturnResponseFromHedging() Assert.Equal("https://enpoint-3:80/some-path?query", Requests[2]); } + [Fact] + public async Task SendAsync_FailedConnect_ShouldReturnResponseFromHedging() + { + const string ClientName = "HedgingClient"; + + // TODO: use mocked request destinations + + var services = new ServiceCollection(); + var clientBuilder = services + .AddHttpClient(ClientName) + .ConfigurePrimaryHttpMessageHandler(() => +#if NETFRAMEWORK + new WinHttpHandler + { + SendTimeout = TimeSpan.FromSeconds(1), +#else + new SocketsHttpHandler + { + ConnectTimeout = TimeSpan.FromSeconds(1), + SslOptions = new System.Net.Security.SslClientAuthenticationOptions + { + ClientCertificates = [], + RemoteCertificateValidationCallback = (_, _, _, _) => true + }, +#endif + }) + .AddStandardHedgingHandler(routing => + routing.ConfigureOrderedGroups(g => + { + g.Groups.Add(new UriEndpointGroup + { + Endpoints = [new WeightedUriEndpoint { Uri = new Uri("https://localhost:3000") }] + }); + + g.Groups.Add(new UriEndpointGroup + { + Endpoints = [new WeightedUriEndpoint { Uri = new Uri("https://microsoft.com") }] + }); + })) + .Configure(opt => + { + opt.Hedging.MaxHedgedAttempts = 10; + opt.Hedging.Delay = TimeSpan.FromSeconds(11); + opt.Endpoint.CircuitBreaker.FailureRatio = 0.99; + opt.Endpoint.CircuitBreaker.SamplingDuration = TimeSpan.FromSeconds(900); + opt.TotalRequestTimeout.Timeout = TimeSpan.FromSeconds(200); + opt.Endpoint.Timeout.Timeout = TimeSpan.FromSeconds(200); + }); + + using var provider = services.BuildServiceProvider(); + var clientFactory = provider.GetRequiredService(); + using var client = clientFactory.CreateClient(ClientName); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2)); + + var ex = await Record.ExceptionAsync(() => client.GetAsync("https://localhost:3000", cts.Token)); + Assert.Null(ex); + } + protected void AssertNoResponse() => Assert.Empty(_responses); protected void AddResponse(HttpStatusCode statusCode) => AddResponse(statusCode, 1); From 2f5ba8c16b8a096c592b9972039004f19d324da4 Mon Sep 17 00:00:00 2001 From: Nikita Balabaev Date: Thu, 2 May 2024 16:40:38 +0200 Subject: [PATCH 2/6] fix the test --- .../Hedging/HedgingTests.cs | 85 ++++------------ .../Hedging/OperationCanceledExceptionMock.cs | 16 +++ .../Hedging/StandardHedgingTests.cs | 97 ++++++++++++++++--- ...ft.Extensions.Http.Resilience.Tests.csproj | 5 - 4 files changed, 120 insertions(+), 83 deletions(-) create mode 100644 test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/OperationCanceledExceptionMock.cs diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/HedgingTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/HedgingTests.cs index 59b68ff1841..6b81d189ee6 100644 --- 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 : IDisposable private readonly Func _requestRoutingStrategyFactory; private readonly IServiceCollection _services; private readonly Queue _responses = new(); + private ServiceProvider? _serviceProvider; private bool _failure; private protected HedgingTests(Func, 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().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().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]); @@ -252,64 +258,6 @@ public async Task SendAsync_FailedExecution_ShouldReturnResponseFromHedging() Assert.Equal("https://enpoint-3:80/some-path?query", Requests[2]); } - [Fact] - public async Task SendAsync_FailedConnect_ShouldReturnResponseFromHedging() - { - const string ClientName = "HedgingClient"; - - // TODO: use mocked request destinations - - var services = new ServiceCollection(); - var clientBuilder = services - .AddHttpClient(ClientName) - .ConfigurePrimaryHttpMessageHandler(() => -#if NETFRAMEWORK - new WinHttpHandler - { - SendTimeout = TimeSpan.FromSeconds(1), -#else - new SocketsHttpHandler - { - ConnectTimeout = TimeSpan.FromSeconds(1), - SslOptions = new System.Net.Security.SslClientAuthenticationOptions - { - ClientCertificates = [], - RemoteCertificateValidationCallback = (_, _, _, _) => true - }, -#endif - }) - .AddStandardHedgingHandler(routing => - routing.ConfigureOrderedGroups(g => - { - g.Groups.Add(new UriEndpointGroup - { - Endpoints = [new WeightedUriEndpoint { Uri = new Uri("https://localhost:3000") }] - }); - - g.Groups.Add(new UriEndpointGroup - { - Endpoints = [new WeightedUriEndpoint { Uri = new Uri("https://microsoft.com") }] - }); - })) - .Configure(opt => - { - opt.Hedging.MaxHedgedAttempts = 10; - opt.Hedging.Delay = TimeSpan.FromSeconds(11); - opt.Endpoint.CircuitBreaker.FailureRatio = 0.99; - opt.Endpoint.CircuitBreaker.SamplingDuration = TimeSpan.FromSeconds(900); - opt.TotalRequestTimeout.Timeout = TimeSpan.FromSeconds(200); - opt.Endpoint.Timeout.Timeout = TimeSpan.FromSeconds(200); - }); - - using var provider = services.BuildServiceProvider(); - var clientFactory = provider.GetRequiredService(); - using var client = clientFactory.CreateClient(ClientName); - using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2)); - - var ex = await Record.ExceptionAsync(() => client.GetAsync("https://localhost:3000", cts.Token)); - Assert.Null(ex); - } - protected void AssertNoResponse() => Assert.Empty(_responses); protected void AddResponse(HttpStatusCode statusCode) => AddResponse(statusCode, 1); @@ -326,7 +274,12 @@ protected void AddResponse(HttpStatusCode statusCode, int count) protected abstract void ConfigureHedgingOptions(Action configure); - protected HttpClient CreateClientWithHandler() => _services.BuildServiceProvider().GetRequiredService().CreateClient(ClientId); + protected HttpClient CreateClientWithHandler() + { + _serviceProvider?.Dispose(); + _serviceProvider = _services.BuildServiceProvider(); + return _serviceProvider.GetRequiredService().CreateClient(ClientId); + } private Task InnerHandlerFunction(HttpRequestMessage request, CancellationToken cancellationToken) { diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/OperationCanceledExceptionMock.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/OperationCanceledExceptionMock.cs new file mode 100644 index 00000000000..7ea18c5782a --- /dev/null +++ b/test/Libraries/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; } +} diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs index fb381f31e1b..b53ed33d7eb 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs @@ -62,7 +62,8 @@ public void Configure_Callback_Ok() { Builder.Configure(o => o.Hedging.MaxHedgedAttempts = 8); - var options = Builder.Services.BuildServiceProvider().GetRequiredService>().Get(Builder.Name); + using var serviceProvider = Builder.Services.BuildServiceProvider(); + var options = serviceProvider.GetRequiredService>().Get(Builder.Name); Assert.Equal(8, options.Hedging.MaxHedgedAttempts); } @@ -76,7 +77,8 @@ public void Configure_CallbackWithServiceProvider_Ok() o.Hedging.MaxHedgedAttempts = 8; }); - var options = Builder.Services.BuildServiceProvider().GetRequiredService>().Get(Builder.Name); + using var serviceProvider = Builder.Services.BuildServiceProvider(); + var options = serviceProvider.GetRequiredService>().Get(Builder.Name); Assert.Equal(8, options.Hedging.MaxHedgedAttempts); } @@ -97,7 +99,8 @@ public void Configure_ValidConfigurationSection_ShouldInitialize() Builder.Configure(section); - var options = Builder.Services.BuildServiceProvider().GetRequiredService>().Get(Builder.Name); + using var serviceProvider = Builder.Services.BuildServiceProvider(); + var options = serviceProvider.GetRequiredService>().Get(Builder.Name); Assert.Equal(8, options.Hedging.MaxHedgedAttempts); } @@ -105,7 +108,8 @@ public void Configure_ValidConfigurationSection_ShouldInitialize() [Fact] public void ActionGenerator_Ok() { - var options = Builder.Services.BuildServiceProvider().GetRequiredService>().Get(Builder.Name); + using var serviceProvider = Builder.Services.BuildServiceProvider(); + var options = serviceProvider.GetRequiredService>().Get(Builder.Name); var generator = options.Hedging.ActionGenerator; var primary = ResilienceContextPool.Shared.Get(); var secondary = ResilienceContextPool.Shared.Get(); @@ -133,9 +137,12 @@ public void Configure_InvalidConfigurationSection_ShouldThrow() Builder.Configure(section); Assert.Throws(() => - Builder.Services.BuildServiceProvider() - .GetRequiredService>() - .Get(Builder.Name)); + { + using var serviceProvider = Builder.Services.BuildServiceProvider(); + return serviceProvider + .GetRequiredService>() + .Get(Builder.Name); + }); } #endif @@ -163,7 +170,7 @@ public void Configure_EmptyConfigurationSection_ShouldThrow() [Fact] public void VerifyPipeline() { - var serviceProvider = Builder.Services.BuildServiceProvider(); + using var serviceProvider = Builder.Services.BuildServiceProvider(); var pipelineProvider = serviceProvider.GetRequiredService>(); // primary handler @@ -209,7 +216,7 @@ public async Task VerifyPipelineSelection(string? customKey) using var request = new HttpRequestMessage(HttpMethod.Get, "https://key:80/discarded"); AddResponse(HttpStatusCode.OK); - var response = await client.SendAsync(request, CancellationToken.None); + using var response = await client.SendAsync(request, CancellationToken.None); provider.VerifyAll(); } @@ -235,14 +242,14 @@ public async Task DynamicReloads_Ok() // act && assert AddResponse(HttpStatusCode.InternalServerError, 3); using var firstRequest = new HttpRequestMessage(HttpMethod.Get, "https://to-be-replaced:1234/some-path?query"); - await client.SendAsync(firstRequest); + using var _ = await client.SendAsync(firstRequest); AssertNoResponse(); reloadAction(new() { { "standard:Hedging:MaxHedgedAttempts", "6" } }); AddResponse(HttpStatusCode.InternalServerError, 7); using var secondRequest = new HttpRequestMessage(HttpMethod.Get, "https://to-be-replaced:1234/some-path?query"); - await client.SendAsync(secondRequest); + using var __ = await client.SendAsync(secondRequest); AssertNoResponse(); } @@ -257,11 +264,77 @@ public async Task NoRouting_Ok() // act && assert AddResponse(HttpStatusCode.InternalServerError, 3); using var firstRequest = new HttpRequestMessage(HttpMethod.Get, "https://some-endpoint:1234/some-path?query"); - await client.SendAsync(firstRequest); + using var _ = await client.SendAsync(firstRequest); AssertNoResponse(); Requests.Should().AllSatisfy(r => r.Should().Be("https://some-endpoint:1234/some-path?query")); } + [Fact] + public async Task SendAsync_FailedConnect_ShouldReturnResponseFromHedging() + { + const string FailingEndpoint = "www.failing-host.com"; + + var services = new ServiceCollection(); + var clientBuilder = services + .AddHttpClient(ClientId) + .ConfigurePrimaryHttpMessageHandler(() => new MockHttpMessageHandler(FailingEndpoint)) + .AddStandardHedgingHandler(routing => + routing.ConfigureOrderedGroups(g => + { + g.Groups.Add(new UriEndpointGroup + { + Endpoints = [new WeightedUriEndpoint { Uri = new Uri($"https://{FailingEndpoint}:3000") }] + }); + + g.Groups.Add(new UriEndpointGroup + { + Endpoints = [new WeightedUriEndpoint { Uri = new Uri("https://microsoft.com") }] + }); + })) + .Configure(opt => + { + opt.Hedging.MaxHedgedAttempts = 10; + opt.Hedging.Delay = TimeSpan.FromSeconds(11); + opt.Endpoint.CircuitBreaker.FailureRatio = 0.99; + opt.Endpoint.CircuitBreaker.SamplingDuration = TimeSpan.FromSeconds(900); + opt.TotalRequestTimeout.Timeout = TimeSpan.FromSeconds(200); + opt.Endpoint.Timeout.Timeout = TimeSpan.FromSeconds(200); + }); + + using var provider = services.BuildServiceProvider(); + var clientFactory = provider.GetRequiredService(); + using var client = clientFactory.CreateClient(ClientId); + + var ex = await Record.ExceptionAsync(async () => + { + using var _ = await client.GetAsync($"https://{FailingEndpoint}:3000"); + }); + + Assert.Null(ex); + } + protected override void ConfigureHedgingOptions(Action configure) => Builder.Configure(options => configure(options.Hedging)); + + private class MockHttpMessageHandler : HttpMessageHandler + { + private readonly string _failingEndpoint; + + public MockHttpMessageHandler(string failingEndpoint) + { + _failingEndpoint = failingEndpoint; + } + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + if (request.RequestUri?.Host == _failingEndpoint) + { + await Task.Delay(100, cancellationToken); + throw new OperationCanceledExceptionMock(new TimeoutException()); + } + + await Task.Delay(1000, cancellationToken); + return new HttpResponseMessage(HttpStatusCode.OK); + } + } } diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Microsoft.Extensions.Http.Resilience.Tests.csproj b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Microsoft.Extensions.Http.Resilience.Tests.csproj index 266794186c7..95e047fabb3 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Microsoft.Extensions.Http.Resilience.Tests.csproj +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Microsoft.Extensions.Http.Resilience.Tests.csproj @@ -4,11 +4,6 @@ Unit tests for Microsoft.Extensions.Http.Resilience. - - - true - - From 21f3d17b4f917766dfc73bbd0fe3ed966e4a8cb1 Mon Sep 17 00:00:00 2001 From: Nikita Balabaev Date: Mon, 6 May 2024 15:36:42 +0200 Subject: [PATCH 3/6] CR --- .../HttpClientHedgingResiliencePredicates.cs | 10 +++++++++- .../Hedging/StandardHedgingTests.cs | 19 ++++++------------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs index 585bd4b5a2f..f3ff38f66cc 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs @@ -2,7 +2,9 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Diagnostics.CodeAnalysis; using System.Net.Http; +using Microsoft.Shared.DiagnosticIds; using Microsoft.Shared.Diagnostics; using Polly; using Polly.CircuitBreaker; @@ -27,7 +29,13 @@ public static bool IsTransient(Outcome outcome) _ => false, }; - internal static bool IsTransient(HedgingPredicateArguments args) + /// + /// Determines whether an should be treated by hedging as a transient failure. + /// + /// A . + /// if outcome is transient, if not. + [Experimental(diagnosticId: DiagnosticIds.Experiments.Resilience, UrlFormat = DiagnosticIds.UrlFormat)] + public static bool IsTransient(HedgingPredicateArguments args) => IsConnectionTimeout(args) || IsTransient(args.Outcome); diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs index b53ed33d7eb..debafe03127 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs @@ -46,7 +46,7 @@ public void EnsureValidated_BasicValidation() { Builder.Configure(options => options.Hedging.MaxHedgedAttempts = -1); - Assert.Throws(() => CreateClientWithHandler()); + Assert.Throws(CreateClientWithHandler); } [Fact] @@ -54,7 +54,7 @@ public void EnsureValidated_AdvancedValidation() { Builder.Configure(options => options.TotalRequestTimeout.Timeout = TimeSpan.FromSeconds(1)); - Assert.Throws(() => CreateClientWithHandler()); + Assert.Throws(CreateClientWithHandler); } [Fact] @@ -276,7 +276,7 @@ public async Task SendAsync_FailedConnect_ShouldReturnResponseFromHedging() const string FailingEndpoint = "www.failing-host.com"; var services = new ServiceCollection(); - var clientBuilder = services + _ = services .AddHttpClient(ClientId) .ConfigurePrimaryHttpMessageHandler(() => new MockHttpMessageHandler(FailingEndpoint)) .AddStandardHedgingHandler(routing => @@ -302,7 +302,7 @@ public async Task SendAsync_FailedConnect_ShouldReturnResponseFromHedging() opt.Endpoint.Timeout.Timeout = TimeSpan.FromSeconds(200); }); - using var provider = services.BuildServiceProvider(); + await using var provider = services.BuildServiceProvider(); var clientFactory = provider.GetRequiredService(); using var client = clientFactory.CreateClient(ClientId); @@ -316,18 +316,11 @@ public async Task SendAsync_FailedConnect_ShouldReturnResponseFromHedging() protected override void ConfigureHedgingOptions(Action configure) => Builder.Configure(options => configure(options.Hedging)); - private class MockHttpMessageHandler : HttpMessageHandler + private class MockHttpMessageHandler(string failingEndpoint) : HttpMessageHandler { - private readonly string _failingEndpoint; - - public MockHttpMessageHandler(string failingEndpoint) - { - _failingEndpoint = failingEndpoint; - } - protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - if (request.RequestUri?.Host == _failingEndpoint) + if (request.RequestUri?.Host == failingEndpoint) { await Task.Delay(100, cancellationToken); throw new OperationCanceledExceptionMock(new TimeoutException()); From 72fdb381294bdbaabf22dade498e52d532dc5f8d Mon Sep 17 00:00:00 2001 From: Nikita Balabaev Date: Tue, 7 May 2024 12:51:58 +0200 Subject: [PATCH 4/6] CR, 2nd round --- .../HttpClientHedgingResiliencePredicates.cs | 20 ++++++++++--------- .../Hedging/HttpHedgingStrategyOptions.cs | 2 +- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs index f3ff38f66cc..895b6cbf0f0 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs @@ -4,11 +4,11 @@ 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; -using Polly.Hedging; namespace Microsoft.Extensions.Http.Resilience; @@ -20,6 +20,7 @@ public static class HttpClientHedgingResiliencePredicates /// /// Determines whether an outcome should be treated by hedging as a transient failure. /// + /// The outcome of the user-specified callback. /// if outcome is transient, if not. public static bool IsTransient(Outcome outcome) => outcome switch @@ -32,17 +33,18 @@ public static bool IsTransient(Outcome outcome) /// /// Determines whether an should be treated by hedging as a transient failure. /// - /// A . + /// The outcome of the user-specified callback. + /// The associated with the execution. /// if outcome is transient, if not. [Experimental(diagnosticId: DiagnosticIds.Experiments.Resilience, UrlFormat = DiagnosticIds.UrlFormat)] - public static bool IsTransient(HedgingPredicateArguments args) - => IsConnectionTimeout(args) - || IsTransient(args.Outcome); + public static bool IsTransient(Outcome outcome, CancellationToken cancellationToken) + => IsConnectionTimeout(outcome, cancellationToken) + || IsTransient(outcome); - internal static bool IsConnectionTimeout(HedgingPredicateArguments args) - => !args.Context.CancellationToken.IsCancellationRequested - && args.Outcome.Exception is OperationCanceledException { Source: "System.Private.CoreLib" } - && args.Outcome.Exception.InnerException is TimeoutException; + internal static bool IsConnectionTimeout(in Outcome outcome, in CancellationToken cancellationToken) + => !cancellationToken.IsCancellationRequested + && outcome.Exception is OperationCanceledException { Source: "System.Private.CoreLib" } + && outcome.Exception.InnerException is TimeoutException; /// /// Determines whether an exception should be treated by hedging as a transient failure. diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs index f4fd9cef3f4..74aa37640ca 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs @@ -21,6 +21,6 @@ public class HttpHedgingStrategyOptions : HedgingStrategyOptions public HttpHedgingStrategyOptions() { - ShouldHandle = args => new ValueTask(HttpClientHedgingResiliencePredicates.IsTransient(args)); + ShouldHandle = args => new ValueTask(HttpClientHedgingResiliencePredicates.IsTransient(args.Outcome, args.Context.CancellationToken)); } } From 6d3eb4ce79618d99cad805b8ec55cfad2a391bc7 Mon Sep 17 00:00:00 2001 From: Nikita Balabaev Date: Tue, 7 May 2024 14:21:43 +0200 Subject: [PATCH 5/6] Use connection timeout check in common strategies --- .../HttpClientHedgingResiliencePredicates.cs | 9 ++------ .../Hedging/HttpHedgingStrategyOptions.cs | 2 +- .../Polly/HttpClientResiliencePredicates.cs | 23 ++++++++++++++++--- .../Polly/HttpRetryStrategyOptions.cs | 2 +- 4 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs index 895b6cbf0f0..81358b4801d 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientHedgingResiliencePredicates.cs @@ -38,13 +38,8 @@ public static bool IsTransient(Outcome outcome) /// if outcome is transient, if not. [Experimental(diagnosticId: DiagnosticIds.Experiments.Resilience, UrlFormat = DiagnosticIds.UrlFormat)] public static bool IsTransient(Outcome outcome, CancellationToken cancellationToken) - => IsConnectionTimeout(outcome, cancellationToken) - || IsTransient(outcome); - - internal static bool IsConnectionTimeout(in Outcome outcome, in CancellationToken cancellationToken) - => !cancellationToken.IsCancellationRequested - && outcome.Exception is OperationCanceledException { Source: "System.Private.CoreLib" } - && outcome.Exception.InnerException is TimeoutException; + => HttpClientResiliencePredicates.IsHttpConnectionTimeout(outcome, cancellationToken) + || IsTransient(outcome); /// /// Determines whether an exception should be treated by hedging as a transient failure. diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs index 74aa37640ca..07272cc9916 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpHedgingStrategyOptions.cs @@ -16,7 +16,7 @@ public class HttpHedgingStrategyOptions : HedgingStrategyOptions class. /// /// - /// 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 exceptions. /// public HttpHedgingStrategyOptions() diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpClientResiliencePredicates.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpClientResiliencePredicates.cs index 0990c779842..3345c75f11a 100644 --- 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,6 +29,17 @@ public static class HttpClientResiliencePredicates _ => false }; + /// + /// Determines whether an should be treated by resilience strategies as a transient failure. + /// + /// The outcome of the user-specified callback. + /// The associated with the execution. + /// if outcome is transient, if not. + [Experimental(diagnosticId: DiagnosticIds.Experiments.Resilience, UrlFormat = DiagnosticIds.UrlFormat)] + public static bool IsTransient(Outcome outcome, CancellationToken cancellationToken) + => IsHttpConnectionTimeout(outcome, cancellationToken) + || IsTransient(outcome); + /// /// Determines whether an exception should be treated by resilience strategies as a transient failure. /// @@ -33,10 +47,14 @@ 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 outcome, in CancellationToken cancellationToken) + => !cancellationToken.IsCancellationRequested + && outcome.Exception is OperationCanceledException { Source: "System.Private.CoreLib" } + && outcome.Exception.InnerException is TimeoutException; + /// /// Determines whether a response contains a transient failure. /// @@ -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 index 2153862f8a4..afde7d0372d 100644 --- 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 public HttpRetryStrategyOptions() { - ShouldHandle = args => new ValueTask(HttpClientResiliencePredicates.IsTransient(args.Outcome)); + ShouldHandle = args => new ValueTask(HttpClientResiliencePredicates.IsTransient(args.Outcome, args.Context.CancellationToken)); BackoffType = DelayBackoffType.Exponential; ShouldRetryAfterHeader = true; UseJitter = true; From 80d518bdfb0b987f1fce1c3f69316624d2857ac0 Mon Sep 17 00:00:00 2001 From: Nikita Balabaev Date: Thu, 9 May 2024 15:00:42 +0200 Subject: [PATCH 6/6] add more tests --- .../Polly/HttpRetryStrategyOptionsTests.cs | 36 ++++++++++--------- ...tpClientBuilderExtensionsTests.Standard.cs | 6 ++-- .../HttpStandardResilienceOptionsTests.cs | 7 +--- 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsTests.cs index 56fb80c56f9..a9fac7d3e0b 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsTests.cs @@ -6,8 +6,10 @@ using System.Net; using System.Net.Http; using System.Net.Http.Headers; +using System.Threading; using System.Threading.Tasks; using FluentAssertions; +using Microsoft.Extensions.Http.Resilience.Test.Hedging; using Polly; using Polly.Retry; using Xunit; @@ -19,16 +21,15 @@ public class HttpRetryStrategyOptionsTests #pragma warning disable S2330 public static readonly IEnumerable HandledExceptionsClassified = new[] { - new object[] { new InvalidCastException(), false }, - new object[] { new HttpRequestException(), true } + new object[] { new InvalidCastException(), null!, false }, + [new HttpRequestException(), null!, true], + [new OperationCanceledExceptionMock(new TimeoutException()), null!, true], + [new OperationCanceledExceptionMock(new TimeoutException()), default(CancellationToken), true], + [new OperationCanceledExceptionMock(new InvalidOperationException()), default(CancellationToken), false], + [new OperationCanceledExceptionMock(new TimeoutException()), new CancellationToken(canceled: true), false], }; - private readonly HttpRetryStrategyOptions _testClass; - - public HttpRetryStrategyOptionsTests() - { - _testClass = new HttpRetryStrategyOptions(); - } + private readonly HttpRetryStrategyOptions _testClass = new(); [Fact] public void Ctor_Defaults() @@ -63,9 +64,10 @@ public async Task ShouldHandleResultAsError_DefaultValue_ShouldClassify(HttpStat [Theory] [MemberData(nameof(HandledExceptionsClassified))] - public async Task ShouldHandleException_DefaultValue_ShouldClassify(Exception exception, bool expectedToHandle) + public async Task ShouldHandleException_DefaultValue_ShouldClassify(Exception exception, CancellationToken? token, bool expectedToHandle) { - var shouldHandle = await _testClass.ShouldHandle(CreateArgs(Outcome.FromException(exception))); + var args = CreateArgs(Outcome.FromException(exception), token ?? default); + var shouldHandle = await _testClass.ShouldHandle(args); Assert.Equal(expectedToHandle, shouldHandle); } @@ -86,9 +88,10 @@ public async Task ShouldHandleResultAsError_DefaultInstance_ShouldClassify(HttpS [Theory] [MemberData(nameof(HandledExceptionsClassified))] - public async Task ShouldHandleException_DefaultInstance_ShouldClassify(Exception exception, bool expectedToHandle) + public async Task ShouldHandleException_DefaultInstance_ShouldClassify(Exception exception, CancellationToken? token, bool expectedToHandle) { - var shouldHandle = await new HttpRetryStrategyOptions().ShouldHandle(CreateArgs(Outcome.FromException(exception))); + var args = CreateArgs(Outcome.FromException(exception), token ?? default); + var shouldHandle = await new HttpRetryStrategyOptions().ShouldHandle(args); Assert.Equal(expectedToHandle, shouldHandle); } @@ -96,7 +99,7 @@ public async Task ShouldHandleException_DefaultInstance_ShouldClassify(Exception public async Task ShouldRetryAfterHeader_InvalidOutcomes_ShouldReturnNull() { var options = new HttpRetryStrategyOptions { ShouldRetryAfterHeader = true }; - using var responseMessage = new HttpResponseMessage { }; + using var responseMessage = new HttpResponseMessage(); Assert.NotNull(options.DelayGenerator); @@ -153,7 +156,8 @@ public void GetDelayGenerator_ShouldGetBasedOnShouldRetryAfterHeader(bool should Assert.Equal(shouldRetryAfterHeader, options.DelayGenerator != null); } - private static RetryPredicateArguments CreateArgs(Outcome outcome) - => new(ResilienceContextPool.Shared.Get(), outcome, 0); - + private static RetryPredicateArguments CreateArgs( + Outcome outcome, + CancellationToken cancellationToken = default) + => new(ResilienceContextPool.Shared.Get(cancellationToken), outcome, 0); } diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/HttpClientBuilderExtensionsTests.Standard.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/HttpClientBuilderExtensionsTests.Standard.cs index afa4233c797..7f0143ded57 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/HttpClientBuilderExtensionsTests.Standard.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/HttpClientBuilderExtensionsTests.Standard.cs @@ -128,7 +128,7 @@ public void AddStandardResilienceHandler_ConfigurationPropertyWithTypo_Throws(Me AddStandardResilienceHandler(mode, builder, _invalidConfigurationSection, options => { }); - Assert.Throws(() => HttpClientBuilderExtensionsTests.GetPipeline(builder.Services, $"test-standard")); + Assert.Throws(() => HttpClientBuilderExtensionsTests.GetPipeline(builder.Services, "test-standard")); } [Fact] @@ -175,7 +175,7 @@ public void AddStandardResilienceHandler_EnsureValidated(bool wholePipeline) } }); - Assert.Throws(() => GetPipeline(builder.Services, $"test-standard")); + Assert.Throws(() => GetPipeline(builder.Services, "test-standard")); } [InlineData(MethodArgs.None)] @@ -193,7 +193,7 @@ public void AddStandardResilienceHandler_EnsureConfigured(MethodArgs mode) AddStandardResilienceHandler(mode, builder, _validConfigurationSection, options => { }); - var pipeline = GetPipeline(builder.Services, $"test-standard"); + var pipeline = GetPipeline(builder.Services, "test-standard"); Assert.NotNull(pipeline); } diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/HttpStandardResilienceOptionsTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/HttpStandardResilienceOptionsTests.cs index 5a216aac6ec..9d55b1f8c0e 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/HttpStandardResilienceOptionsTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/HttpStandardResilienceOptionsTests.cs @@ -9,12 +9,7 @@ namespace Microsoft.Extensions.Http.Resilience.Test.Resilience; public class HttpStandardResilienceOptionsTests { - private readonly HttpStandardResilienceOptions _options; - - public HttpStandardResilienceOptionsTests() - { - _options = new HttpStandardResilienceOptions(); - } + private readonly HttpStandardResilienceOptions _options = new(); [Fact] public void Ctor_EnsureDefaults()