From 053deaaca4743db67d880a822af0c63b30a951ab Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Tue, 2 Jan 2024 18:49:20 +0100 Subject: [PATCH 1/4] Fix stack trace growing for opened circuit breaker --- .../Controller/CircuitStateController.cs | 18 +++++++------- .../Controller/CircuitStateControllerTests.cs | 24 +++++++++++++++++++ 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs b/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs index a00907ed227..ba5d7ec20be 100644 --- a/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs +++ b/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs @@ -20,7 +20,7 @@ internal sealed class CircuitStateController : IDisposable private DateTimeOffset _blockedUntil; private CircuitState _circuitState = CircuitState.Closed; private Outcome? _lastOutcome; - private BrokenCircuitException _breakingException = new(); + private Exception? _breakingException; private bool _disposed; #pragma warning disable S107 @@ -139,8 +139,8 @@ public ValueTask CloseCircuitAsync(ResilienceContext context) exception = _circuitState switch { - CircuitState.Open => _breakingException, - CircuitState.HalfOpen when !isHalfOpen => _breakingException, + CircuitState.Open => CreateBrokenCircuitException(), + CircuitState.HalfOpen when !isHalfOpen => CreateBrokenCircuitException(), CircuitState.Isolated => new IsolatedCircuitException(), _ => null }; @@ -305,14 +305,16 @@ private void SetLastHandledOutcome_NeedsLock(Outcome outcome) if (outcome.Exception is Exception exception) { - _breakingException = new BrokenCircuitException(BrokenCircuitException.DefaultMessage, exception); - } - else - { - _breakingException = new BrokenCircuitException(BrokenCircuitException.DefaultMessage); + _breakingException = exception; } } + private BrokenCircuitException CreateBrokenCircuitException() => _breakingException switch + { + Exception exception => new BrokenCircuitException(exception.Message, exception), + _ => new BrokenCircuitException(BrokenCircuitException.DefaultMessage) + }; + private void OpenCircuit_NeedsLock(Outcome outcome, bool manual, ResilienceContext context, out Task? scheduledTask) { OpenCircuitFor_NeedsLock(outcome, _breakDuration, manual, context, out scheduledTask); diff --git a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs index f69f4ca4600..84b503ef6d9 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs @@ -124,6 +124,30 @@ public async Task OnActionPreExecute_CircuitOpenedByValue() GetBlockedTill(controller).Should().Be(_timeProvider.GetUtcNow() + _options.BreakDuration); } + [Fact] + public async Task OnActionPreExecute_CircuitOpened_EnsureExceptionStackTraceDoesNotGrow() + { + var stacks = new List(); + var ctxt = ResilienceContextPool.Shared.Get(); + using var controller = CreateController(); + + await OpenCircuit(controller, Outcome.FromResult(99)); + + for (int i = 0; i < 100; i++) + { + try + { + (await controller.OnActionPreExecuteAsync(ctxt)).Value.ThrowIfException(); + } + catch (BrokenCircuitException e) + { + stacks.Add(e.StackTrace); + } + } + + stacks.Distinct().Should().HaveCount(1); + } + [Fact] public async Task HalfOpen_EnsureBreakDuration() { From 26301a68ad75878e0b41d7dff15c936346125bd1 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Tue, 2 Jan 2024 19:02:38 +0100 Subject: [PATCH 2/4] fixes --- .../CircuitBreaker/Controller/CircuitStateControllerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs index 84b503ef6d9..b365179733a 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs @@ -141,7 +141,7 @@ public async Task OnActionPreExecute_CircuitOpened_EnsureExceptionStackTraceDoes } catch (BrokenCircuitException e) { - stacks.Add(e.StackTrace); + stacks.Add(e.StackTrace!); } } From 372cecbc8b7a75418e2494f19131af626c0d9915 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Wed, 3 Jan 2024 09:11:59 +0100 Subject: [PATCH 3/4] PR comments and fixes --- .../Controller/CircuitStateController.cs | 8 ++----- .../Controller/CircuitStateControllerTests.cs | 24 +++++++++++++++---- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs b/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs index ba5d7ec20be..e2333370884 100644 --- a/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs +++ b/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs @@ -302,16 +302,12 @@ private bool PermitHalfOpenCircuitTest_NeedsLock() private void SetLastHandledOutcome_NeedsLock(Outcome outcome) { _lastOutcome = outcome; - - if (outcome.Exception is Exception exception) - { - _breakingException = exception; - } + _breakingException = outcome.Exception; } private BrokenCircuitException CreateBrokenCircuitException() => _breakingException switch { - Exception exception => new BrokenCircuitException(exception.Message, exception), + Exception exception => new BrokenCircuitException(BrokenCircuitException.DefaultMessage, exception), _ => new BrokenCircuitException(BrokenCircuitException.DefaultMessage) }; diff --git a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs index b365179733a..c7b086b0780 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs @@ -124,24 +124,38 @@ public async Task OnActionPreExecute_CircuitOpenedByValue() GetBlockedTill(controller).Should().Be(_timeProvider.GetUtcNow() + _options.BreakDuration); } - [Fact] - public async Task OnActionPreExecute_CircuitOpened_EnsureExceptionStackTraceDoesNotGrow() + [InlineData(true)] + [InlineData(false)] + [Theory] + public async Task OnActionPreExecute_CircuitOpened_EnsureExceptionStackTraceDoesNotGrow(bool innerException) { var stacks = new List(); - var ctxt = ResilienceContextPool.Shared.Get(); + var context = ResilienceContextPool.Shared.Get(); using var controller = CreateController(); - await OpenCircuit(controller, Outcome.FromResult(99)); + await OpenCircuit( + controller, + innerException ? Outcome.FromException(new InvalidOperationException()) : Outcome.FromResult(99)); for (int i = 0; i < 100; i++) { try { - (await controller.OnActionPreExecuteAsync(ctxt)).Value.ThrowIfException(); + (await controller.OnActionPreExecuteAsync(context)).Value.ThrowIfException(); } catch (BrokenCircuitException e) { stacks.Add(e.StackTrace!); + e.Message.Should().Be("The circuit is now open and is not allowing calls."); + + if (innerException) + { + e.InnerException.Should().BeOfType(); + } + else + { + e.InnerException.Should().BeNull(); + } } } From bc433bbd5614682c2c327299336880b5f85517df Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Wed, 3 Jan 2024 09:29:59 +0100 Subject: [PATCH 4/4] fixes --- .../CircuitBreaker/BrokenCircuitExceptionTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/Polly.Core.Tests/CircuitBreaker/BrokenCircuitExceptionTests.cs b/test/Polly.Core.Tests/CircuitBreaker/BrokenCircuitExceptionTests.cs index 3fb989e7a59..7b69c987058 100644 --- a/test/Polly.Core.Tests/CircuitBreaker/BrokenCircuitExceptionTests.cs +++ b/test/Polly.Core.Tests/CircuitBreaker/BrokenCircuitExceptionTests.cs @@ -7,6 +7,7 @@ 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();