diff --git a/src/Polly.Core/Hedging/Controller/HedgingHandler.cs b/src/Polly.Core/Hedging/Controller/HedgingHandler.cs index 4b61b14b909..e5839ae6d95 100644 --- a/src/Polly.Core/Hedging/Controller/HedgingHandler.cs +++ b/src/Polly.Core/Hedging/Controller/HedgingHandler.cs @@ -2,7 +2,8 @@ namespace Polly.Hedging.Utils; internal sealed record class HedgingHandler( Func, ValueTask> ShouldHandle, - Func, Func>>?> ActionGenerator) + Func, Func>>?> ActionGenerator, + Func, ValueTask>? OnHedging) { public Func>>? GenerateAction(HedgingActionGeneratorArguments args) { diff --git a/src/Polly.Core/Hedging/Controller/TaskExecution.cs b/src/Polly.Core/Hedging/Controller/TaskExecution.cs index 568173ea299..c3f8d129f2e 100644 --- a/src/Polly.Core/Hedging/Controller/TaskExecution.cs +++ b/src/Polly.Core/Hedging/Controller/TaskExecution.cs @@ -127,6 +127,8 @@ public async ValueTask InitializeAsync( return true; } + await HandleOnHedgingAsync(snapshot.Context, attemptNumber - 1).ConfigureAwait(Context.ContinueOnCapturedContext); + ExecutionTaskSafe = ExecuteSecondaryActionAsync(action); } else @@ -137,6 +139,21 @@ public async ValueTask InitializeAsync( return true; } + private async Task HandleOnHedgingAsync(ResilienceContext primaryContext, int attemptNumber) + { + var args = new OnHedgingArguments( + primaryContext, + Context, + attemptNumber); + + _telemetry.Report(new(ResilienceEventSeverity.Warning, HedgingConstants.OnHedgingEventName), Context, args); + + if (_handler.OnHedging is { } onHedging) + { + await onHedging(args).ConfigureAwait(Context.ContinueOnCapturedContext); + } + } + private HedgingActionGeneratorArguments CreateArguments( Func>> primaryCallback, ResilienceContext primaryContext, diff --git a/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs b/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs index 8fe5ff18b80..46bf1193696 100644 --- a/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs +++ b/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs @@ -7,19 +7,22 @@ namespace Polly.Hedging; /// /// The type of the result. /// +/// The represents the context that was received by the hedging strategy and used to execute the primary action. +/// To prevent race conditions, the hedging strategy then clones the primary context into and uses it to execute the hedged action. +/// Every hedged action gets its own context that is cloned from the primary. +/// /// Always use the constructor when creating this struct, otherwise we do not guarantee binary compatibility. +/// /// public readonly struct HedgingActionGeneratorArguments { /// /// Initializes a new instance of the struct. /// - /// The primary resilience context. - /// - /// The context that will be passed to action generated by . - /// . + /// The primary context received by the hedging strategy. + /// The action context. cloned from the primary context. /// The zero-based hedging attempt number. - /// The callback passed to hedging strategy. + /// The callback passed to the hedging strategy. public HedgingActionGeneratorArguments( ResilienceContext primaryContext, ResilienceContext actionContext, @@ -33,12 +36,12 @@ public HedgingActionGeneratorArguments( } /// - /// Gets the primary resilience context. + /// Gets the primary resilience context as received by the hedging strategy. /// public ResilienceContext PrimaryContext { get; } /// - /// Gets the context that will be passed to action generated by . + /// Gets the action context that will be used for the hedged action. /// /// /// This context is cloned from . @@ -51,7 +54,7 @@ public HedgingActionGeneratorArguments( public int AttemptNumber { get; } /// - /// Gets the callback passed to hedging strategy. + /// Gets the callback passed to the hedging strategy. /// public Func>> Callback { get; } } diff --git a/src/Polly.Core/Hedging/HedgingResiliencePipelineBuilderExtensions.cs b/src/Polly.Core/Hedging/HedgingResiliencePipelineBuilderExtensions.cs index 1c46dd4463a..ef4e327b0b5 100644 --- a/src/Polly.Core/Hedging/HedgingResiliencePipelineBuilderExtensions.cs +++ b/src/Polly.Core/Hedging/HedgingResiliencePipelineBuilderExtensions.cs @@ -35,13 +35,12 @@ public static class HedgingResiliencePipelineBuilderExtensions private static HedgingResilienceStrategy CreateHedgingStrategy(StrategyBuilderContext context, HedgingStrategyOptions options) { - var handler = new HedgingHandler(options.ShouldHandle!, options.ActionGenerator); + var handler = new HedgingHandler(options.ShouldHandle!, options.ActionGenerator, options.OnHedging); return new HedgingResilienceStrategy( options.Delay, options.MaxHedgedAttempts, handler, - options.OnHedging, options.DelayGenerator, context.TimeProvider, context.Telemetry); diff --git a/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs b/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs index b8c8cf1ea25..097bb6ef578 100644 --- a/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs +++ b/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs @@ -1,4 +1,5 @@ using System.Diagnostics.CodeAnalysis; +using Polly.Hedging.Controller; using Polly.Hedging.Utils; using Polly.Telemetry; @@ -6,15 +7,12 @@ namespace Polly.Hedging; internal sealed class HedgingResilienceStrategy : ResilienceStrategy { - private readonly TimeProvider _timeProvider; - private readonly ResilienceStrategyTelemetry _telemetry; private readonly HedgingController _controller; public HedgingResilienceStrategy( TimeSpan hedgingDelay, int maxHedgedAttempts, HedgingHandler hedgingHandler, - Func, ValueTask>? onHedging, Func>? hedgingDelayGenerator, TimeProvider timeProvider, ResilienceStrategyTelemetry telemetry) @@ -22,11 +20,7 @@ public HedgingResilienceStrategy( HedgingDelay = hedgingDelay; TotalAttempts = maxHedgedAttempts + 1; // include the initial attempt DelayGenerator = hedgingDelayGenerator; - _timeProvider = timeProvider; HedgingHandler = hedgingHandler; - OnHedging = onHedging; - - _telemetry = telemetry; _controller = new HedgingController(telemetry, timeProvider, HedgingHandler, TotalAttempts); } @@ -38,8 +32,6 @@ public HedgingResilienceStrategy( public HedgingHandler HedgingHandler { get; } - public Func, ValueTask>? OnHedging { get; } - [ExcludeFromCodeCoverage] // coverlet issue protected internal override async ValueTask> ExecuteCore( Func>> callback, @@ -71,11 +63,8 @@ private async ValueTask> ExecuteCoreAsync( var cancellationToken = context.CancellationToken; var continueOnCapturedContext = context.ContinueOnCapturedContext; - var attempt = -1; while (true) { - attempt++; - var start = _timeProvider.GetTimestamp(); if (cancellationToken.IsCancellationRequested) { return Outcome.FromException(new OperationCanceledException(cancellationToken).TrySetStackTrace()); @@ -92,10 +81,6 @@ private async ValueTask> ExecuteCoreAsync( var execution = await hedgingContext.TryWaitForCompletedExecutionAsync(delay).ConfigureAwait(continueOnCapturedContext); if (execution is null) { - // 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( - new OnHedgingArguments(context, null, attempt, duration: delay)).ConfigureAwait(context.ContinueOnCapturedContext); continue; } @@ -106,23 +91,6 @@ await HandleOnHedgingAsync( execution.AcceptOutcome(); return outcome; } - - var executionTime = _timeProvider.GetElapsedTime(start); - await HandleOnHedgingAsync( - new OnHedgingArguments(context, outcome, attempt, executionTime)).ConfigureAwait(context.ContinueOnCapturedContext); - } - } - - private async ValueTask HandleOnHedgingAsync(OnHedgingArguments args) - { - _telemetry.Report, T>(new(ResilienceEventSeverity.Warning, HedgingConstants.OnHedgingEventName), args.Context, default, args); - - 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(args).ConfigureAwait(args.Context.ContinueOnCapturedContext); } } diff --git a/src/Polly.Core/Hedging/OnHedgingArguments.cs b/src/Polly.Core/Hedging/OnHedgingArguments.cs index 3ac88c1867a..b5fffb13adc 100644 --- a/src/Polly.Core/Hedging/OnHedgingArguments.cs +++ b/src/Polly.Core/Hedging/OnHedgingArguments.cs @@ -7,43 +7,43 @@ namespace Polly.Hedging; /// /// The type of result. /// +/// The represents the context that was received by the hedging strategy and used to execute the primary action. +/// To prevent race conditions, the hedging strategy then clones the primary context into and uses it to execute the hedged action. +/// Every hedged action gets its own context that is cloned from the primary. +/// /// Always use the constructor when creating this struct, otherwise we do not guarantee binary compatibility. +/// /// public readonly struct OnHedgingArguments { /// /// Initializes a new instance of the struct. /// - /// The context in which the resilience operation or event occurred. - /// The outcome of the resilience operation or event. + /// The primary context received by the hedging strategy. + /// The action context. cloned from the primary context. /// The zero-based hedging attempt number. - /// The execution duration of hedging attempt or the hedging delay in case the attempt was not finished in time. - public OnHedgingArguments(ResilienceContext context, Outcome? outcome, int attemptNumber, TimeSpan duration) + public OnHedgingArguments(ResilienceContext primaryContext, ResilienceContext actionContext, int attemptNumber) { - Context = context; - Outcome = outcome; + PrimaryContext = primaryContext; + ActionContext = actionContext; AttemptNumber = attemptNumber; - Duration = duration; } /// - /// Gets the outcome that needs to be hedged, if any. + /// Gets the primary resilience context as received by the hedging strategy. /// - /// If this property is , it's an indication that user-callback or hedged operation did not complete within the hedging delay. - public Outcome? Outcome { get; } + public ResilienceContext PrimaryContext { get; } /// - /// Gets the context of this event. + /// Gets the action context that will be used for the hedged action. /// - public ResilienceContext Context { get; } + /// + /// This context is cloned from . + /// + public ResilienceContext ActionContext { get; } /// /// Gets the zero-based hedging attempt number. /// public int AttemptNumber { get; } - - /// - /// Gets the execution duration of hedging attempt or the hedging delay in case the attempt was not finished in time. - /// - public TimeSpan Duration { get; } } diff --git a/src/Polly.Core/PublicAPI.Unshipped.txt b/src/Polly.Core/PublicAPI.Unshipped.txt index 96906d8113d..8ea9b8c5d6c 100644 --- a/src/Polly.Core/PublicAPI.Unshipped.txt +++ b/src/Polly.Core/PublicAPI.Unshipped.txt @@ -143,12 +143,11 @@ Polly.Hedging.HedgingStrategyOptions.OnHedging.set -> void Polly.Hedging.HedgingStrategyOptions.ShouldHandle.get -> System.Func, System.Threading.Tasks.ValueTask>! Polly.Hedging.HedgingStrategyOptions.ShouldHandle.set -> void Polly.Hedging.OnHedgingArguments +Polly.Hedging.OnHedgingArguments.ActionContext.get -> Polly.ResilienceContext! Polly.Hedging.OnHedgingArguments.AttemptNumber.get -> int -Polly.Hedging.OnHedgingArguments.Context.get -> Polly.ResilienceContext! -Polly.Hedging.OnHedgingArguments.Duration.get -> System.TimeSpan Polly.Hedging.OnHedgingArguments.OnHedgingArguments() -> void -Polly.Hedging.OnHedgingArguments.OnHedgingArguments(Polly.ResilienceContext! context, Polly.Outcome? outcome, int attemptNumber, System.TimeSpan duration) -> void -Polly.Hedging.OnHedgingArguments.Outcome.get -> Polly.Outcome? +Polly.Hedging.OnHedgingArguments.OnHedgingArguments(Polly.ResilienceContext! primaryContext, Polly.ResilienceContext! actionContext, int attemptNumber) -> void +Polly.Hedging.OnHedgingArguments.PrimaryContext.get -> Polly.ResilienceContext! Polly.HedgingResiliencePipelineBuilderExtensions Polly.LegacySupport Polly.Outcome diff --git a/test/Polly.Core.Tests/Hedging/HedgingHandlerTests.cs b/test/Polly.Core.Tests/Hedging/HedgingHandlerTests.cs index 2f988c279b1..5955c8058db 100644 --- a/test/Polly.Core.Tests/Hedging/HedgingHandlerTests.cs +++ b/test/Polly.Core.Tests/Hedging/HedgingHandlerTests.cs @@ -10,7 +10,10 @@ public async Task GenerateAction_Generic_Ok() { var handler = new HedgingHandler( args => PredicateResult.True(), - args => () => Outcome.FromResultAsValueTask("ok")); + args => () => Outcome.FromResultAsValueTask("ok"), + args => default); + + handler.OnHedging.Should().NotBeNull(); var action = handler.GenerateAction(new HedgingActionGeneratorArguments( ResilienceContextPool.Shared.Get(), diff --git a/test/Polly.Core.Tests/Hedging/HedgingHelper.cs b/test/Polly.Core.Tests/Hedging/HedgingHelper.cs index 995977544b5..d985d837972 100644 --- a/test/Polly.Core.Tests/Hedging/HedgingHelper.cs +++ b/test/Polly.Core.Tests/Hedging/HedgingHelper.cs @@ -7,9 +7,10 @@ internal static class HedgingHelper { public static HedgingHandler CreateHandler( Func, bool> shouldHandle, - Func, Func>>?> generator) + Func, Func>>?> generator, + Func, ValueTask>? onHedging = null) { - return new HedgingHandler(args => new ValueTask(shouldHandle(args.Outcome!))!, generator); + return new HedgingHandler(args => new ValueTask(shouldHandle(args.Outcome!))!, generator, onHedging); } } diff --git a/test/Polly.Core.Tests/Hedging/HedgingResiliencePipelineBuilderExtensionsTests.cs b/test/Polly.Core.Tests/Hedging/HedgingResiliencePipelineBuilderExtensionsTests.cs index 13e877146f2..c446eabf917 100644 --- a/test/Polly.Core.Tests/Hedging/HedgingResiliencePipelineBuilderExtensionsTests.cs +++ b/test/Polly.Core.Tests/Hedging/HedgingResiliencePipelineBuilderExtensionsTests.cs @@ -1,5 +1,4 @@ using System.ComponentModel.DataAnnotations; -using System.Globalization; using Polly.Hedging; using Polly.Testing; @@ -35,8 +34,7 @@ public void AddHedgingT_InvalidOptions_Throws() [Fact] public async Task AddHedging_IntegrationTest() { - var hedgingWithoutOutcome = false; - ConcurrentQueue results = new(); + int hedgingCount = 0; var strategy = _builder .AddHedging(new() @@ -64,15 +62,7 @@ public async Task AddHedging_IntegrationTest() }, OnHedging = args => { - if (args.Outcome is { } outcome) - { - results.Enqueue(outcome.Result!.ToString(CultureInfo.InvariantCulture)!); - } - else - { - hedgingWithoutOutcome = true; - } - + hedgingCount++; return default; } }) @@ -85,8 +75,6 @@ public async Task AddHedging_IntegrationTest() }); result.Should().Be("success"); - results.Should().HaveCountGreaterThan(0); - results.Distinct().Should().ContainSingle("error"); - hedgingWithoutOutcome.Should().BeTrue(); + hedgingCount.Should().Be(4); } } diff --git a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs index 82e757c0738..a26247366b7 100644 --- a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs +++ b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs @@ -21,7 +21,6 @@ public class HedgingResilienceStrategyTests : IDisposable private readonly HedgingTimeProvider _timeProvider; private readonly HedgingActions _actions; private readonly PrimaryStringTasks _primaryTasks; - private readonly List _results = new(); private readonly CancellationTokenSource _cts = new(); private readonly ITestOutputHelper _testOutput; private HedgingHandler? _handler; @@ -170,21 +169,20 @@ public async Task ExecuteAsync_EnsurePrimaryContextFlows() _options.MaxHedgedAttempts = 3; _options.OnHedging = args => { - args.Context.Should().Be(primaryContext); - - if (args.AttemptNumber == 0) - { - args.Context.Properties.Set(key, "dummy"); - } - + args.ActionContext.Should().NotBe(args.PrimaryContext); + args.PrimaryContext.Should().Be(primaryContext); + args.PrimaryContext.Properties.GetValue(key, string.Empty).Should().Be("dummy"); attempts++; - return default; }; ConfigureHedging(args => { - args.PrimaryContext.Properties.GetValue(key, string.Empty).Should().Be("dummy"); + if (args.AttemptNumber == 1) + { + args.PrimaryContext.Properties.Set(key, "dummy"); + } + args.PrimaryContext.Should().Be(primaryContext); return () => Outcome.FromResultAsValueTask(Failure); }); @@ -192,7 +190,7 @@ public async Task ExecuteAsync_EnsurePrimaryContextFlows() var strategy = Create(); var result = await strategy.ExecuteAsync(_ => new ValueTask(Failure), primaryContext); - attempts.Should().Be(4); + attempts.Should().Be(3); primaryContext.Properties.GetValue(key, string.Empty).Should().Be("dummy"); } @@ -286,35 +284,6 @@ await TestUtilities.AssertWithTimeoutAsync(() => await task; } - [Fact] - public async Task ExecuteAsync_EnsureSecondaryHedgedTaskReportedWithNoOutcome() - { - // arrange - using var cancelled = new ManualResetEvent(false); - var hasOutcome = true; - _options.OnHedging = args => - { - hasOutcome = args.Outcome is not null; - return default; - }; - - ConfigureHedging(context => Outcome.FromResultAsValueTask(Success)); - - 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() { @@ -330,7 +299,7 @@ public async Task ExecuteAsync_EnsureDiscardedResultDisposed() }; }); - var pipeline = Create(handler, null).AsPipeline(); + var pipeline = Create(handler).AsPipeline(); // act var resultTask = pipeline.ExecuteAsync(async token => @@ -549,8 +518,8 @@ public async Task ExecuteAsync_Secondary_CustomPropertiesAvailable() public async Task ExecuteAsync_OnHedgingEventThrows_EnsureExceptionRethrown() { // arrange - ConfigureHedging(args => () => Outcome.FromResultAsValueTask(Success)); _options.OnHedging = _ => throw new InvalidOperationException("my-exception"); + ConfigureHedging(args => () => Outcome.FromResultAsValueTask(Success)); var strategy = Create(); // act @@ -883,21 +852,33 @@ public async Task ExecuteAsync_EnsureExceptionStackTracePreserved() [Fact] public async Task ExecuteAsync_EnsureOnHedgingCalled() { + var primaryContext = ResilienceContextPool.Shared.Get(); + var key = new ResiliencePropertyKey("my-key"); + primaryContext.Properties.Set(key, "my-value"); + var attempts = new List(); _options.OnHedging = args => { - args.Outcome.Should().NotBeNull(); - args.Outcome!.Value.Result.Should().Be(Failure); + args.PrimaryContext.Should().Be(primaryContext); + args.ActionContext.Should().NotBe(args.PrimaryContext); + args.PrimaryContext.Properties.GetValue(key, string.Empty).Should().Be("my-value"); + args.ActionContext.Properties.GetValue(key, string.Empty).Should().Be("my-value"); + args.ActionContext.Properties.Set(key, "new-value"); attempts.Add(args.AttemptNumber); return default; }; - ConfigureHedging(res => res.Result == Failure, args => () => Outcome.FromResultAsValueTask(Failure)); + ConfigureHedging(res => res.Result == Failure, + args => () => + { + args.ActionContext.Properties.GetValue(key, string.Empty).Should().Be("new-value"); + return Outcome.FromResultAsValueTask(Failure); + }); var strategy = Create(); - await strategy.ExecuteAsync(_ => new ValueTask(Failure)); + await strategy.ExecuteAsync(_ => new ValueTask(Failure), primaryContext); - attempts.Should().HaveCount(_options.MaxHedgedAttempts + 1); + attempts.Should().HaveCount(_options.MaxHedgedAttempts); attempts.Should().BeInAscendingOrder(); attempts[0].Should().Be(0); } @@ -912,25 +893,12 @@ public async Task ExecuteAsync_EnsureOnHedgingTelemetry() var strategy = Create(); await strategy.ExecuteAsync((_, _) => new ValueTask(Failure), context, "state"); - _events.Should().HaveCount(_options.MaxHedgedAttempts + 5); + _events.Should().HaveCount(_options.MaxHedgedAttempts + 4); _events.Select(v => v.Event.EventName).Distinct().Should().HaveCount(2); } private void ConfigureHedging() { - _options.OnHedging = args => - { - lock (_results) - { - if (args.Outcome.HasValue) - { - _results.Add(args.Outcome.Value.Result!); - } - } - - return default; - }; - ConfigureHedging(_ => false, _actions.Generator); } @@ -948,7 +916,7 @@ private void ConfigureHedging( Func, bool> shouldHandle, Func, Func>>?> generator) { - _handler = HedgingHelper.CreateHandler(shouldHandle, generator); + _handler = HedgingHelper.CreateHandler(shouldHandle, generator, _options.OnHedging); } private void ConfigureHedging(TimeSpan delay) => ConfigureHedging(args => async () => @@ -957,15 +925,12 @@ private void ConfigureHedging(TimeSpan delay) => ConfigureHedging(args => async return Outcome.FromResult("secondary"); }); - private ResiliencePipeline Create() => Create(_handler!, _options.OnHedging).AsPipeline(); + private ResiliencePipeline Create() => Create(_handler!).AsPipeline(); - private HedgingResilienceStrategy Create( - HedgingHandler handler, - Func, ValueTask>? onHedging) => new( + private HedgingResilienceStrategy Create(HedgingHandler handler) => new( _options.Delay, _options.MaxHedgedAttempts, handler, - onHedging, _options.DelayGenerator, _timeProvider, _telemetry); diff --git a/test/Polly.Core.Tests/Hedging/OnHedgingArgumentsTests.cs b/test/Polly.Core.Tests/Hedging/OnHedgingArgumentsTests.cs index 8429277c7ea..75e44c5967b 100644 --- a/test/Polly.Core.Tests/Hedging/OnHedgingArgumentsTests.cs +++ b/test/Polly.Core.Tests/Hedging/OnHedgingArgumentsTests.cs @@ -7,11 +7,10 @@ public class OnHedgingArgumentsTests [Fact] public void Ctor_Ok() { - var args = new OnHedgingArguments(ResilienceContextPool.Shared.Get(), Outcome.FromResult(1), 1, TimeSpan.FromSeconds(1)); + var args = new OnHedgingArguments(ResilienceContextPool.Shared.Get(), ResilienceContextPool.Shared.Get(), 1); - args.Context.Should().NotBeNull(); - args.Outcome!.Value.Result.Should().Be(1); + args.PrimaryContext.Should().NotBeNull(); + args.ActionContext.Should().NotBeNull(); args.AttemptNumber.Should().Be(1); - args.Duration.Should().Be(TimeSpan.FromSeconds(1)); } }