Simple circuit breaker implementation

Question

I am trying to implement a basic circuit breaker design for my internal API calls. I would appreciate some criticism and feedback about my code. I am also planning to implement an interface off of the class once I am happy with it. As mentioned this circuit breaker design will be used for internal gRPC calls between my microservices. This was written in .NET Core 3.0.

I look forward to your feedback!

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Text;
using System.Timers;
namespace CircuitBreaker
{
 public class CircuitBreaker
 {
 private Action _currentAction;
 private int _failureCount = 0;
 private readonly int _threshold = 0;
 private readonly System.Timers.Timer _timer;
 private CircuitState State { get; set; }
 public enum CircuitState
 {
 Closed,
 Open,
 HalfOpen
 }
 public CircuitBreaker(int threshold, int timeOut)
 {
 State = CircuitState.Closed;
 _threshold = threshold;
 _timer = new Timer(timeOut);
 _timer.Elapsed += TimerElapsed;
 }
 public void ExecuteAction(Action action)
 {
 _currentAction = action;
 try
 {
 action();
 }
 catch (Exception ex)
 {
 if (ex.InnerException == null)
 throw;
 if (State == CircuitState.HalfOpen)
 Trip();
 else if (_failureCount < _threshold)
 {
 _failureCount++;
 Invoke();
 }
 else if(_failureCount >= _threshold)
 Trip();
 }
 if(State == CircuitState.HalfOpen)
 Reset();
 if (_failureCount > 0)
 _failureCount = 0;
 }
 public void Trip()
 {
 if (State != CircuitState.Open)
 ChangeState(CircuitState.Open);
 _timer.Start();
 }
 public void Reset()
 {
 ChangeState(CircuitState.Closed);
 _timer.Stop();
 }
 private void ChangeState(CircuitState callerCircuitState)
 {
 State = callerCircuitState;
 }
 private void Invoke()
 {
 ExecuteAction(_currentAction);
 }
 private void TimerElapsed(object sender, System.Timers.ElapsedEventArgs e)
 {
 if (State == CircuitState.Open)
 {
 ChangeState(CircuitState.HalfOpen);
 _timer.Stop();
 Invoke();
 }
 }
 }
}

• iSR5: Have you checked out Polly on Nuget? if not, check it out, as it has already Circuit Breaker github.com/App-vNext/Polly
• Tachyon: I had a look at Polly, but would really like my own implementation to be reviewed. :)

Answer 1

Welcome to Code Review. Not too bad but I would suggest a few things.

_failureCount is only used in ExecuteAction so I would only define it locally to ExecuteAction and renamed it simply failureCount.

You may want to expose threshold publicly as a property since different instances could have differing thresholds. Suggest:

public int Threshold { get; }

Consider having a ToString() override.

CircuitState enum is okay where its at, but if it were me, I usually define it external to the class.

You may consider having the State be gettable publicly. Suggest:

public CircuitState State { get; private set; }

I see no reason for Invoke(). Just call ExecuteAction(_currentAction) directly.

The constructor has a timeout parameter. I would encourage clarity in the name with millisecondsTimeout.

And finally, the biggest issue I see is that you really should get into the practice of use { } with if. I know C++ was okay with this, and C# allows it, but here at CR we strongly discourage it because it could lead to nefarious hard-to-find bugs. There are lots of places where you would change this; here is but one example:

if (State != CircuitState.Open)
{
 ChangeState(CircuitState.Open);
}

• Malivil: You can't just move _failureCount to be a local variable. It needs to be in the class scope so it can count the number of failures over a set of ExecuteAction calls, otherwise it will never be greater than 1 due to the recursive (through Invoke) nature of ExectueAction. You could, instead, move it to be a parameter of ExecuteAction with a default value of 0 and then pass it in the call that replaces Invoke if you want it to not be in the class scope.