From 5b9145e20b82537d9e592bd8792d4ce612075047 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Mon, 19 Jun 2023 10:29:47 +0200 Subject: [PATCH 1/3] Fix OnHedging not being called --- .../Hedging/HedgingResilienceStrategy.cs | 40 ++++++++++++++----- .../Hedging/HedgingStrategyOptions.TResult.cs | 4 ++ src/Polly.Core/Hedging/OnHedgingArguments.cs | 7 +++- ...esilienceStrategyBuilderExtensionsTests.cs | 16 +++++++- .../Hedging/HedgingResilienceStrategyTests.cs | 29 ++++++++++++++ 5 files changed, 83 insertions(+), 13 deletions(-) diff --git a/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs b/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs index 5a941f8edb6..88f8d763b0c 100644 --- a/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs +++ b/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs @@ -69,8 +69,10 @@ private async ValueTask> ExecuteCoreAsync( ResilienceContext context, TState state) { + var attempt = -1; while (true) { + attempt++; var continueOnCapturedContext = context.ContinueOnCapturedContext; var cancellationToken = context.CancellationToken; @@ -79,7 +81,9 @@ private async ValueTask> ExecuteCoreAsync( return new Outcome(new OperationCanceledException(cancellationToken).TrySetStackTrace()); } - if ((await hedgingContext.LoadExecutionAsync(callback, state).ConfigureAwait(context.ContinueOnCapturedContext)).Outcome is Outcome outcome) + var loadedExecution = await hedgingContext.LoadExecutionAsync(callback, state).ConfigureAwait(context.ContinueOnCapturedContext); + + if (loadedExecution.Outcome is Outcome outcome) { return outcome; } @@ -90,6 +94,10 @@ private async ValueTask> ExecuteCoreAsync( { // If completedHedgedTask is null it indicates that we still do not have any finished hedged task within the hedging delay. // We will create additional hedged task in the next iteration. + await HandleOnHedgingAsync( + context, + new Outcome(default(TResult)), + new OnHedgingArguments(attempt, HasOutcome: false)).ConfigureAwait(context.ContinueOnCapturedContext); continue; } @@ -101,16 +109,28 @@ private async ValueTask> ExecuteCoreAsync( return outcome; } - var onHedgingArgs = new OutcomeArguments(context, outcome, new OnHedgingArguments(context, hedgingContext.LoadedTasks - 1)); - _telemetry.Report(HedgingConstants.OnHedgingEventName, onHedgingArgs); + await HandleOnHedgingAsync( + context, + outcome, + new OnHedgingArguments(attempt, HasOutcome: true)).ConfigureAwait(context.ContinueOnCapturedContext); + } + } - if (OnHedging is not null) - { - // If nothing has been returned or thrown yet, the result is a transient failure, - // and other hedged request will be awaited. - // Before it, one needs to perform the task adjacent to each hedged call. - await OnHedging.HandleAsync(onHedgingArgs).ConfigureAwait(continueOnCapturedContext); - } + private async ValueTask HandleOnHedgingAsync(ResilienceContext context, Outcome outcome, OnHedgingArguments args) + { + var onHedgingArgs = new OutcomeArguments( + context, + outcome, + args); + + _telemetry.Report(HedgingConstants.OnHedgingEventName, onHedgingArgs); + + if (OnHedging is not null) + { + // If nothing has been returned or thrown yet, the result is a transient failure, + // and other hedged request will be awaited. + // Before it, one needs to perform the task adjacent to each hedged call. + await OnHedging.HandleAsync(onHedgingArgs).ConfigureAwait(context.ContinueOnCapturedContext); } } diff --git a/src/Polly.Core/Hedging/HedgingStrategyOptions.TResult.cs b/src/Polly.Core/Hedging/HedgingStrategyOptions.TResult.cs index 5a5fab3750a..093254cb5c4 100644 --- a/src/Polly.Core/Hedging/HedgingStrategyOptions.TResult.cs +++ b/src/Polly.Core/Hedging/HedgingStrategyOptions.TResult.cs @@ -85,6 +85,10 @@ public class HedgingStrategyOptions : ResilienceStrategyOptions /// /// /// Defaults to . + /// + /// The hedging is executed when the current attempt outcome is not successful and the predicate returns or when + /// the current attempt did not finish within the . + /// /// public Func, ValueTask>? OnHedging { get; set; } } diff --git a/src/Polly.Core/Hedging/OnHedgingArguments.cs b/src/Polly.Core/Hedging/OnHedgingArguments.cs index 199911a93af..a163d9796e7 100644 --- a/src/Polly.Core/Hedging/OnHedgingArguments.cs +++ b/src/Polly.Core/Hedging/OnHedgingArguments.cs @@ -3,6 +3,9 @@ namespace Polly.Hedging; /// /// Represents arguments used by the on-hedging event. /// -/// The context associated with the execution of a user-provided callback. /// The zero-based hedging attempt number. -public record OnHedgingArguments(ResilienceContext Context, int Attempt); +/// +/// Determines whether the outcome is available before loading the next hedged task. +/// No outcome indicates that the previous action did not finish within hedging delay. +/// +public record OnHedgingArguments(int Attempt, bool HasOutcome); diff --git a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyBuilderExtensionsTests.cs b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyBuilderExtensionsTests.cs index 8e6051b0903..8c79ccdbdf1 100644 --- a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyBuilderExtensionsTests.cs +++ b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyBuilderExtensionsTests.cs @@ -52,6 +52,7 @@ public void AddHedgingT_InvalidOptions_Throws() [Fact] public async Task AddHedging_IntegrationTest() { + var hedgingWithoutOutcome = false; ConcurrentQueue results = new(); var strategy = _builder @@ -78,7 +79,19 @@ public async Task AddHedging_IntegrationTest() return "error".AsOutcome().AsOutcome(); }; }, - OnHedging = args => { results.Enqueue(args.Result!.ToString()!); return default; } + OnHedging = args => + { + if (args.Arguments.HasOutcome) + { + results.Enqueue(args.Result!.ToString()!); + } + else + { + hedgingWithoutOutcome = true; + } + + return default; + } }) .Build(); @@ -91,5 +104,6 @@ public async Task AddHedging_IntegrationTest() result.Should().Be("success"); results.Should().HaveCountGreaterThan(0); results.Distinct().Should().ContainSingle("error"); + hedgingWithoutOutcome.Should().BeTrue(); } } diff --git a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs index 7f854df8c5d..f4e1585de0e 100644 --- a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs +++ b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs @@ -243,6 +243,35 @@ public async Task ExecuteAsync_EnsurePrimaryTaskCancelled_Ok() await task; } + [Fact] + public async Task ExecuteAsync_EnsureSecondaryHedgedTaskReportedWithNoOutcome() + { + // arrange + using var cancelled = new ManualResetEvent(false); + var hasOutcome = true; + _options.OnHedging = args => + { + hasOutcome = args.Arguments.HasOutcome; + return default; + }; + + ConfigureHedging(context => Success.AsOutcomeAsync()); + + var strategy = Create(); + + // act + var task = strategy.ExecuteAsync(async token => + { + await _timeProvider.Delay(TimeSpan.FromHours(24), token); + return Success; + }); + + // assert + _timeProvider.Advance(TimeSpan.FromHours(2)); + hasOutcome.Should().BeFalse(); + await task; + } + [Fact] public async Task ExecuteAsync_EnsureDiscardedResultDisposed() { From 55733d7dd7c6b8df4675fb7b1f876b966f4837f7 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Mon, 19 Jun 2023 10:38:14 +0200 Subject: [PATCH 2/3] improve tests --- test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs index f4e1585de0e..7801b062c4a 100644 --- a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs +++ b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs @@ -843,6 +843,7 @@ public async Task ExecuteAsync_EnsureOnHedgingCalled() var attempts = new List(); _options.OnHedging = args => { + args.Arguments.HasOutcome.Should().BeTrue(); args.Result.Should().Be(Failure); attempts.Add(args.Arguments.Attempt); return default; From 907bde2def8ad699d01c425b3d8f9a813ecaf752 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Mon, 19 Jun 2023 10:47:11 +0200 Subject: [PATCH 3/3] fixes --- src/Polly.Core/Hedging/OnHedgingArguments.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Polly.Core/Hedging/OnHedgingArguments.cs b/src/Polly.Core/Hedging/OnHedgingArguments.cs index a163d9796e7..b1930cced0b 100644 --- a/src/Polly.Core/Hedging/OnHedgingArguments.cs +++ b/src/Polly.Core/Hedging/OnHedgingArguments.cs @@ -6,6 +6,6 @@ namespace Polly.Hedging; /// The zero-based hedging attempt number. /// /// Determines whether the outcome is available before loading the next hedged task. -/// No outcome indicates that the previous action did not finish within hedging delay. +/// No outcome indicates that the previous action did not finish within the hedging delay. /// public record OnHedgingArguments(int Attempt, bool HasOutcome);