From 0f38c51f65823fe3cb0fd7db4d7c5da723aca7fb Mon Sep 17 00:00:00 2001 From: Lehonti Ramos <17771375+Lehonti@users.noreply.github.com> Date: Fri, 4 Aug 2023 09:00:02 +0200 Subject: [PATCH] Refactor code to reduce nesting (#1453) Reduced nesting levels through block-scoped `using`s and the inversion of an `if` block. Co-authored-by: Lehonti --- .../AdvancedCircuitController.cs | 97 +++++++++---------- .../CircuitBreaker/CircuitStateController.cs | 47 ++++----- .../ConsecutiveCountCircuitController.cs | 77 +++++++-------- src/Polly/Timeout/AsyncTimeoutEngine.cs | 57 ++++++----- src/Polly/Timeout/TimeoutEngine.cs | 75 +++++++------- 5 files changed, 166 insertions(+), 187 deletions(-) diff --git a/src/Polly/CircuitBreaker/AdvancedCircuitController.cs b/src/Polly/CircuitBreaker/AdvancedCircuitController.cs index ae09698baf8..6c959aac478 100644 --- a/src/Polly/CircuitBreaker/AdvancedCircuitController.cs +++ b/src/Polly/CircuitBreaker/AdvancedCircuitController.cs @@ -29,73 +29,70 @@ Action onHalfOpen public override void OnCircuitReset(Context context) { - using (TimedLock.Lock(_lock)) - { - // Is only null during initialization of the current class - // as the variable is not set, before the base class calls - // current method from constructor. - _metrics?.Reset_NeedsLock(); + using var _ = TimedLock.Lock(_lock); - ResetInternal_NeedsLock(context); - } + // Is only null during initialization of the current class + // as the variable is not set, before the base class calls + // current method from constructor. + _metrics?.Reset_NeedsLock(); + + ResetInternal_NeedsLock(context); } public override void OnActionSuccess(Context context) { - using (TimedLock.Lock(_lock)) - { - switch (_circuitState) - { - case CircuitState.HalfOpen: - OnCircuitReset(context); - break; + using var _ = TimedLock.Lock(_lock); - case CircuitState.Closed: - break; + switch (_circuitState) + { + case CircuitState.HalfOpen: + OnCircuitReset(context); + break; - case CircuitState.Open: - case CircuitState.Isolated: - break; // A successful call result may arrive when the circuit is open, if it was placed before the circuit broke. We take no special action; only time passing governs transitioning from Open to HalfOpen state. + case CircuitState.Closed: + break; - default: - throw new InvalidOperationException("Unhandled CircuitState."); - } + case CircuitState.Open: + case CircuitState.Isolated: + break; // A successful call result may arrive when the circuit is open, if it was placed before the circuit broke. We take no special action; only time passing governs transitioning from Open to HalfOpen state. - _metrics.IncrementSuccess_NeedsLock(); + default: + throw new InvalidOperationException("Unhandled CircuitState."); } + + _metrics.IncrementSuccess_NeedsLock(); } public override void OnActionFailure(DelegateResult outcome, Context context) { - using (TimedLock.Lock(_lock)) + using var _ = TimedLock.Lock(_lock); + + _lastOutcome = outcome; + + switch (_circuitState) { - _lastOutcome = outcome; + case CircuitState.HalfOpen: + Break_NeedsLock(context); + return; + + case CircuitState.Closed: + _metrics.IncrementFailure_NeedsLock(); + var healthCount = _metrics.GetHealthCount_NeedsLock(); - switch (_circuitState) - { - case CircuitState.HalfOpen: + int throughput = healthCount.Total; + if (throughput >= _minimumThroughput && (double)healthCount.Failures / throughput >= _failureThreshold) + { Break_NeedsLock(context); - return; - - case CircuitState.Closed: - _metrics.IncrementFailure_NeedsLock(); - var healthCount = _metrics.GetHealthCount_NeedsLock(); - - int throughput = healthCount.Total; - if (throughput >= _minimumThroughput && (double)healthCount.Failures / throughput >= _failureThreshold) - { - Break_NeedsLock(context); - } - break; - - case CircuitState.Open: - case CircuitState.Isolated: - _metrics.IncrementFailure_NeedsLock(); - break; // A failure call result may arrive when the circuit is open, if it was placed before the circuit broke. We take no action beyond tracking the metric; we do not want to duplicate-signal onBreak; we do not want to extend time for which the circuit is broken. We do not want to mask the fact that the call executed (as replacing its result with a Broken/IsolatedCircuitException would do). - - default: - throw new InvalidOperationException("Unhandled CircuitState."); - } + } + break; + + case CircuitState.Open: + case CircuitState.Isolated: + _metrics.IncrementFailure_NeedsLock(); + break; // A failure call result may arrive when the circuit is open, if it was placed before the circuit broke. We take no action beyond tracking the metric; we do not want to duplicate-signal onBreak; we do not want to extend time for which the circuit is broken. We do not want to mask the fact that the call executed (as replacing its result with a Broken/IsolatedCircuitException would do). + + default: + throw new InvalidOperationException("Unhandled CircuitState."); } } } diff --git a/src/Polly/CircuitBreaker/CircuitStateController.cs b/src/Polly/CircuitBreaker/CircuitStateController.cs index f46cbda5081..c83403d6c10 100644 --- a/src/Polly/CircuitBreaker/CircuitStateController.cs +++ b/src/Polly/CircuitBreaker/CircuitStateController.cs @@ -36,15 +36,15 @@ public CircuitState CircuitState return _circuitState; } - using (TimedLock.Lock(_lock)) + using var _ = TimedLock.Lock(_lock); + + if (_circuitState == CircuitState.Open && !IsInAutomatedBreak_NeedsLock) { - if (_circuitState == CircuitState.Open && !IsInAutomatedBreak_NeedsLock) - { - _circuitState = CircuitState.HalfOpen; - _onHalfOpen(); - } - return _circuitState; + _circuitState = CircuitState.HalfOpen; + _onHalfOpen(); } + + return _circuitState; } } @@ -52,10 +52,8 @@ public Exception LastException { get { - using (TimedLock.Lock(_lock)) - { - return _lastOutcome?.Exception; - } + using var _ = TimedLock.Lock(_lock); + return _lastOutcome?.Exception; } } @@ -63,11 +61,8 @@ public TResult LastHandledResult { get { - using (TimedLock.Lock(_lock)) - { - return _lastOutcome != null - ? _lastOutcome.Result : default; - } + using var _ = TimedLock.Lock(_lock); + return _lastOutcome != null ? _lastOutcome.Result : default; } } @@ -75,12 +70,10 @@ public TResult LastHandledResult public void Isolate() { - using (TimedLock.Lock(_lock)) - { - _lastOutcome = new DelegateResult(new IsolatedCircuitException("The circuit is manually held open and is not allowing calls.")); - BreakFor_NeedsLock(TimeSpan.MaxValue, Context.None()); - _circuitState = CircuitState.Isolated; - } + using var _ = TimedLock.Lock(_lock); + _lastOutcome = new DelegateResult(new IsolatedCircuitException("The circuit is manually held open and is not allowing calls.")); + BreakFor_NeedsLock(TimeSpan.MaxValue, Context.None()); + _circuitState = CircuitState.Isolated; } protected void Break_NeedsLock(Context context) => @@ -118,13 +111,13 @@ protected void ResetInternal_NeedsLock(Context context) protected bool PermitHalfOpenCircuitTest() { long currentlyBlockedUntil = _blockedTill; - if (SystemClock.UtcNow().Ticks >= currentlyBlockedUntil) + if (SystemClock.UtcNow().Ticks < currentlyBlockedUntil) { - // It's time to permit a / another trial call in the half-open state ... - // ... but to prevent race conditions/multiple calls, we have to ensure only _one_ thread wins the race to own this next call. - return Interlocked.CompareExchange(ref _blockedTill, SystemClock.UtcNow().Ticks + _durationOfBreak.Ticks, currentlyBlockedUntil) == currentlyBlockedUntil; + return false; } - return false; + // It's time to permit a / another trial call in the half-open state ... + // ... but to prevent race conditions/multiple calls, we have to ensure only _one_ thread wins the race to own this next call. + return Interlocked.CompareExchange(ref _blockedTill, SystemClock.UtcNow().Ticks + _durationOfBreak.Ticks, currentlyBlockedUntil) == currentlyBlockedUntil; } private BrokenCircuitException GetBreakingException() diff --git a/src/Polly/CircuitBreaker/ConsecutiveCountCircuitController.cs b/src/Polly/CircuitBreaker/ConsecutiveCountCircuitController.cs index a102e3e7e48..d4214391997 100644 --- a/src/Polly/CircuitBreaker/ConsecutiveCountCircuitController.cs +++ b/src/Polly/CircuitBreaker/ConsecutiveCountCircuitController.cs @@ -16,65 +16,60 @@ Action onHalfOpen public override void OnCircuitReset(Context context) { - using (TimedLock.Lock(_lock)) - { - _consecutiveFailureCount = 0; - - ResetInternal_NeedsLock(context); - } + using var _ = TimedLock.Lock(_lock); + _consecutiveFailureCount = 0; + ResetInternal_NeedsLock(context); } public override void OnActionSuccess(Context context) { - using (TimedLock.Lock(_lock)) + using var _ = TimedLock.Lock(_lock); + + switch (_circuitState) { - switch (_circuitState) - { - case CircuitState.HalfOpen: - OnCircuitReset(context); - break; + case CircuitState.HalfOpen: + OnCircuitReset(context); + break; - case CircuitState.Closed: - _consecutiveFailureCount = 0; - break; + case CircuitState.Closed: + _consecutiveFailureCount = 0; + break; - case CircuitState.Open: - case CircuitState.Isolated: - break; // A successful call result may arrive when the circuit is open, if it was placed before the circuit broke. We take no action; only time passing governs transitioning from Open to HalfOpen state. + case CircuitState.Open: + case CircuitState.Isolated: + break; // A successful call result may arrive when the circuit is open, if it was placed before the circuit broke. We take no action; only time passing governs transitioning from Open to HalfOpen state. - default: - throw new InvalidOperationException("Unhandled CircuitState."); - } + default: + throw new InvalidOperationException("Unhandled CircuitState."); } } public override void OnActionFailure(DelegateResult outcome, Context context) { - using (TimedLock.Lock(_lock)) + using var _ = TimedLock.Lock(_lock); + + _lastOutcome = outcome; + + switch (_circuitState) { - _lastOutcome = outcome; + case CircuitState.HalfOpen: + Break_NeedsLock(context); + return; - switch (_circuitState) - { - case CircuitState.HalfOpen: + case CircuitState.Closed: + _consecutiveFailureCount += 1; + if (_consecutiveFailureCount >= _exceptionsAllowedBeforeBreaking) + { Break_NeedsLock(context); - return; - - case CircuitState.Closed: - _consecutiveFailureCount += 1; - if (_consecutiveFailureCount >= _exceptionsAllowedBeforeBreaking) - { - Break_NeedsLock(context); - } - break; + } + break; - case CircuitState.Open: - case CircuitState.Isolated: - break; // A failure call result may arrive when the circuit is open, if it was placed before the circuit broke. We take no action; we do not want to duplicate-signal onBreak; we do not want to extend time for which the circuit is broken. We do not want to mask the fact that the call executed (as replacing its result with a Broken/IsolatedCircuitException would do). + case CircuitState.Open: + case CircuitState.Isolated: + break; // A failure call result may arrive when the circuit is open, if it was placed before the circuit broke. We take no action; we do not want to duplicate-signal onBreak; we do not want to extend time for which the circuit is broken. We do not want to mask the fact that the call executed (as replacing its result with a Broken/IsolatedCircuitException would do). - default: - throw new InvalidOperationException("Unhandled CircuitState."); - } + default: + throw new InvalidOperationException("Unhandled CircuitState."); } } } diff --git a/src/Polly/Timeout/AsyncTimeoutEngine.cs b/src/Polly/Timeout/AsyncTimeoutEngine.cs index da85c341d2c..c161463497b 100644 --- a/src/Polly/Timeout/AsyncTimeoutEngine.cs +++ b/src/Polly/Timeout/AsyncTimeoutEngine.cs @@ -14,45 +14,42 @@ internal static async Task ImplementationAsync( cancellationToken.ThrowIfCancellationRequested(); TimeSpan timeout = timeoutProvider(context); - using (CancellationTokenSource timeoutCancellationTokenSource = new CancellationTokenSource()) - { - using (CancellationTokenSource combinedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token)) - { - Task actionTask = null; - CancellationToken combinedToken = combinedTokenSource.Token; + using var timeoutCancellationTokenSource = new CancellationTokenSource(); + using var combinedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token); - try - { - if (timeoutStrategy == TimeoutStrategy.Optimistic) - { - SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); - return await action(context, combinedToken).ConfigureAwait(continueOnCapturedContext); - } + Task actionTask = null; + CancellationToken combinedToken = combinedTokenSource.Token; - // else: timeoutStrategy == TimeoutStrategy.Pessimistic + try + { + if (timeoutStrategy == TimeoutStrategy.Optimistic) + { + SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); + return await action(context, combinedToken).ConfigureAwait(continueOnCapturedContext); + } - Task timeoutTask = timeoutCancellationTokenSource.Token.AsTask(); + // else: timeoutStrategy == TimeoutStrategy.Pessimistic - SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); + Task timeoutTask = timeoutCancellationTokenSource.Token.AsTask(); - actionTask = action(context, combinedToken); + SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); - return await (await Task.WhenAny(actionTask, timeoutTask).ConfigureAwait(continueOnCapturedContext)).ConfigureAwait(continueOnCapturedContext); + actionTask = action(context, combinedToken); - } - catch (Exception ex) - { - // Note that we cannot rely on testing (operationCanceledException.CancellationToken == combinedToken || operationCanceledException.CancellationToken == timeoutCancellationTokenSource.Token) - // as either of those tokens could have been onward combined with another token by executed code, and so may not be the token expressed on operationCanceledException.CancellationToken. - if (ex is OperationCanceledException && timeoutCancellationTokenSource.IsCancellationRequested) - { - await onTimeoutAsync(context, timeout, actionTask, ex).ConfigureAwait(continueOnCapturedContext); - throw new TimeoutRejectedException("The delegate executed asynchronously through TimeoutPolicy did not complete within the timeout.", ex); - } + return await (await Task.WhenAny(actionTask, timeoutTask).ConfigureAwait(continueOnCapturedContext)).ConfigureAwait(continueOnCapturedContext); - throw; - } + } + catch (Exception ex) + { + // Note that we cannot rely on testing (operationCanceledException.CancellationToken == combinedToken || operationCanceledException.CancellationToken == timeoutCancellationTokenSource.Token) + // as either of those tokens could have been onward combined with another token by executed code, and so may not be the token expressed on operationCanceledException.CancellationToken. + if (ex is OperationCanceledException && timeoutCancellationTokenSource.IsCancellationRequested) + { + await onTimeoutAsync(context, timeout, actionTask, ex).ConfigureAwait(continueOnCapturedContext); + throw new TimeoutRejectedException("The delegate executed asynchronously through TimeoutPolicy did not complete within the timeout.", ex); } + + throw; } } diff --git a/src/Polly/Timeout/TimeoutEngine.cs b/src/Polly/Timeout/TimeoutEngine.cs index 77c19585ff6..b49f6ee1f7a 100644 --- a/src/Polly/Timeout/TimeoutEngine.cs +++ b/src/Polly/Timeout/TimeoutEngine.cs @@ -15,52 +15,49 @@ internal static TResult Implementation( cancellationToken.ThrowIfCancellationRequested(); TimeSpan timeout = timeoutProvider(context); - using (CancellationTokenSource timeoutCancellationTokenSource = new CancellationTokenSource()) - { - using (CancellationTokenSource combinedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token)) - { - CancellationToken combinedToken = combinedTokenSource.Token; + using var timeoutCancellationTokenSource = new CancellationTokenSource(); + using var combinedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token); - Task actionTask = null; - try - { - if (timeoutStrategy == TimeoutStrategy.Optimistic) - { - SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); - return action(context, combinedToken); - } + CancellationToken combinedToken = combinedTokenSource.Token; - // else: timeoutStrategy == TimeoutStrategy.Pessimistic + Task actionTask = null; + try + { + if (timeoutStrategy == TimeoutStrategy.Optimistic) + { + SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); + return action(context, combinedToken); + } - SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); + // else: timeoutStrategy == TimeoutStrategy.Pessimistic - actionTask = Task.Run(() => - action(context, combinedToken) // cancellation token here allows the user delegate to react to cancellation: possibly clear up; then throw an OperationCanceledException. - , combinedToken); // cancellation token here only allows Task.Run() to not begin the passed delegate at all, if cancellation occurs prior to invoking the delegate. - try - { - actionTask.Wait(timeoutCancellationTokenSource.Token); // cancellation token here cancels the Wait() and causes it to throw, but does not cancel actionTask. We use only timeoutCancellationTokenSource.Token here, not combinedToken. If we allowed the user's cancellation token to cancel the Wait(), in this pessimistic scenario where the user delegate may not observe that cancellation, that would create a no-longer-observed task. That task could in turn later fault before completing, risking an UnobservedTaskException. - } - catch (AggregateException ex) when (ex.InnerExceptions.Count == 1) // Issue #270. Unwrap extra AggregateException caused by the way pessimistic timeout policy for synchronous executions is necessarily constructed. - { - ExceptionDispatchInfo.Capture(ex.InnerException).Throw(); - } + SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); - return actionTask.Result; - } - catch (Exception ex) - { - // Note that we cannot rely on testing (operationCanceledException.CancellationToken == combinedToken || operationCanceledException.CancellationToken == timeoutCancellationTokenSource.Token) - // as either of those tokens could have been onward combined with another token by executed code, and so may not be the token expressed on operationCanceledException.CancellationToken. - if (ex is OperationCanceledException && timeoutCancellationTokenSource.IsCancellationRequested) - { - onTimeout(context, timeout, actionTask, ex); - throw new TimeoutRejectedException("The delegate executed through TimeoutPolicy did not complete within the timeout.", ex); - } + actionTask = Task.Run(() => + action(context, combinedToken) // cancellation token here allows the user delegate to react to cancellation: possibly clear up; then throw an OperationCanceledException. + , combinedToken); // cancellation token here only allows Task.Run() to not begin the passed delegate at all, if cancellation occurs prior to invoking the delegate. + try + { + actionTask.Wait(timeoutCancellationTokenSource.Token); // cancellation token here cancels the Wait() and causes it to throw, but does not cancel actionTask. We use only timeoutCancellationTokenSource.Token here, not combinedToken. If we allowed the user's cancellation token to cancel the Wait(), in this pessimistic scenario where the user delegate may not observe that cancellation, that would create a no-longer-observed task. That task could in turn later fault before completing, risking an UnobservedTaskException. + } + catch (AggregateException ex) when (ex.InnerExceptions.Count == 1) // Issue #270. Unwrap extra AggregateException caused by the way pessimistic timeout policy for synchronous executions is necessarily constructed. + { + ExceptionDispatchInfo.Capture(ex.InnerException).Throw(); + } - throw; - } + return actionTask.Result; + } + catch (Exception ex) + { + // Note that we cannot rely on testing (operationCanceledException.CancellationToken == combinedToken || operationCanceledException.CancellationToken == timeoutCancellationTokenSource.Token) + // as either of those tokens could have been onward combined with another token by executed code, and so may not be the token expressed on operationCanceledException.CancellationToken. + if (ex is OperationCanceledException && timeoutCancellationTokenSource.IsCancellationRequested) + { + onTimeout(context, timeout, actionTask, ex); + throw new TimeoutRejectedException("The delegate executed through TimeoutPolicy did not complete within the timeout.", ex); } + + throw; } } }