From 29f6e842a6c35a7f5c9a8402d19962115da49298 Mon Sep 17 00:00:00 2001 From: David Lee Date: Fri, 4 Oct 2024 01:53:14 +0800 Subject: [PATCH] Add RetryAfter to BrokenCircuitException. --- docs/strategies/circuit-breaker.md | 2 +- .../CircuitBreaker/BrokenCircuitException.cs | 34 +++++++++++++++ .../Controller/CircuitStateController.cs | 12 ++++-- src/Polly.Core/PublicAPI.Unshipped.txt | 4 ++ .../BrokenCircuitExceptionTests.cs | 25 +++++++++-- ...itBreakerResiliencePipelineBuilderTests.cs | 42 ++++++++++++++----- .../CircuitBreakerResilienceStrategyTests.cs | 2 +- .../Controller/CircuitStateControllerTests.cs | 17 +++++--- .../IsolatedCircuitExceptionTests.cs | 14 +++++-- 9 files changed, 122 insertions(+), 30 deletions(-) diff --git a/docs/strategies/circuit-breaker.md b/docs/strategies/circuit-breaker.md index 4a2bef29d6d..1ceeeac3833 100644 --- a/docs/strategies/circuit-breaker.md +++ b/docs/strategies/circuit-breaker.md @@ -14,7 +14,7 @@ --- -The circuit breaker **reactive** resilience strategy shortcuts the execution if the underlying resource is detected as unhealthy. The detection process is done via sampling. If the sampled executions' failure-success ratio exceeds a predefined threshold then a circuit breaker will prevent any new executions by throwing a `BrokenCircuitException`. After a preset duration the circuit breaker performs a probe, because the assumption is that this period was enough for the resource to self-heal. Depending on the outcome of the probe, the circuit will either allow new executions or continue to block them. +The circuit breaker **reactive** resilience strategy shortcuts the execution if the underlying resource is detected as unhealthy. The detection process is done via sampling. If the sampled executions' failure-success ratio exceeds a predefined threshold then a circuit breaker will prevent any new executions by throwing a `BrokenCircuitException`. After a preset duration the circuit breaker performs a probe, because the assumption is that this period was enough for the resource to self-heal. Depending on the outcome of the probe, the circuit will either allow new executions or continue to block them. If an execution is blocked by the circuit breaker, the thrown exception may indicate the amount of time executions will continue to be blocked through its `RetryAfter` property. > [!NOTE] > Be aware that the Circuit Breaker strategy [rethrows all exceptions](https://github.com/App-vNext/Polly/wiki/Circuit-Breaker#exception-handling), including those that are handled. A Circuit Breaker's role is to monitor faults and break the circuit when a certain threshold is reached; it does not manage retries. Combine the Circuit Breaker with a Retry strategy if needed. diff --git a/src/Polly.Core/CircuitBreaker/BrokenCircuitException.cs b/src/Polly.Core/CircuitBreaker/BrokenCircuitException.cs index d240e64702a..744bf1d31a8 100644 --- a/src/Polly.Core/CircuitBreaker/BrokenCircuitException.cs +++ b/src/Polly.Core/CircuitBreaker/BrokenCircuitException.cs @@ -22,6 +22,14 @@ public BrokenCircuitException() { } + /// + /// Initializes a new instance of the class. + /// + /// The period after which the circuit will close. + public BrokenCircuitException(TimeSpan retryAfter) + : base($"The circuit is now open and is not allowing calls. It can be retried after '{retryAfter}'.") + => RetryAfter = retryAfter; + /// /// Initializes a new instance of the class. /// @@ -31,6 +39,14 @@ public BrokenCircuitException(string message) { } + /// + /// Initializes a new instance of the class. + /// + /// The message that describes the error. + /// The period after which the circuit will close. + public BrokenCircuitException(string message, TimeSpan retryAfter) + : base(message) => RetryAfter = retryAfter; + /// /// Initializes a new instance of the class. /// @@ -41,6 +57,15 @@ public BrokenCircuitException(string message, Exception inner) { } + /// + /// Initializes a new instance of the class. + /// + /// The message that describes the error. + /// The period after which the circuit will close. + /// The inner exception. + public BrokenCircuitException(string message, TimeSpan retryAfter, Exception inner) + : base(message, inner) => RetryAfter = retryAfter; + #pragma warning disable RS0016 // Add public types and members to the declared API #if !NETCOREAPP /// @@ -54,4 +79,13 @@ protected BrokenCircuitException(SerializationInfo info, StreamingContext contex } #endif #pragma warning restore RS0016 // Add public types and members to the declared API + + /// + /// Gets the amount of time before the circuit can become closed, if known. + /// + /// + /// This value is specified when the instance is constructed and may be inaccurate if consumed at a later time. + /// Can be if not provided or if the circuit was manually isolated. + /// + public TimeSpan? RetryAfter { get; } } diff --git a/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs b/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs index 40dd2c5e35a..9076965334d 100644 --- a/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs +++ b/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs @@ -305,11 +305,15 @@ private void SetLastHandledOutcome_NeedsLock(Outcome outcome) _breakingException = outcome.Exception; } - private BrokenCircuitException CreateBrokenCircuitException() => _breakingException switch + private BrokenCircuitException CreateBrokenCircuitException() { - Exception exception => new BrokenCircuitException(BrokenCircuitException.DefaultMessage, exception), - _ => new BrokenCircuitException(BrokenCircuitException.DefaultMessage) - }; + TimeSpan retryAfter = _blockedUntil - _timeProvider.GetUtcNow(); + return _breakingException switch + { + Exception exception => new BrokenCircuitException(BrokenCircuitException.DefaultMessage, retryAfter, exception), + _ => new BrokenCircuitException(BrokenCircuitException.DefaultMessage, retryAfter) + }; + } private void OpenCircuit_NeedsLock(Outcome outcome, bool manual, ResilienceContext context, out Task? scheduledTask) => OpenCircuitFor_NeedsLock(outcome, _breakDuration, manual, context, out scheduledTask); diff --git a/src/Polly.Core/PublicAPI.Unshipped.txt b/src/Polly.Core/PublicAPI.Unshipped.txt index ab058de62d4..f82785466ab 100644 --- a/src/Polly.Core/PublicAPI.Unshipped.txt +++ b/src/Polly.Core/PublicAPI.Unshipped.txt @@ -1 +1,5 @@ #nullable enable +Polly.CircuitBreaker.BrokenCircuitException.BrokenCircuitException(string! message, System.TimeSpan retryAfter) -> void +Polly.CircuitBreaker.BrokenCircuitException.BrokenCircuitException(string! message, System.TimeSpan retryAfter, System.Exception! inner) -> void +Polly.CircuitBreaker.BrokenCircuitException.BrokenCircuitException(System.TimeSpan retryAfter) -> void +Polly.CircuitBreaker.BrokenCircuitException.RetryAfter.get -> System.TimeSpan? diff --git a/test/Polly.Core.Tests/CircuitBreaker/BrokenCircuitExceptionTests.cs b/test/Polly.Core.Tests/CircuitBreaker/BrokenCircuitExceptionTests.cs index ded9279f4d1..dc2211fa63e 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/BrokenCircuitExceptionTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/BrokenCircuitExceptionTests.cs @@ -7,10 +7,27 @@ public class BrokenCircuitExceptionTests [Fact] public void Ctor_Ok() { - new BrokenCircuitException().Message.Should().Be("The circuit is now open and is not allowing calls."); - new BrokenCircuitException("Dummy.").Message.Should().Be("Dummy."); - new BrokenCircuitException("Dummy.", new InvalidOperationException()).Message.Should().Be("Dummy."); - new BrokenCircuitException("Dummy.", new InvalidOperationException()).InnerException.Should().BeOfType(); + var retryAfter = new TimeSpan(1, 0, 0); + var defaultException = new BrokenCircuitException(); + defaultException.Message.Should().Be("The circuit is now open and is not allowing calls."); + defaultException.RetryAfter.Should().BeNull(); + var retryAfterException = new BrokenCircuitException(retryAfter); + retryAfterException.Message.Should().Be($"The circuit is now open and is not allowing calls. It can be retried after '{retryAfter}'."); + retryAfterException.RetryAfter.Should().Be(retryAfter); + var dummyMessageException = new BrokenCircuitException("Dummy."); + dummyMessageException.Message.Should().Be("Dummy."); + dummyMessageException.RetryAfter.Should().BeNull(); + var dummyMessageWithRetryAfterException = new BrokenCircuitException("Dummy.", retryAfter); + dummyMessageWithRetryAfterException.Message.Should().Be("Dummy."); + dummyMessageWithRetryAfterException.RetryAfter.Should().Be(retryAfter); + var dummyMessageExceptionWithInnerException = new BrokenCircuitException("Dummy.", new InvalidOperationException()); + dummyMessageExceptionWithInnerException.Message.Should().Be("Dummy."); + dummyMessageExceptionWithInnerException.InnerException.Should().BeOfType(); + dummyMessageExceptionWithInnerException.RetryAfter.Should().BeNull(); + var dummyMessageExceptionWithInnerExceptionAndRetryAfter = new BrokenCircuitException("Dummy.", retryAfter, new InvalidOperationException()); + dummyMessageExceptionWithInnerExceptionAndRetryAfter.Message.Should().Be("Dummy."); + dummyMessageExceptionWithInnerExceptionAndRetryAfter.InnerException.Should().BeOfType(); + dummyMessageExceptionWithInnerExceptionAndRetryAfter.RetryAfter.Should().Be(retryAfter); } #if !NETCOREAPP diff --git a/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResiliencePipelineBuilderTests.cs b/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResiliencePipelineBuilderTests.cs index 5cee0687360..c5c1c809a83 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResiliencePipelineBuilderTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResiliencePipelineBuilderTests.cs @@ -68,7 +68,8 @@ public void AddCircuitBreaker_IntegrationTest() int closed = 0; int halfOpened = 0; - var breakDuration = TimeSpan.FromSeconds(1); + var halfBreakDuration = TimeSpan.FromMilliseconds(500); + var breakDuration = halfBreakDuration + halfBreakDuration; var options = new CircuitBreakerStrategyOptions { @@ -94,15 +95,25 @@ public void AddCircuitBreaker_IntegrationTest() opened.Should().Be(1); halfOpened.Should().Be(0); closed.Should().Be(0); - Assert.Throws(() => strategy.Execute(_ => 0)); + BrokenCircuitException exception = Assert.Throws(() => strategy.Execute(_ => 0)); + exception.RetryAfter.Should().Be(breakDuration); + + // Circuit still open after some time + timeProvider.Advance(halfBreakDuration); + opened.Should().Be(1); + halfOpened.Should().Be(0); + closed.Should().Be(0); + exception = Assert.Throws(() => strategy.Execute(_ => 0)); + exception.RetryAfter.Should().Be(halfBreakDuration); // Circuit Half Opened - timeProvider.Advance(breakDuration); + timeProvider.Advance(halfBreakDuration); strategy.Execute(_ => -1); - Assert.Throws(() => strategy.Execute(_ => 0)); + exception = Assert.Throws(() => strategy.Execute(_ => 0)); opened.Should().Be(2); halfOpened.Should().Be(1); closed.Should().Be(0); + exception.RetryAfter.Should().Be(breakDuration); // Now close it timeProvider.Advance(breakDuration); @@ -119,7 +130,8 @@ public void AddCircuitBreaker_IntegrationTest_WithBreakDurationGenerator() int closed = 0; int halfOpened = 0; - var breakDuration = TimeSpan.FromSeconds(1); + var halfBreakDuration = TimeSpan.FromMilliseconds(500); + var breakDuration = halfBreakDuration + halfBreakDuration; var options = new CircuitBreakerStrategyOptions { @@ -146,15 +158,25 @@ public void AddCircuitBreaker_IntegrationTest_WithBreakDurationGenerator() opened.Should().Be(1); halfOpened.Should().Be(0); closed.Should().Be(0); - Assert.Throws(() => strategy.Execute(_ => 0)); + BrokenCircuitException exception = Assert.Throws(() => strategy.Execute(_ => 0)); + exception.RetryAfter.Should().Be(breakDuration); + + // Circuit still open after some time + timeProvider.Advance(halfBreakDuration); + opened.Should().Be(1); + halfOpened.Should().Be(0); + closed.Should().Be(0); + exception = Assert.Throws(() => strategy.Execute(_ => 0)); + exception.RetryAfter.Should().Be(halfBreakDuration); // Circuit Half Opened - timeProvider.Advance(breakDuration); + timeProvider.Advance(halfBreakDuration); strategy.Execute(_ => -1); - Assert.Throws(() => strategy.Execute(_ => 0)); + exception = Assert.Throws(() => strategy.Execute(_ => 0)); opened.Should().Be(2); halfOpened.Should().Be(1); closed.Should().Be(0); + exception.RetryAfter.Should().Be(breakDuration); // Now close it timeProvider.Advance(breakDuration); @@ -178,8 +200,8 @@ public async Task AddCircuitBreakers_WithIsolatedManualControl_ShouldBeIsolated( .AddCircuitBreaker(new() { ManualControl = manualControl }) .Build(); - strategy1.Invoking(s => s.Execute(() => { })).Should().Throw(); - strategy2.Invoking(s => s.Execute(() => { })).Should().Throw(); + strategy1.Invoking(s => s.Execute(() => { })).Should().Throw().Where(e => e.RetryAfter == null); + strategy2.Invoking(s => s.Execute(() => { })).Should().Throw().Where(e => e.RetryAfter == null); await manualControl.CloseAsync(); diff --git a/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResilienceStrategyTests.cs b/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResilienceStrategyTests.cs index 500b830aee4..7a0748683d5 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResilienceStrategyTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResilienceStrategyTests.cs @@ -53,7 +53,7 @@ public async Task Ctor_ManualControl_EnsureAttached() var strategy = Create(); await _options.ManualControl.IsolateAsync(CancellationToken.None); - strategy.Invoking(s => s.Execute(_ => 0)).Should().Throw(); + strategy.Invoking(s => s.Execute(_ => 0)).Should().Throw().Where(e => e.RetryAfter == null); await _options.ManualControl.CloseAsync(CancellationToken.None); diff --git a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs index dc993b44f32..7a623544629 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs @@ -52,7 +52,8 @@ public async Task IsolateAsync_Ok() called.Should().BeTrue(); var outcome = await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get()); - outcome.Value.Exception.Should().BeOfType(); + outcome.Value.Exception.Should().BeOfType() + .And.Subject.As().RetryAfter.Should().BeNull(); // now close it await controller.CloseCircuitAsync(ResilienceContextPool.Shared.Get()); @@ -119,7 +120,7 @@ public async Task OnActionPreExecute_CircuitOpenedByValue() await OpenCircuit(controller, Outcome.FromResult(99)); var error = (BrokenCircuitException)(await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get())).Value.Exception!; - error.Should().BeOfType(); + error.Should().BeOfType().And.Subject.As().RetryAfter.Should().NotBeNull(); GetBlockedTill(controller).Should().Be(_timeProvider.GetUtcNow() + _options.BreakDuration); } @@ -147,6 +148,7 @@ await OpenCircuit( { stacks.Add(e.StackTrace!); e.Message.Should().Be("The circuit is now open and is not allowing calls."); + e.RetryAfter.Should().NotBeNull(); if (innerException) { @@ -206,6 +208,7 @@ public async Task OnActionPreExecute_CircuitOpenedByException() await OpenCircuit(controller, Outcome.FromException(new InvalidOperationException())); var error = (BrokenCircuitException)(await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get())).Value.Exception!; error.InnerException.Should().BeOfType(); + error.RetryAfter.Should().NotBeNull(); } [Fact] @@ -258,7 +261,7 @@ public async Task OnActionPreExecute_HalfOpen() // act await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get()); var error = (await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get())).Value.Exception; - error.Should().BeOfType(); + error.Should().BeOfType().And.Subject.As().RetryAfter.Should().NotBeNull(); // assert controller.CircuitState.Should().Be(CircuitState.HalfOpen); @@ -462,7 +465,7 @@ public async Task OnActionFailureAsync_VoidResult_EnsureBreakingExceptionNotSet( // assert controller.LastException.Should().BeNull(); var outcome = await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get()); - outcome.Value.Exception.Should().BeOfType(); + outcome.Value.Exception.Should().BeOfType().And.Subject.As().RetryAfter.Should().NotBeNull(); } [Fact] @@ -495,9 +498,11 @@ public async Task Flow_Closed_HalfOpen_Open_HalfOpen_Closed() controller.CircuitState.Should().Be(CircuitState.Open); // execution rejected - AdvanceTime(TimeSpan.FromMilliseconds(1)); + TimeSpan advanceTimeRejected = TimeSpan.FromMilliseconds(1); + AdvanceTime(advanceTimeRejected); var outcome = await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get()); - outcome.Value.Exception.Should().BeOfType(); + outcome.Value.Exception.Should().BeOfType() + .And.Subject.As().RetryAfter.Should().Be(_options.BreakDuration - advanceTimeRejected); // wait and try, transition to half open AdvanceTime(_options.BreakDuration + _options.BreakDuration); diff --git a/test/Polly.Core.Tests/CircuitBreaker/IsolatedCircuitExceptionTests.cs b/test/Polly.Core.Tests/CircuitBreaker/IsolatedCircuitExceptionTests.cs index 5e381a06e3b..6698f8148cc 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/IsolatedCircuitExceptionTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/IsolatedCircuitExceptionTests.cs @@ -7,10 +7,16 @@ public class IsolatedCircuitExceptionTests [Fact] public void Ctor_Ok() { - new IsolatedCircuitException("Dummy.").Message.Should().Be("Dummy."); - new IsolatedCircuitException().Message.Should().Be("The circuit is manually held open and is not allowing calls."); - new IsolatedCircuitException("Dummy.", new InvalidOperationException()).Message.Should().Be("Dummy."); - new IsolatedCircuitException("Dummy.", new InvalidOperationException()).InnerException.Should().BeOfType(); + var dummyMessageException = new IsolatedCircuitException("Dummy."); + dummyMessageException.Message.Should().Be("Dummy."); + dummyMessageException.RetryAfter.Should().BeNull(); + var defaultException = new IsolatedCircuitException(); + defaultException.Message.Should().Be("The circuit is manually held open and is not allowing calls."); + defaultException.RetryAfter.Should().BeNull(); + var dummyMessageExceptionWithInnerException = new IsolatedCircuitException("Dummy.", new InvalidOperationException()); + dummyMessageExceptionWithInnerException.Message.Should().Be("Dummy."); + dummyMessageExceptionWithInnerException.InnerException.Should().BeOfType(); + dummyMessageExceptionWithInnerException.RetryAfter.Should().BeNull(); } #if !NETCOREAPP