4
\$\begingroup\$

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();
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Have you checked out Polly on Nuget? if not, check it out, as it has already Circuit Breaker github.com/App-vNext/Polly \$\endgroup\$ – iSR5 Dec 10 '19 at 16:40
  • 1
    \$\begingroup\$ I had a look at Polly, but would really like my own implementation to be reviewed. :) \$\endgroup\$ – Tachyon Dec 11 '19 at 8:14
2
\$\begingroup\$

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);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Malivil Dec 11 '19 at 18:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.