From e07cee52e6851c446e21d2e725ee48a4ede76988 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Mon, 25 Sep 2023 16:48:48 +0200 Subject: [PATCH 1/7] Fix the behavior of `OnHedging` event --- .../Hedging/Controller/HedgingHandler.cs | 3 +- .../Hedging/Controller/TaskExecution.cs | 17 ++++ ...gingResiliencePipelineBuilderExtensions.cs | 3 +- .../Hedging/HedgingResilienceStrategy.cs | 33 +------- src/Polly.Core/Hedging/OnHedgingArguments.cs | 29 +++---- src/Polly.Core/PublicAPI.Unshipped.txt | 7 +- .../Hedging/HedgingHandlerTests.cs | 5 +- .../Polly.Core.Tests/Hedging/HedgingHelper.cs | 5 +- ...esiliencePipelineBuilderExtensionsTests.cs | 18 +---- .../Hedging/HedgingResilienceStrategyTests.cs | 81 ++++--------------- .../Hedging/OnHedgingArgumentsTests.cs | 7 +- 11 files changed, 66 insertions(+), 142 deletions(-) diff --git a/src/Polly.Core/Hedging/Controller/HedgingHandler.cs b/src/Polly.Core/Hedging/Controller/HedgingHandler.cs index 4b61b14b90..e5839ae6d9 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 568173ea29..bb9987db29 100644 --- a/src/Polly.Core/Hedging/Controller/TaskExecution.cs +++ b/src/Polly.Core/Hedging/Controller/TaskExecution.cs @@ -127,6 +127,8 @@ public void Cancel() return true; } + await HandleOnHedgingAsync(snapshot.Context, attemptNumber - 1).ConfigureAwait(Context.ContinueOnCapturedContext); + ExecutionTaskSafe = ExecuteSecondaryActionAsync(action); } else @@ -137,6 +139,21 @@ public void Cancel() 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 not null) + { + await _handler.OnHedging(args).ConfigureAwait(Context.ContinueOnCapturedContext); + } + } + private HedgingActionGeneratorArguments CreateArguments( Func>> primaryCallback, ResilienceContext primaryContext, diff --git a/src/Polly.Core/Hedging/HedgingResiliencePipelineBuilderExtensions.cs b/src/Polly.Core/Hedging/HedgingResiliencePipelineBuilderExtensions.cs index 1c46dd4463..ef4e327b0b 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 b8c8cf1ea2..cb8510a378 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 @@ internal sealed class HedgingResilienceStrategy : ResilienceStrategy 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 @@ internal sealed class HedgingResilienceStrategy : ResilienceStrategy public HedgingHandler HedgingHandler { get; } - public Func, ValueTask>? OnHedging { get; } - [ExcludeFromCodeCoverage] // coverlet issue protected internal override async ValueTask> ExecuteCore( Func>> callback, @@ -72,10 +64,10 @@ internal sealed class HedgingResilienceStrategy : ResilienceStrategy 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 +84,6 @@ internal sealed class HedgingResilienceStrategy : ResilienceStrategy 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 +94,6 @@ internal sealed class HedgingResilienceStrategy : ResilienceStrategy 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 3ac88c1867..34eb9f629c 100644 --- a/src/Polly.Core/Hedging/OnHedgingArguments.cs +++ b/src/Polly.Core/Hedging/OnHedgingArguments.cs @@ -14,36 +14,31 @@ namespace Polly.Hedging; /// /// 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 outcome of the resilience operation or event. + /// The action 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. /// - /// 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 96906d8113..8ea9b8c5d6 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 2f988c279b..5955c8058d 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 995977544b..d985d83797 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 13e877146f..c446eabf91 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 82e757c073..12751a2d6a 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 @@ 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.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 @@ -886,8 +855,6 @@ public async Task ExecuteAsync_EnsureOnHedgingCalled() var attempts = new List(); _options.OnHedging = args => { - args.Outcome.Should().NotBeNull(); - args.Outcome!.Value.Result.Should().Be(Failure); attempts.Add(args.AttemptNumber); return default; }; @@ -897,7 +864,7 @@ public async Task ExecuteAsync_EnsureOnHedgingCalled() var strategy = Create(); await strategy.ExecuteAsync(_ => new ValueTask(Failure)); - attempts.Should().HaveCount(_options.MaxHedgedAttempts + 1); + attempts.Should().HaveCount(_options.MaxHedgedAttempts); attempts.Should().BeInAscendingOrder(); attempts[0].Should().Be(0); } @@ -912,25 +879,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 +902,7 @@ private void ConfigureHedging(Func, Func 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 +911,12 @@ private void ConfigureHedging(Func, Func 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 8429277c7e..75e44c5967 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)); } } From 482042f365028fa2ae22186a88c1191dc99802c8 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Mon, 25 Sep 2023 18:49:04 +0200 Subject: [PATCH 2/7] cleanup --- src/Polly.Core/Hedging/HedgingResilienceStrategy.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs b/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs index cb8510a378..097bb6ef57 100644 --- a/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs +++ b/src/Polly.Core/Hedging/HedgingResilienceStrategy.cs @@ -63,11 +63,8 @@ internal sealed class HedgingResilienceStrategy : ResilienceStrategy var cancellationToken = context.CancellationToken; var continueOnCapturedContext = context.ContinueOnCapturedContext; - var attempt = -1; - while (true) { - attempt++; if (cancellationToken.IsCancellationRequested) { return Outcome.FromException(new OperationCanceledException(cancellationToken).TrySetStackTrace()); From 150bdacad3ce7ec742a4083fbcdd8afa1cc0d610 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Tue, 26 Sep 2023 07:39:45 +0200 Subject: [PATCH 3/7] improve test --- .../Hedging/HedgingResilienceStrategyTests.cs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs index 12751a2d6a..2c789d9acf 100644 --- a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs +++ b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs @@ -1,3 +1,4 @@ +using NSubstitute; using Polly.Hedging; using Polly.Hedging.Utils; using Polly.Telemetry; @@ -852,17 +853,31 @@ 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.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); attempts.Should().BeInAscendingOrder(); From 0f1a366fdd071253eadbb80ceb4d37fc50eb211e Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Tue, 26 Sep 2023 07:48:13 +0200 Subject: [PATCH 4/7] fixes --- test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs index 2c789d9acf..a26247366b 100644 --- a/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs +++ b/test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs @@ -1,4 +1,3 @@ -using NSubstitute; using Polly.Hedging; using Polly.Hedging.Utils; using Polly.Telemetry; From fe6377b5d5a37b00a55d1733055b914556d90824 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Tue, 26 Sep 2023 09:26:32 +0200 Subject: [PATCH 5/7] PR comments --- .../Hedging/Controller/TaskExecution.cs | 4 ++-- .../Hedging/HedgingActionGeneratorArguments.cs | 15 +++++++++------ src/Polly.Core/Hedging/OnHedgingArguments.cs | 11 ++++++++--- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/Polly.Core/Hedging/Controller/TaskExecution.cs b/src/Polly.Core/Hedging/Controller/TaskExecution.cs index bb9987db29..c3f8d129f2 100644 --- a/src/Polly.Core/Hedging/Controller/TaskExecution.cs +++ b/src/Polly.Core/Hedging/Controller/TaskExecution.cs @@ -148,9 +148,9 @@ private async Task HandleOnHedgingAsync(ResilienceContext primaryContext, int at _telemetry.Report(new(ResilienceEventSeverity.Warning, HedgingConstants.OnHedgingEventName), Context, args); - if (_handler.OnHedging is not null) + if (_handler.OnHedging is { } onHedging) { - await _handler.OnHedging(args).ConfigureAwait(Context.ContinueOnCapturedContext); + await onHedging(args).ConfigureAwait(Context.ContinueOnCapturedContext); } } diff --git a/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs b/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs index 8fe5ff18b8..85f6de80f2 100644 --- a/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs +++ b/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs @@ -7,17 +7,20 @@ 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, 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 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 hedging strategy. + /// The action context. cloned from the primary context. /// The zero-based hedging attempt number. /// The callback passed to hedging strategy. public HedgingActionGeneratorArguments( @@ -33,12 +36,12 @@ namespace Polly.Hedging; } /// - /// Gets the primary resilience context. + /// Gets the primary resilience context as received by 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 . diff --git a/src/Polly.Core/Hedging/OnHedgingArguments.cs b/src/Polly.Core/Hedging/OnHedgingArguments.cs index 34eb9f629c..dff2c4cf3d 100644 --- a/src/Polly.Core/Hedging/OnHedgingArguments.cs +++ b/src/Polly.Core/Hedging/OnHedgingArguments.cs @@ -14,9 +14,14 @@ namespace Polly.Hedging; /// /// Initializes a new instance of the struct. /// - /// The outcome of the resilience operation or event. - /// The action context. + /// The primary context received by hedging strategy. + /// The action context. cloned from the primary context. /// The zero-based hedging attempt number. + /// + /// The represents the context that was received by the hedging strategy and used to execute the primary action. + /// To prevent race conditions, 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 primary. + /// public OnHedgingArguments(ResilienceContext primaryContext, ResilienceContext actionContext, int attemptNumber) { PrimaryContext = primaryContext; @@ -25,7 +30,7 @@ public OnHedgingArguments(ResilienceContext primaryContext, ResilienceContext ac } /// - /// Gets the primary resilience context. + /// Gets the primary resilience context as received by hedging strategy. /// public ResilienceContext PrimaryContext { get; } From 85fb4d763331e8274886c36addbd9b217b929fcc Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Tue, 26 Sep 2023 10:24:18 +0200 Subject: [PATCH 6/7] PR comments --- .../Hedging/HedgingActionGeneratorArguments.cs | 10 +++++----- src/Polly.Core/Hedging/OnHedgingArguments.cs | 14 +++++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs b/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs index 85f6de80f2..843ab2d141 100644 --- a/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs +++ b/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs @@ -8,8 +8,8 @@ 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, 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 primary. +/// 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. /// @@ -22,7 +22,7 @@ namespace Polly.Hedging; /// The primary context received by 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, @@ -36,7 +36,7 @@ namespace Polly.Hedging; } /// - /// Gets the primary resilience context as received by hedging strategy. + /// Gets the primary resilience context as received by the hedging strategy. /// public ResilienceContext PrimaryContext { get; } @@ -54,7 +54,7 @@ namespace Polly.Hedging; 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/OnHedgingArguments.cs b/src/Polly.Core/Hedging/OnHedgingArguments.cs index dff2c4cf3d..b5fffb13ad 100644 --- a/src/Polly.Core/Hedging/OnHedgingArguments.cs +++ b/src/Polly.Core/Hedging/OnHedgingArguments.cs @@ -7,21 +7,21 @@ 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 primary context received by hedging strategy. + /// The primary context received by the hedging strategy. /// The action context. cloned from the primary context. /// The zero-based hedging attempt number. - /// - /// The represents the context that was received by the hedging strategy and used to execute the primary action. - /// To prevent race conditions, 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 primary. - /// public OnHedgingArguments(ResilienceContext primaryContext, ResilienceContext actionContext, int attemptNumber) { PrimaryContext = primaryContext; @@ -30,7 +30,7 @@ public OnHedgingArguments(ResilienceContext primaryContext, ResilienceContext ac } /// - /// Gets the primary resilience context as received by hedging strategy. + /// Gets the primary resilience context as received by the hedging strategy. /// public ResilienceContext PrimaryContext { get; } From 9b2c8c4ce859e413f00efb72ea2a5bb4b8f4b0b3 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Tue, 26 Sep 2023 10:37:48 +0200 Subject: [PATCH 7/7] PR comments --- src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs b/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs index 843ab2d141..46bf119369 100644 --- a/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs +++ b/src/Polly.Core/Hedging/HedgingActionGeneratorArguments.cs @@ -19,7 +19,7 @@ namespace Polly.Hedging; /// /// Initializes a new instance of the struct. /// - /// The primary context received by hedging strategy. + /// 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 the hedging strategy.