From de6eb20fd8b808a3f821fc2ab1eef2eeb0b99fa7 Mon Sep 17 00:00:00 2001 From: Dominic Ullmann <35130113+DominicUllmann@users.noreply.github.com> Date: Sat, 24 Feb 2024 09:59:51 +0100 Subject: [PATCH] Fix CircuitBreaker handling of Half-Open (#1991) * Rename methods to clarify intention * Ensure that an unhandled exception in half open state closes the circuit instead of blocking any further transition from half open state * Fix mutation survived in StrategyHelper; add explicit test to replace no longer existing implicit test in CircuitBreakerResilienceStrategyTests --- .../CircuitBreakerResilienceStrategy.cs | 6 ++-- .../Controller/CircuitStateController.cs | 4 +-- .../CircuitBreakerResilienceStrategyTests.cs | 6 ++-- .../Controller/CircuitStateControllerTests.cs | 30 +++++++++---------- .../Utils/StrategyHelperTests.cs | 23 ++++++++++++++ 5 files changed, 45 insertions(+), 24 deletions(-) diff --git a/src/Polly.Core/CircuitBreaker/CircuitBreakerResilienceStrategy.cs b/src/Polly.Core/CircuitBreaker/CircuitBreakerResilienceStrategy.cs index d509f99fd8..e6d8bd1927 100644 --- a/src/Polly.Core/CircuitBreaker/CircuitBreakerResilienceStrategy.cs +++ b/src/Polly.Core/CircuitBreaker/CircuitBreakerResilienceStrategy.cs @@ -39,11 +39,11 @@ protected internal override async ValueTask> ExecuteCore(Func var args = new CircuitBreakerPredicateArguments(context, outcome); if (await _handler(args).ConfigureAwait(context.ContinueOnCapturedContext)) { - await _controller.OnActionFailureAsync(outcome, context).ConfigureAwait(context.ContinueOnCapturedContext); + await _controller.OnHandledOutcomeAsync(outcome, context).ConfigureAwait(context.ContinueOnCapturedContext); } - else if (outcome.Exception is null) + else { - await _controller.OnActionSuccessAsync(outcome, context).ConfigureAwait(context.ContinueOnCapturedContext); + await _controller.OnUnhandledOutcomeAsync(outcome, context).ConfigureAwait(context.ContinueOnCapturedContext); } return outcome; diff --git a/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs b/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs index 9cddd1b524..70645725cd 100644 --- a/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs +++ b/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs @@ -163,7 +163,7 @@ public ValueTask CloseCircuitAsync(ResilienceContext context) return null; } - public ValueTask OnActionSuccessAsync(Outcome outcome, ResilienceContext context) + public ValueTask OnUnhandledOutcomeAsync(Outcome outcome, ResilienceContext context) { EnsureNotDisposed(); @@ -189,7 +189,7 @@ public ValueTask OnActionSuccessAsync(Outcome outcome, ResilienceContext cont return ExecuteScheduledTaskAsync(task, context); } - public ValueTask OnActionFailureAsync(Outcome outcome, ResilienceContext context) + public ValueTask OnHandledOutcomeAsync(Outcome outcome, ResilienceContext context) { EnsureNotDisposed(); diff --git a/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResilienceStrategyTests.cs b/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResilienceStrategyTests.cs index 4dbdb62a20..500b830aee 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResilienceStrategyTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResilienceStrategyTests.cs @@ -105,16 +105,14 @@ public void Execute_HandledException_OnFailureCalled() } [Fact] - public void Execute_UnhandledException_NoCalls() + public void Execute_UnhandledException_OnActionSuccess() { _options.ShouldHandle = args => new ValueTask(args.Outcome.Exception is InvalidOperationException); var strategy = Create(); strategy.Invoking(s => s.Execute(_ => throw new ArgumentException())).Should().Throw(); - _behavior.DidNotReceiveWithAnyArgs().OnActionFailure(default, out Arg.Any()); - _behavior.DidNotReceiveWithAnyArgs().OnActionSuccess(default); - _behavior.DidNotReceiveWithAnyArgs().OnCircuitClosed(); + _behavior.Received(1).OnActionSuccess(CircuitState.Closed); } public void Dispose() => _controller.Dispose(); diff --git a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs index 6ce574759e..dc993b44f3 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs @@ -108,8 +108,8 @@ public async Task Disposed_EnsureThrows() await Assert.ThrowsAsync(async () => await controller.CloseCircuitAsync(ResilienceContextPool.Shared.Get())); await Assert.ThrowsAsync(async () => await controller.IsolateCircuitAsync(ResilienceContextPool.Shared.Get())); await Assert.ThrowsAsync(async () => await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get())); - await Assert.ThrowsAsync(async () => await controller.OnActionSuccessAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get())); - await Assert.ThrowsAsync(async () => await controller.OnActionFailureAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get())); + await Assert.ThrowsAsync(async () => await controller.OnUnhandledOutcomeAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get())); + await Assert.ThrowsAsync(async () => await controller.OnHandledOutcomeAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get())); } [Fact] @@ -182,7 +182,7 @@ public async Task HalfOpen_EnsureCorrectStateTransitionAfterExecution(bool succe if (success) { - await controller.OnActionSuccessAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get()); + await controller.OnUnhandledOutcomeAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get()); controller.CircuitState.Should().Be(CircuitState.Closed); _circuitBehavior.Received().OnActionSuccess(CircuitState.HalfOpen); @@ -190,7 +190,7 @@ public async Task HalfOpen_EnsureCorrectStateTransitionAfterExecution(bool succe } else { - await controller.OnActionFailureAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get()); + await controller.OnHandledOutcomeAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get()); controller.CircuitState.Should().Be(CircuitState.Open); _circuitBehavior.DidNotReceiveWithAnyArgs().OnActionSuccess(default); @@ -226,9 +226,9 @@ public async Task OnActionFailure_EnsureLock() using var controller = CreateController(); // act - var executeAction = Task.Run(() => controller.OnActionFailureAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get())); + var executeAction = Task.Run(() => controller.OnHandledOutcomeAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get())); executing.WaitOne(); - var executeAction2 = Task.Run(() => controller.OnActionFailureAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get())); + var executeAction2 = Task.Run(() => controller.OnHandledOutcomeAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get())); // assert #pragma warning disable xUnit1031 // Do not use blocking task operations in test method @@ -285,7 +285,7 @@ public async Task OnActionSuccess_EnsureCorrectBehavior(CircuitState state, Circ await TransitionToState(controller, state); // act - await controller.OnActionSuccessAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get()); + await controller.OnUnhandledOutcomeAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get()); // assert controller.CircuitState.Should().Be(expectedState); @@ -330,7 +330,7 @@ public async Task OnActionFailureAsync_EnsureCorrectBehavior(CircuitState state, .Do(x => x[1] = shouldBreak); // act - await controller.OnActionFailureAsync(Outcome.FromResult(99), ResilienceContextPool.Shared.Get()); + await controller.OnHandledOutcomeAsync(Outcome.FromResult(99), ResilienceContextPool.Shared.Get()); // assert controller.LastHandledOutcome!.Value.Result.Should().Be(99); @@ -367,7 +367,7 @@ public async Task OnActionFailureAsync_EnsureBreakDurationGeneration() .Do(x => x[1] = true); // act - await controller.OnActionFailureAsync(Outcome.FromResult(99), ResilienceContextPool.Shared.Get()); + await controller.OnHandledOutcomeAsync(Outcome.FromResult(99), ResilienceContextPool.Shared.Get()); // assert var blockedTill = GetBlockedTill(controller); @@ -430,7 +430,7 @@ public async Task OnActionFailureAsync_EnsureBreakDurationNotOverflow(bool overf .Do(x => x[1] = shouldBreak); // act - await controller.OnActionFailureAsync(Outcome.FromResult(99), ResilienceContextPool.Shared.Get()); + await controller.OnHandledOutcomeAsync(Outcome.FromResult(99), ResilienceContextPool.Shared.Get()); // assert var blockedTill = GetBlockedTill(controller); @@ -457,7 +457,7 @@ public async Task OnActionFailureAsync_VoidResult_EnsureBreakingExceptionNotSet( .Do(x => x[1] = shouldBreak); // act - await controller.OnActionFailureAsync(Outcome.FromResult(99), ResilienceContextPool.Shared.Get()); + await controller.OnHandledOutcomeAsync(Outcome.FromResult(99), ResilienceContextPool.Shared.Get()); // assert controller.LastException.Should().BeNull(); @@ -472,7 +472,7 @@ public async Task Flow_Closed_HalfOpen_Closed() await TransitionToState(controller, CircuitState.HalfOpen); - await controller.OnActionSuccessAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get()); + await controller.OnUnhandledOutcomeAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get()); controller.CircuitState.Should().Be(CircuitState.Closed); _circuitBehavior.Received().OnActionSuccess(CircuitState.HalfOpen); @@ -491,7 +491,7 @@ public async Task Flow_Closed_HalfOpen_Open_HalfOpen_Closed() _circuitBehavior.When(v => v.OnActionFailure(CircuitState.HalfOpen, out Arg.Any())) .Do(x => x[1] = shouldBreak); - await controller.OnActionFailureAsync(Outcome.FromResult(0), context); + await controller.OnHandledOutcomeAsync(Outcome.FromResult(0), context); controller.CircuitState.Should().Be(CircuitState.Open); // execution rejected @@ -505,7 +505,7 @@ public async Task Flow_Closed_HalfOpen_Open_HalfOpen_Closed() controller.CircuitState.Should().Be(CircuitState.HalfOpen); // close circuit - await controller.OnActionSuccessAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get()); + await controller.OnUnhandledOutcomeAsync(Outcome.FromResult(0), ResilienceContextPool.Shared.Get()); controller.CircuitState.Should().Be(CircuitState.Closed); _circuitBehavior.Received().OnActionSuccess(CircuitState.HalfOpen); @@ -562,7 +562,7 @@ private async Task OpenCircuit(CircuitStateController controller, Outcome v.OnActionFailure(CircuitState.Closed, out Arg.Any())) .Do(x => x[1] = breakCircuit); - await controller.OnActionFailureAsync(outcome ?? Outcome.FromResult(10), ResilienceContextPool.Shared.Get().Initialize(true)); + await controller.OnHandledOutcomeAsync(outcome ?? Outcome.FromResult(10), ResilienceContextPool.Shared.Get().Initialize(true)); } private void AdvanceTime(TimeSpan timespan) => _timeProvider.Advance(timespan); diff --git a/test/Polly.Core.Tests/Utils/StrategyHelperTests.cs b/test/Polly.Core.Tests/Utils/StrategyHelperTests.cs index afe03cc55e..d08aa6ee77 100644 --- a/test/Polly.Core.Tests/Utils/StrategyHelperTests.cs +++ b/test/Polly.Core.Tests/Utils/StrategyHelperTests.cs @@ -39,4 +39,27 @@ await TestUtilities.AssertWithTimeoutAsync(async () => outcome.Exception.Should().BeOfType(); }); + + [InlineData(true)] + [InlineData(false)] + [Theory] + public async Task ExecuteCallbackSafeAsync_AsyncCallback_CompletedOk(bool isAsync) => + await TestUtilities.AssertWithTimeoutAsync(async () => + { + var outcomeTask = StrategyHelper.ExecuteCallbackSafeAsync( + async (_, _) => + { + if (isAsync) + { + await Task.Delay(15); + } + + return Outcome.FromResult("success"); + }, + ResilienceContextPool.Shared.Get(), + "dummy"); + + outcomeTask.IsCompleted.Should().Be(!isAsync); + (await outcomeTask).Result.Should().Be("success"); + }); }