Will this Circuit Breaker catch fire?

Question

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:

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

    /// <summary>
    /// Changes the state of the breaker to <see cref="ClosedState"/>, allowing normal execution.
    /// </summary>
    void ToClosedState();

    /// <summary>
    /// Trips the breaker and changes the state to <see cref="OpenState"/>, blocking normal execution.
    /// </summary>
    void ToOpenState();

    /// <summary>
    /// Changes the state of the breaker to <see cref="HalfOpenState"/>, allowing normal execution.
    /// </summary>
    void ToHalfOpenState();

    /// <summary>
    /// Attempts to execute the specified action.
    /// </summary>
    /// <param name="action">A method that takes no parameters and returns void.</param>
    void Execute(Action action);

    /// <summary>
    /// Attempts to execute the specified action.
    /// </summary>
    /// <typeparam name="T">The return type.</typeparam>
    /// <param name="action">A method that takes no parameters and returns a value.</param>
    /// <returns></returns>
    T Execute<T>(Func<T> action);

    /// <summary>
    /// Increments the failure count.
    /// </summary>
    void IncrementFailCount();

    /// <summary>
    /// Resets the failure count.
    /// </summary>
    void ResetFailCount();

    /// <summary>
    /// Returns true if failure count has reached a threshold and breaker should be tripped.
    /// </summary>
    /// <returns></returns>
    bool IsThresholdReached();

    /// <summary>
    /// Implements actions to take upon circuit breaker tripping.
    /// </summary>
    /// <param name="e"></param>
    void OnCircuitBreakerTripped(Exception e);
}

With a base class for all circuit breaker states:

/// <summary>
/// A base class for all <see cref="CircuitBreaker"/> states.
/// </summary>
public abstract class CircuitBreakerStateBase
{
    /// <summary>
    /// The constructor-injected circuit breaker.
    /// </summary>
    protected readonly ICircuitBreaker CircuitBreaker;

    /// <summary>
    /// Initializes a new cicuit breaker state.
    /// </summary>
    /// <param name="circuitBreaker">The circuit breaker.</param>
    protected CircuitBreakerStateBase(ICircuitBreaker circuitBreaker)
    {
        CircuitBreaker = circuitBreaker;
    }

    /// <summary>
    /// A method that runs before the protected code.
    /// </summary>
    public virtual void BeforeExecute() { }

    /// <summary>
    /// A method that runs after the protected code.
    /// </summary>
    public virtual void AfterExecute() { }

    /// <summary>
    /// A method that runs upon failure of the protected code.
    /// </summary>
    /// <param name="e">The exception that was caught.</param>
    public virtual void OnException(Exception e)
    {
        CircuitBreaker.IncrementFailCount();
    }
}

ClosedState allows normal execution of the "protected code", and if that code throws, OnException will increment the failure count and switch the circuit breaker to OpenState if the code has thrown a given number of times:

/// <summary>
/// A state that indicates that the circuit is closed and normal execution is allowed.
/// </summary>
public class ClosedState : CircuitBreakerStateBase
{
    /// <summary>
    /// Initializes a <see cref="ClosedState"/> circuit breaker state, meaning circuit is closed and operating normally.
    /// </summary>
    /// <param name="circuitBreaker">The circuit breaker.</param>
    public ClosedState(ICircuitBreaker circuitBreaker) 
        : base(circuitBreaker)
    {
        CircuitBreaker.ResetFailCount();
    }

    /// <summary>
    /// A method that runs upon failure of the protected code.
    /// </summary>
    /// <param name="e">The exception that was caught.</param>
    public override void OnException(Exception e)
    {
        base.OnException(e);
        if (!CircuitBreaker.IsThresholdReached()) return;

        CircuitBreaker.ToOpenState();
        CircuitBreaker.OnCircuitBreakerTripped(e);
    }
}

When the circuit is in OpenState, normal execution is completely blocked for a given timeout delay - any attempt to run the protected code in that state will fire up an OpenCircuitException. After the timeout has elapsed, the circuit goes into HalfOpenState:

/// <summary>
/// A state that indicates that the circuit is opened and normal execution is blocked.
/// </summary>
public class OpenState : CircuitBreakerStateBase
{
    private readonly Timer _timer;

    /// <summary>
    /// Initializes a <see cref="OpenState"/> circuit breaker state, meaning circuit is open and will not operate normally.
    /// </summary>
    /// <param name="circuitBreaker">The circuit breaker.</param>
    public OpenState(ICircuitBreaker circuitBreaker)
        : base(circuitBreaker)
    {
        _timer = new Timer(CircuitBreaker.Timeout.TotalMilliseconds);
        _timer.Elapsed += TimeoutReached;
        _timer.AutoReset = false;
        _timer.Start();
    }

    private void TimeoutReached(object sender, ElapsedEventArgs e)
    {
        CircuitBreaker.ToHalfOpenState();
    }

    /// <summary>
    /// A method that runs before the protected code.
    /// </summary>
    /// <exception cref="OpenCircuitException"></exception>
    public override void BeforeExecute()
    {
        base.BeforeExecute();
        throw new OpenCircuitException();
    }
}

A half-open circuit breaker will try to run the protected code and close the circuit if it succeeds, or fall back to OpenState if it fails again:

/// <summary>
/// A state that indicates that the circuit is half-opened; successful execution will close the circuit, failure will re-open it.
/// </summary>
public class HalfOpenState : CircuitBreakerStateBase
{
    /// <summary>
    /// Initializes a <see cref="HalfOpenState"/> circuit breaker state, meaning circuit is half-open and will operate normally.
    /// </summary>
    /// <param name="circuitBreaker">The circuit breaker.</param>
    public HalfOpenState(ICircuitBreaker circuitBreaker)
        : base(circuitBreaker)
    { }

    /// <summary>
    /// A method that runs upon failure of the protected code.
    /// </summary>
    /// <param name="e">The exception that was caught.</param>
    public override void OnException(Exception e)
    {
        base.OnException(e);
        CircuitBreaker.ToOpenState();
        CircuitBreaker.OnCircuitBreakerTripped(e);
    }

    /// <summary>
    /// A method that runs after the protected code.
    /// </summary>
    public override void AfterExecute()
    {
        base.AfterExecute();
        CircuitBreaker.ToClosedState();
    }
}

---

Then you can address this cross-cutting concern with DI Interception, with an interceptor that can intercept the calls and wrap them in your circuit breaker:

/// <summary>
/// An interceptor that implements a CircuitBreaker pattern.
/// </summary>
public class CircuitBreakerInterceptor : IInterceptor
{
    private readonly ICircuitBreaker _breaker;

    /// <summary>
    /// Initializes a new instance of the <see cref="CircuitBreakerInterceptor"/> class.
    /// </summary>
    /// <param name="breaker">An implemtation of a CircuitBreaker.</param>
    public CircuitBreakerInterceptor(ICircuitBreaker breaker)
    {
        _breaker = breaker;
    }

    /// <summary>
    /// A method that intercepts specified invocation, wrapping the call with the CircuitBreaker.
    /// </summary>
    /// <exception cref="OpenCircuitException"> thrown when circuit breaker is in <see cref="OpenState"/> state.</exception>
    /// <param name="invocation"></param>
    public void Intercept(IInvocation invocation)
    {
        _breaker.Execute(invocation.Proceed);
    }
}

---

An implementation of an ICircuitBreaker could look like this - here I'm just taking in a logger, but it could just as well be a MessageBoxCircuitBreaker that displays a message instead:

public class LoggingCircuitBreaker : ICircuitBreaker
{
    private readonly ILogger _logger;
    private readonly object _monitor = new object();
    private CircuitBreakerStateBase _state;

    private static int _failCount;
    /// <summary>
    /// Gets the failure count.
    /// </summary>
    /// <value>
    /// The failure count.
    /// </value>
    public int FailCount { get { return _failCount; } }

    /// <summary>
    /// Gets the failure threshold, representing the number of allowed failures before circuit switches to <see cref="OpenState"/>
    /// </summary>
    /// <value>
    /// The failure threshold.
    /// </value>
    public int Threshold { get; private set; }
    /// <summary>
    /// A <see cref="TimeSpan" /> indicating the duration of the <see cref="OpenState" /> state.
    /// </summary>
    public TimeSpan Timeout { get; private set; }

    /// <summary>
    /// Creates a new CircuitBreaker that logs exceptions.
    /// </summary>
    /// <param name="threshold">Number of allowed failures in Closed state.</param>
    /// <param name="timeout">A <see cref="TimeSpan"/> that determines how long a breaker remains in Open state.</param>
    /// <param name="logger"></param>
    public LoggingCircuitBreaker(int threshold, TimeSpan timeout, ILogger logger)
    {
        if (threshold < 1) throw new ArgumentException("threshold", "Threshold must be greater than 1.");
        if (timeout.TotalMilliseconds < 1) throw  new ArgumentException("timeout", "Timeout must be greater than 1 millisecond.");

        _logger = logger;

        Threshold = threshold;
        Timeout = timeout;
        ToClosedState();
    }

    /// <summary>
    /// Changes the state of the breaker to <see cref="ClosedState" />, allowing normal execution.
    /// </summary>
    public void ToClosedState()
    {
        lock (_monitor)
        {
            _state = new ClosedState(this);
        }
    }

    /// <summary>
    /// Trips the breaker and changes the state to <see cref="OpenState" />, blocking normal execution.
    /// </summary>
    public void ToOpenState()
    {
        lock (_monitor)
        {
            _state = new OpenState(this);
        }
    }

    /// <summary>
    /// Changes the state of the breaker to <see cref="HalfOpenState" />, allowing normal execution.
    /// </summary>
    public void ToHalfOpenState()
    {
        lock (_monitor)
        {
            _state = new HalfOpenState(this);
        }
    }

    /// <summary>
    /// Increments the failure count.
    /// </summary>
    public void IncrementFailCount()
    {
        Interlocked.Increment(ref _failCount);
    }

    /// <summary>
    /// Resets the failure count.
    /// </summary>
    public void ResetFailCount()
    {
        lock (_monitor)
        {
            _failCount = 0;
        }
    }

    /// <summary>
    /// Returns true if failure count has reached a threshold and breaker should be tripped.
    /// </summary>
    /// <returns></returns>
    public bool IsThresholdReached()
    {
        return _failCount >= Threshold;
    }

    /// <summary>
    /// Attempts to execute the specified action.
    /// </summary>
    /// <param name="action">A method that takes no parameters and returns void.</param>
    public void Execute(Action action)
    {
        lock (_monitor) { _state.BeforeExecute(); }

        try
        {
            action();
        }
        catch (Exception e)
        {
            lock (_monitor) { _state.OnException(e); }
            throw;
        }

        lock (_monitor) { _state.AfterExecute(); }
    }

    /// <summary>
    /// Attempts to execute the specified action.
    /// </summary>
    /// <typeparam name="T">The return type.</typeparam>
    /// <param name="action">A method that takes no parameters and returns a value.</param>
    /// <returns></returns>
    public T Execute<T>(Func<T> action)
    {
        lock (_monitor) { _state.BeforeExecute(); }

        T result;
        try
        {
            result = action();
        }
        catch (Exception e)
        {
            lock (_monitor) { _state.OnException(e); }
            throw;
        }

        lock (_monitor) { _state.AfterExecute(); }
        return result;
    }

    /// <summary>
    /// Implements actions to take upon circuit breaker tripping.
    /// </summary>
    /// <param name="e">The exception that tripped the circuit breaker.</param>
    public void OnCircuitBreakerTripped(Exception e)
    {
        _logger.ErrorException("Circuit breaker tripped!", e);
    }
}

The above implementation is meant to be thread-safe - but thread safety isn't exactly my cup of tea and I'd like to know if the code has potential race conditions or other threading issues I'm not aware of.

Of course every other aspect of the code is open to constructive comments (I'm seeing quite a few code style /readability points that I'd write differently today).

Answer 1

Style/Readability

• Unnecessary XML comments. For example:

  /// <summary>
  /// Gets the failure count.
  /// </summary>
  /// <value>
  /// The failure count.
  /// </value>
  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.:

  /// <summary>
  /// Gets the failure threshold, representing the number of allowed failures before circuit switches to <see cref="OpenState"/>
  /// </summary>
  /// <value>
  /// The failure threshold.
  /// </value>
  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 CloseState itself
• OnCircuitBreakerTripped: This actually doesn't need to be here at all. Currently the circuit breaker calls _state.OnException(e), only for the state to then call back from that method to CircuitBreaker.OnCircuitBreakerTripped(e). This should be removed from the interface and either made a protected method on a base circuit breaker class or just left to the specific circuit breaker classes which need it.

This change is relatively straightforward. Parameters such as the timeout and threshold will be passed into the state's constructor and stored on the state. Counts are also stored on the state. Preferably we'd avoid the use of statics to store information like this, which means storing the state objects rather than constructing a new one every time there's a change, but that is also not too difficult.

Removing the To...State methods

So we've cut down the interface and cleaned up most of the inappropriate intimacy. Potentially at this point the solution is good enough. But the To...State methods still smell a bit, requiring the state classes to be passed an ICircuitBreaker. One example of when this would cause problems is if a new state needed to be added. For example, perhaps two graduated half-open states, the first one letting through very few executions and the second one letting through more.

It's not very likely to come up, so further refactoring may not be worthwhile, but one approach is for the states to hold references to the other states which they can transition to, and return the resultant state when they act. So BeforeExecute, AfterExecute and OnException would all return states.

So for example, since ClosedState can transition to OpenState, it would need to be passed a reference to the instance of the open state. Then instead of calling CircuitBreaker.ToOpenState, it would just return that open state. Or if no transition was needed, it would return itself. This allows the reference to ICircuitBreaker to be removed altogether, along with the To...State methods.

The fly in the ointment is that the states cannot all be passed to each other on construction because of the circular chain of references. There's a few ways to deal with this, but injecting via a setter is probably fine since there's only one place that the states will be built- in the circuit breaker's constructor.

If these methods are not removed altogether as described above, a different potential improvement would be to move them to a different interface, also implemented by any concrete circuit breakers. This would give better adherence to the interface segregation principle.

Concurrency and Asynchronicity

Unfortunately neither of these are things I have a lot of experience with, but there are three potential requirements related to to them:

• Multiple concurrent executions with the same circuit breaker
• Asynchronous executions with a circuit breaker
• Multiple different circuit breaker instances.

Concurrency is mentioned in the msdn article:

The same circuit breaker could be accessed by a large number of concurrent instances of an application. The implementation should not block concurrent requests or add excessive overhead to each call to an operation.

Due to your locking in the execute methods, this requirement won't be met, each execution will have to wait for the previous to complete to free up the resource. Depending on the situation, this could be a serious performance issue.

Multiple circuit breakers may be required if the pattern is used in multiple places where the expected reasons for failure are unrelated. The simplest way to make this possible is to avoid static members on any of the relevant classes.

I'm sure there's more to be said about concurrency and asynchronicity- in particular how to achieve effective locking- but I'll leave them up to another answerer.

Misc

• The use of an Action parameter to Execute by the interceptor is really nice. It avoids having to go through overloads for every one of the many Func and Action classes, or lose type safety by having a general Delegate parameter with the arguments as an object[]. So given that you can handle everything with that one method, why is there a version that takes a Func? This looks like it should be removed.
• As I alluded to a couple of times ICircuitBreaker should have a base class, since some of the members will be common to most or all concrete implementations.

Answer 2

There are a few fishy points as you say:

1. The most glaring one: ICircuitBreaker does not follow interface-segregation principle.

2. States know too much about the object they are states of.

3. Copy/Pasted Execute method.

4. LoggingCircuitBreaker has little to do with logging.

Other smaller points:

1. Static state: _failCount. Making it non-static is not a refactoring, as it changes behavior, but I say it is more a bugfix.

2. Doing something non-trivial in constructor: OpenState.

3. That non-trivial thing is a unnamed code chunk.

4. Programmer needs to remember calling base.OnException(e);

5. Programmer needs to remember calling CircuitBreaker.OnCircuitBreakerTripped(e); after calling CircuitBreaker.ToOpenState();

I will suggest refactorings addressing those points. I wouldn't suggest a plain rewrite, as rewriting means writing new code, and new code comes with new bugs.

To deal with ICircuitBreaker not following ISP, you should just split the interface. The principle is called interface segregation for a reason. The problem may show itself to the reader as a MethodX does not belong on InterfaceY; but as you said to @svick you can not just delete it from the interface, without causing compiler errors, fixing which may cause even more compile errors. I kept ICircuitBreaker for the Execute methods and ICircuitStateMachine for everything else. Naming is hard, but they don't have to be perfect: you can always (re)rename things.

Now the opportunity to deal with the repetitive Execute methods presents itself. I would advise against just deleting T Execute<T>(Func<T> action); as it is the more general case, and a method invocation not returning a value is a peculiarity of the AOP library you happened to be using. What I would do is implement the specific case as an extension:

public interface ICircuitBreaker
{
    T Execute<T>(Func<T> action);
}

public static class CircuitBreakerExtensions
{
    public static void Execute(this ICircuitBreaker circuitBreaker, Action action)
    {
        circuitBreaker.Execute<object>(() => { action(); return null; });
    }
}

We can now move logging concern out of the LoggingCircuitBreaker. I wouldn't just make the class abstract. Inheritance goes against composability and shouldn't make classes abstract unless moved/removed functionality is sine qua non. In this case method name OnCircuitBreakerTripped already says what we should do, convert it to an event:

public event Action<Exception> OnCircuitBreakerTripped;

then at the use site:

circuitBreaker.OnCircuitBreakerTripped += e => {
    _logger.ErrorException("Circuit breaker tripped!", e);
};

This also breaks the unnecessary dependency from CircuitBreaker to ILogger.

By now we fixed the interface of the library, now is a good time to stop refactoring.

But I did not :) and I did many smaller refactorings until I reduced the ICircuitStateMachine (everything on the ICircuitBreaker except Execute) to

public enum States {Open = 1, Closed, HalfOpen}

Though I don't remember all of them. :) Here is the overview and the end result:

In CircuitBreakerStateBase, we see that AfterExecute and OnException can cause state transition, whereas BeforeExecute cannot. AfterExecute and OnException are some kind of event handler, whereas BeforeExecute is some kind of a validation method. I renamed AfterExecute to OnSuccess, and OnException to OnFailure. BeforeExecute was renamed as ValidateNotOpen. This way it is less conceptually coupled to how it is used and more in line with what it does.

public abstract class CircuitBreakerStateBase
{
    public virtual void ValidateNotOpen() {}

    public virtual States OnSuccess() { return 0; }

    public virtual States OnFailure(Exception e) { return 0; }
}

Let's see how the states look:

public class OpenState : CircuitBreakerStateBase
{
    public override void ValidateNotOpen()
    {
        throw new OpenCircuitException();
    }
}

public class HalfOpenState : CircuitBreakerStateBase
{
    public override States OnFailure(Exception e)
    {
        return States.Open;
    }

    public override States OnSuccess()
    {
        return States.Closed;
    }
}

public class ClosedState : CircuitBreakerStateBase
{
    private int failCount = 0;
    private int threshold;

    public ClosedState(int threshold) 
    {
        this.threshold = threshold;
    }

    public override States OnFailure(Exception e)
    {
        failCount++;
        if (failCount < threshold) return 0;

        return States.Open;
    }

}

This reads pretty much like a State Transition Table of a Finite State Machine with two inputs: Success and Failure. OpenState clearly a sink node for example: no transitions.

It actually is a transition table:

private void ToState(States nextState)
{
    ToState(nextState, null);
}

private void ToState(States nextState, Exception e)
{
    if (nextState == 0) return;

    switch(nextState)
    {
        case States.Open: ToOpenState(e); break;
        case States.Closed: ToClosedState(); break;
        case States.HalfOpen: ToHalfOpenState(); break;
        default: throw new InvalidOperationException();
    }
}

I think it is now easier to understand what Execute does without reading each of BeforeExecute, AfterExecute, OnException as their names had previously no information content. That much decoupling is appropriate for Interceptor.Proceed() or some totally abstract ExecutionWrapper.Execute() but not for a class named CircuitBreaker with clear specs for what it does before and after execution.

public T Execute<T>(Func<T> action)
{
    lock (_monitor) { _state.ValidateNotOpen(); }

    T result;
    try
    {
        result = action();
    }
    catch (Exception e)
    {
        lock (_monitor) { 
            ToState(_state.OnFailure(e), e);
        }
        throw;
    }

    lock (_monitor) { 
        ToState(_state.OnSuccess());
    }

    return result;
}

ToClosedState and ToHalfOpenState are as is, but some behavior has moved into ToOpenState.

private void ToOpenState(Exception e)
{
    lock (_monitor)
    {
        _state = new OpenState();
    }
    AddTimerAction(timeout, () => ToHalfOpenState());

    OnCircuitBreakerTripped(e);
}

private static void AddTimerAction(TimeSpan timeOut, Action action)
{
    Timer _timer = new Timer(timeOut.TotalMilliseconds);
    _timer.Elapsed += (sender, eventArgs) => action();
    _timer.AutoReset = false;
    _timer.Start();
}

After these changes the programmer does not need to remember calling base.XXX() at the beginning of state methods or call OnCircuitBreakerTripped at the end.

AddTimerAction is moved out of constructor. It is now a stateless static utility function, thus can be moved elsewhere, made public, or easily tested extensively on its own.

As a finishing note: Since I am not responsible for the code, I played quite fast&loose with refactoring. You probably would do some real testing, but I used the following to see at least code kept compiling and doing something somewhat sensible:

var circuitBreaker = new CircuitBreaker(
    2, TimeSpan.FromMilliseconds(1000));

circuitBreaker.OnCircuitBreakerTripped += e => {
    Console.WriteLine("OnCircuitBreakerTripped");
    Console.WriteLine(e);
};

for (int i = 0; i < 10; i++) 
{
    circuitBreaker.Execute(
        () => {
            Console.WriteLine("success");
        });
}

for (int i = 0; i < 10; i++) 
{
    try 
    {
    circuitBreaker.Execute(
        () => {
            Console.WriteLine("fail");
            throw new Exception();
        });
    }
    catch(Exception) { }
}