From fe7ed5f375591b8b62f16b371f913a926c53ac76 Mon Sep 17 00:00:00 2001 From: Iliar Turdushev Date: Wed, 13 Nov 2024 10:19:46 +0100 Subject: [PATCH 1/2] Fixes #5248 Adds APIs allowing to disable automatic retries for a given list of HTTP methods --- .../HttpRetryStrategyOptionsExtensions.cs | 67 +++++++++++ ...HttpRetryStrategyOptionsExtensionsTests.cs | 108 ++++++++++++++++++ 2 files changed, 175 insertions(+) create mode 100644 src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptionsExtensions.cs create mode 100644 test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsExtensionsTests.cs diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptionsExtensions.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptionsExtensions.cs new file mode 100644 index 00000000000..086447f910c --- /dev/null +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptionsExtensions.cs @@ -0,0 +1,67 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics.CodeAnalysis; +using System.Linq; +using System.Net.Http; +using System.Threading.Tasks; +using Microsoft.Shared.DiagnosticIds; +using Microsoft.Shared.Diagnostics; +using Polly; + +namespace Microsoft.Extensions.Http.Resilience; + +/// +/// Extensions for . +/// +[Experimental(diagnosticId: DiagnosticIds.Experiments.Resilience, UrlFormat = DiagnosticIds.UrlFormat)] +public static class HttpRetryStrategyOptionsExtensions +{ +#if !NET8_0_OR_GREATER + private static readonly HttpMethod _connect = new("CONNECT"); + private static readonly HttpMethod _patch = new("PATCH"); +#endif + + /// + /// Disables retry attempts for POST, PATCH, PUT, DELETE, and CONNECT HTTP methods. + /// + /// The retry strategy options. + public static void DisableForUnsafeHttpMethods(this HttpRetryStrategyOptions options) + { + options.DisableFor( + HttpMethod.Delete, HttpMethod.Post, HttpMethod.Put, +#if !NET8_0_OR_GREATER + _connect, _patch); +#else + HttpMethod.Connect, HttpMethod.Patch); +#endif + } + + /// + /// Disables retry attempts for the given list of HTTP methods. + /// + /// The retry strategy options. + /// The list of HTTP methods. + public static void DisableFor(this HttpRetryStrategyOptions options, params HttpMethod[] methods) + { + _ = Throw.IfNull(options); + _ = Throw.IfNullOrEmpty(methods); + + var shouldHandle = options.ShouldHandle; + + options.ShouldHandle = async args => + { + var result = await shouldHandle(args).ConfigureAwait(args.Context.ContinueOnCapturedContext); + + if (result && + args.Outcome.Result is HttpResponseMessage response && + response.RequestMessage is HttpRequestMessage request) + { + return !methods.Contains(request.Method); + } + + return result; + }; + } +} + diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsExtensionsTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsExtensionsTests.cs new file mode 100644 index 00000000000..6d5e55fda51 --- /dev/null +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsExtensionsTests.cs @@ -0,0 +1,108 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Net.Http; +using System.Threading.Tasks; +using Polly; +using Polly.Retry; +using Xunit; + +namespace Microsoft.Extensions.Http.Resilience.Test.Polly; + +public class HttpRetryStrategyOptionsExtensionsTests +{ + [Fact] + public void DisableFor_RetryOptionsIsNull_Throws() + { + Assert.Throws(() => ((HttpRetryStrategyOptions)null!).DisableFor(HttpMethod.Get)); + } + + [Fact] + public void DisableFor_HttpMethodsIsNull_Throws() + { + Assert.Throws(() => new HttpRetryStrategyOptions().DisableFor(null!)); + } + + [Fact] + public void DisableFor_HttpMethodsIsEmptry_Throws() + { + Assert.Throws(() => new HttpRetryStrategyOptions().DisableFor([])); + } + + [Theory] + [InlineData("POST", false)] + [InlineData("DELETE", false)] + [InlineData("GET", true)] + public async Task DisableFor_PositiveScenario(string httpMethod, bool shouldHandle) + { + var options = new HttpRetryStrategyOptions { ShouldHandle = _ => PredicateResult.True() }; + options.DisableFor(HttpMethod.Post, HttpMethod.Delete); + + using var request = new HttpRequestMessage { Method = new HttpMethod(httpMethod) }; + using var response = new HttpResponseMessage { RequestMessage = request }; + + Assert.Equal(shouldHandle, await options.ShouldHandle(CreatePredicateArguments(response))); + } + + [Fact] + public async Task DisableFor_RespectsOriginalShouldHandlePredicate() + { + var options = new HttpRetryStrategyOptions { ShouldHandle = _ => PredicateResult.False() }; + options.DisableFor(HttpMethod.Post); + + using var request = new HttpRequestMessage { Method = HttpMethod.Get }; + using var response = new HttpResponseMessage { RequestMessage = request }; + + Assert.False(await options.ShouldHandle(CreatePredicateArguments(response))); + } + + [Fact] + public async Task DisableFor_ResponseMessageIsNull_DoesNotDisableRetries() + { + var options = new HttpRetryStrategyOptions { ShouldHandle = _ => PredicateResult.True() }; + options.DisableFor(HttpMethod.Post); + + Assert.True(await options.ShouldHandle(CreatePredicateArguments(null))); + } + + [Fact] + public async Task DisableFor_RequestMessageIsNull_DoesNotDisableRetries() + { + var options = new HttpRetryStrategyOptions { ShouldHandle = _ => PredicateResult.True() }; + options.DisableFor(HttpMethod.Post); + + using var response = new HttpResponseMessage { RequestMessage = null }; + + Assert.True(await options.ShouldHandle(CreatePredicateArguments(response))); + } + + [Theory] + [InlineData("POST", false)] + [InlineData("DELETE", false)] + [InlineData("PUT", false)] + [InlineData("PATCH", false)] + [InlineData("CONNECT", false)] + [InlineData("GET", true)] + [InlineData("HEAD", true)] + [InlineData("TRACE", true)] + [InlineData("OPTIONS", true)] + public async Task DisableForUnsafeHttpMethods_PositiveScenario(string httpMethod, bool shouldHandle) + { + var options = new HttpRetryStrategyOptions { ShouldHandle = _ => PredicateResult.True() }; + options.DisableForUnsafeHttpMethods(); + + using var request = new HttpRequestMessage { Method = new HttpMethod(httpMethod) }; + using var response = new HttpResponseMessage { RequestMessage = request }; + + Assert.Equal(shouldHandle, await options.ShouldHandle(CreatePredicateArguments(response))); + } + + private static RetryPredicateArguments CreatePredicateArguments(HttpResponseMessage? response) + { + return new RetryPredicateArguments( + ResilienceContextPool.Shared.Get(), + Outcome.FromResult(response), + attemptNumber: 1); + } +} From 8d7a2209e95f142bfd3d1146bc2ad2b97450e048 Mon Sep 17 00:00:00 2001 From: Iliar Turdushev Date: Wed, 20 Nov 2024 13:14:10 +0100 Subject: [PATCH 2/2] Fixes #5248 Adds a check ensuring options.ShouldHandle is not null --- .../Polly/HttpRetryStrategyOptionsExtensions.cs | 3 +-- .../Polly/HttpRetryStrategyOptionsExtensionsTests.cs | 7 +++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptionsExtensions.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptionsExtensions.cs index 086447f910c..85168988c7b 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptionsExtensions.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptionsExtensions.cs @@ -44,10 +44,9 @@ public static void DisableForUnsafeHttpMethods(this HttpRetryStrategyOptions opt /// The list of HTTP methods. public static void DisableFor(this HttpRetryStrategyOptions options, params HttpMethod[] methods) { - _ = Throw.IfNull(options); _ = Throw.IfNullOrEmpty(methods); - var shouldHandle = options.ShouldHandle; + var shouldHandle = Throw.IfNullOrMemberNull(options, options?.ShouldHandle); options.ShouldHandle = async args => { diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsExtensionsTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsExtensionsTests.cs index 6d5e55fda51..4d43c020d1f 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsExtensionsTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Polly/HttpRetryStrategyOptionsExtensionsTests.cs @@ -30,6 +30,13 @@ public void DisableFor_HttpMethodsIsEmptry_Throws() Assert.Throws(() => new HttpRetryStrategyOptions().DisableFor([])); } + [Fact] + public void DisableFor_ShouldHandleIsNull_Throws() + { + var options = new HttpRetryStrategyOptions { ShouldHandle = null! }; + Assert.Throws(() => options.DisableFor(HttpMethod.Get)); + } + [Theory] [InlineData("POST", false)] [InlineData("DELETE", false)]