Will this Circuit Breaker catch fire?

I was going to post this code as an answer to a recent question, but I wrote this code a little while ago (like, a year ago; if I recall correctly I wrote this after reading this article) and I'd like to get some feedback on it instead.

The idea is to address a cross-cutting concern using DI interception, by implementing a CircuitBreakerInterceptor that intercepts calls and wraps them in a "retry" logic.

Actually MSDN is more precise about it:


The purpose of the Circuit Breaker pattern is different from that of the Retry Pattern. The Retry Pattern enables an application to retry an operation in the expectation that it will succeed. The Circuit Breaker pattern prevents an application from performing an operation that is likely to fail. An application may combine these two patterns by using the Retry pattern to invoke an operation through a circuit breaker. However, the retry logic should be sensitive to any exceptions returned by the circuit breaker and abandon retry attempts if the circuit breaker indicates that a fault is not transient.

Here's the interface for the circuit breaker:

```
///
/// An interface for a circuit breaker, which allows wrapping a feature in a "circuit" that can only execute when closed or half-closed.
///
public interface ICircuitBreaker
{
///
/// A indicating the duration of the state.
///
TimeSpan Timeout { get; }

///
/// Changes the state of the breaker to , allowing normal execution.
///
void ToClosedState();

///
/// Trips the breaker and changes the state to , blocking normal execution.
///
void ToOpenState();

///
/// Changes the state of the breaker to , allowing normal execution.
///
void ToHalfOpenState();

///
/// Attempts to execute the specified action.
///
/// A method that takes no parameters and returns void.
void Execute(Action action);

///
/// Attempts to execute the specified action.
///
/// The re

Style/Readability

-
Unnecessary XML comments. For example:

/// 
/// Gets the failure count.
/// 
/// 
/// The failure count.
/// 
public int FailCount { get { return _failCount; } }

The summary and value not only repeat each other, but also repeat the property name. Statement in comments of what's already stated clearly in the code is just another form of repetition, albeit a relatively benign one. At best adds nothing, at worst makes the code slightly harder to read.

-
Naming. The advantage of having those XML comments is that when you go through and prune the unnecessary ones, you'll also see if there are any where the document is needed because the member name itself is missing information it should have. E.g.:

/// 
/// Gets the failure threshold, representing the number of allowed failures before circuit switches to 
/// 
/// 
/// The failure threshold.
/// 
public int Threshold { get; private set; }

It's probably a matter of judgement whether the second clause of the summary comment is useful, but the first part as well as the value comment are a sign that this property should probably be called FailureThreshold

Simple vs. SOLID

As I'm sure you realise, it would be possible to do a much simpler implementation of this. As a sketch, the main execution method may look something like:

public void Execute(Action action)
{
    if(_currentState == CircuitState.Open)
        throw new OpenCircuitException();
    try
    {
        action();
        SetStateClosed();
    }
    catch
    {
        _failCounter++;
        if(_currentState == CircuitState.HalfOpen || _failCounter == _failThreshold)
            SetStateOpen();
        throw;
    }
}

The timer/half-open logic would go inside SetStateOpen, and hopefully the rest is clear just from the code. I'm not suggesting this is exactly how you'd write it, just giving an outline.

This would be a very simple implementation, it would only require this one public method, the CircuitState enum and the two private SetState... methods.

So, quickly running through programming principles, is there any where this falls down compared to the larger implementation? Going with SOLID, it conforms to the Single Responsibility Principle. Liskov Substitution, Interface Segregation and Dependency Inversion are irrelevant. But there is one problem point: the Open/Closed principle.

Clearly, most changes would require going in and modifying existing code, probably really getting our hands dirty mucking around with that logic. But now that we've identified that, there's two follow-up questions:

• Are the requirements for this class likely enough to change to justify the more complex design?

• Are the potential changes in requirement likely to require substantial modification of the design in the OP?

If the answer to question 1 is no, then you're done. A nice simple design along the lines of the previous outline will be fine, and anything much more will be needless complexity.

If the answer is yes, then the second question becomes relevant. So a focus for the rest of the review will be ensuring the answer to that second question is no.

Adding a success threshold and subsequent refactoring

One difference between this implementation and the description in the article is that in this version, the circuit breaker can only ever stay in the half-open state for one execution. If that execution is successful, it moves to closed, otherwise it moves to open. In the article, the transition from half-open to closed has a success threshold just like the transition from closed to open has a failure threshold. This is something that really should be in the design, in addition to being an illustrative driving example for refactoring.

So how would this be done with the current design? We'd have to add three members to the ICircuitBreaker interface: IncrementSuccessCount(), ResetSuccessCount(), IsSuccessThresholdReached(). Then we'd have to update the circuit breaker concrete classes- or potentially a base class if we had one- to implement them, as well as the private backing field. And the only class that will actually want to use this isn't the circuit breaker itself but the HalfOpenState.

This smells of feature envy. A good option is the usual one for feature envy: move the members that only particular states care about out of the ICircuitBreaker interface to the state class. So let's go through the interface members one by one and see what can be done with them:

• Timeout: Remove, this can be handled by passing it to OpenState's constructor instead.

• To...State(): For now these need to be left as they are, I'll address them a little further on

• Execute(): These are what external users of the circuit breaker actually need, so they need to be left, though I'll mention the Func version later.

• IncrementFailCount, ResetFailCount and IsThresholdReached: Remove, the fail counter and threshold will be handled by `

StackExchange Code Review Q#53991, answer score: 13