Skip to content

Fix issue #1979: an unhandled exception in half open state must transition to closed and not prevent leaving half open state forever #1991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
martincostello merged 3 commits into from
Feb 24, 2024
Merged

Fix issue #1979: an unhandled exception in half open state must transition to closed and not prevent leaving half open state forever #1991

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51961ac
rename methods to clarify intention
Feb 22, 2024
bc2f3d4
ensure that an unhandled exception in half open state closes the circ…
Feb 22, 2024
99d26b0
fix mutation survived in StrategyHelper; add explicit test to replace…
Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
rename methods to clarify intention
  • Loading branch information
Dominic Ullmann committed Feb 22, 2024
commit 51961ace14b25e4e0395d9ea8c8e4932b3eddf93
4 changes: 2 additions & 2 deletions src/Polly.Core/CircuitBreaker/CircuitBreakerResilienceStrategy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ protected internal override async ValueTask<Outcome<T>> ExecuteCore<TState>(Func
var args = new CircuitBreakerPredicateArguments<T>(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)
{
await _controller.OnActionSuccessAsync(outcome, context).ConfigureAwait(context.ContinueOnCapturedContext);
await _controller.OnUnhandledOutcomeAsync(outcome, context).ConfigureAwait(context.ContinueOnCapturedContext);
}

return outcome;
Expand Down
4 changes: 2 additions & 2 deletions src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public ValueTask CloseCircuitAsync(ResilienceContext context)
return null;
}

public ValueTask OnActionSuccessAsync(Outcome<T> outcome, ResilienceContext context)
public ValueTask OnUnhandledOutcomeAsync(Outcome<T> outcome, ResilienceContext context)
{
EnsureNotDisposed();

Expand All @@ -189,7 +189,7 @@ public ValueTask OnActionSuccessAsync(Outcome<T> outcome, ResilienceContext cont
return ExecuteScheduledTaskAsync(task, context);
}

public ValueTask OnActionFailureAsync(Outcome<T> outcome, ResilienceContext context)
public ValueTask OnHandledOutcomeAsync(Outcome<T> outcome, ResilienceContext context)
{
EnsureNotDisposed();

Expand Down
30 changes: 15 additions & 15 deletions test/Polly.Core.Tests/CircuitBreaker/Controller/CircuitStateControllerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ public async Task Disposed_EnsureThrows()
await Assert.ThrowsAsync<ObjectDisposedException>(async () => await controller.CloseCircuitAsync(ResilienceContextPool.Shared.Get()));
await Assert.ThrowsAsync<ObjectDisposedException>(async () => await controller.IsolateCircuitAsync(ResilienceContextPool.Shared.Get()));
await Assert.ThrowsAsync<ObjectDisposedException>(async () => await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get()));
await Assert.ThrowsAsync<ObjectDisposedException>(async () => await controller.OnActionSuccessAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get()));
await Assert.ThrowsAsync<ObjectDisposedException>(async () => await controller.OnActionFailureAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get()));
await Assert.ThrowsAsync<ObjectDisposedException>(async () => await controller.OnUnhandledOutcomeAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get()));
await Assert.ThrowsAsync<ObjectDisposedException>(async () => await controller.OnHandledOutcomeAsync(Outcome.FromResult(10), ResilienceContextPool.Shared.Get()));
}

[Fact]
Expand Down Expand Up @@ -182,15 +182,15 @@ 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);
_circuitBehavior.Received().OnCircuitClosed();
}
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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -491,7 +491,7 @@ public async Task Flow_Closed_HalfOpen_Open_HalfOpen_Closed()
_circuitBehavior.When(v => v.OnActionFailure(CircuitState.HalfOpen, out Arg.Any<bool>()))
.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
Expand All @@ -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);
Expand Down Expand Up @@ -562,7 +562,7 @@ private async Task OpenCircuit(CircuitStateController<int> controller, Outcome<i
_circuitBehavior.When(v => v.OnActionFailure(CircuitState.Closed, out Arg.Any<bool>()))
.Do(x => x[1] = breakCircuit);

await controller.OnActionFailureAsync(outcome ?? Outcome.FromResult(10), ResilienceContextPool.Shared.Get().Initialize<int>(true));
await controller.OnHandledOutcomeAsync(outcome ?? Outcome.FromResult(10), ResilienceContextPool.Shared.Get().Initialize<int>(true));
}

private void AdvanceTime(TimeSpan timespan) => _timeProvider.Advance(timespan);
Expand Down