From 6fcf0f68a116868f18d5be97e32d7a442d85f140 Mon Sep 17 00:00:00 2001 From: martintmk <103487740+martintmk@users.noreply.github.com> Date: Mon, 19 Jun 2023 11:23:11 +0200 Subject: [PATCH] Fix OnHedging not being called (#1320) --- .../Hedging/HedgingResilienceStrategy.cs | 40 ++++++++++++++----- .../Hedging/HedgingStrategyOptions.TResult.cs | 4 ++ src/Polly.Core/Hedging/OnHedgingArguments.cs | 7 +++- ...esilienceStrategyBuilderExtensionsTests.cs | 16 +++++++- .../Hedging/HedgingResilienceStrategyTests.cs | 30 ++++++++++++++ 5 files changed, 84 insertions(+), 13 deletions(-) diff --git a/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs b/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs index 5a941f8edb..88f8d763b0 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 5a5fab3750..093254cb5c 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 199911a93a..b1930cced0 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 the 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 8e6051b090..8c79ccdbdf 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 7f854df8c5..7801b062c4 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() { @@ -814,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;