rubyist / circuitbreaker Goto Github PK

Looking at the code, there is no protection against using a circuit in a multithreaded environment.
There are locks in panel for Add()/Get() but not for circuits themselves. To avoid Data Race, a mutex
lock was placed around the (*Breaker).Call(). Is there a better way to do it?

Got the following in our test:
WARNING: DATA RACE
Write by goroutine 60:
github.com/rubyist/circuitbreaker.(_Breaker).Success()
/home/vagrant/rio-workspace/src/github.com/rubyist/circuitbreaker/circuitbreaker.go:266 +0xb1
github.com/rubyist/circuitbreaker.(_Breaker).Call()
/home/vagrant/rio-workspace/src/github.com/rubyist/circuitbreaker/circuitbreaker.go:326 +0x124

Previous write by goroutine 57:
github.com/rubyist/circuitbreaker.(_Breaker).Success()
/home/vagrant/rio-workspace/src/github.com/rubyist/circuitbreaker/circuitbreaker.go:266 +0xb1
github.com/rubyist/circuitbreaker.(_Breaker).Call()
/home/vagrant/rio-workspace/src/github.com/rubyist/circuitbreaker/circuitbreaker.go:326 +0x124
github.comcast.com/viper-cog/rio-a8-updater/breaker.CallCircuitBreaker()

One might want to use something other than a TimeoutBreaker. Now that there's an interface, you should be able to give it any type of breaker.

I am using circuit.NewHTTPClient and was wondering what you think about this.

client = circuit.NewHTTPClient(time.Second*5, 10, nil)
response, err := client.Do(request)
if err != nil {
    return
}
defer response.Body.Close()

I get this error message on the defer
2015/09/02 01:39:50 http: panic serving 10.0.8.48:36515: runtime error: invalid memory address or nil pointer dereference

I haven't tested this but I think that the call is timing out. This means that a nil response body and nil error is returned. I think that the error is not being returned properly.

func (c *HTTPClient) Do(req *http.Request) (*http.Response, error) {
    var resp *http.Response
    var err error
    breaker := c.breakerLookup(req.URL.String())
    breaker.Call(func() error {
        resp, err = c.Client.Do(req)
        return err
    }, c.timeout)
    return resp, err
}

should look like this

func (c *HTTPClient) Do(req *http.Request) (*http.Response, error) {
    var resp *http.Response
    var err error
    breaker := c.breakerLookup(req.URL.String())
    err = breaker.Call(func() error {
        resp, err = c.Client.Do(req)
        return err
    }, c.timeout)
    return resp, err
}

Hi,

You are using an old import path : github.com/cenk/backoff wich is now : github.com/cenkalti/backoff

Is that possible to update it please ?

Thank you

    var count int

    cb := circuit.NewThresholdBreaker(3)
//circuit function should be call 3 times
    err := cb.Call(func() error {
        count += 1
        _, err := rpc.DialHTTP("tcp", "127.0.0.1:4000") //connection error occurred
        return err //err not nil
    }, 0)

    fmt.Printf("called %d times tripped: %t err:%v\n", count, cb.Tripped(), err)

Output:
called 1 times tripped: false err:dial tcp 127.0.0.1:4000: connection refused

We could perhaps condense the structure a bit. Now that we have CircuitBreaker as an interface, I'm not sure we need separate TrippableBreaker and ResettingBreaker structs. We could probably go with the following:

TrippableBreaker: This is the base circuit breaker which contains the trip state logic and auto reset logic.

FrequencyBreaker: This is a breaker that trips if a threshold is met within a certain time period.

ThresholdBreaker: This is a FrequencyBreaker with a time period of 0.

TimeoutBreaker: This is a FrequencyBreaker with a timeout (and possibly duration of 0).

Essentially, TrippableBreaker contains all of the trip/reset logic and the individual breaker types provide failure semantics.

When the function Call sets a timeout, a coroutine will be opened internally

circuitbreaker.go line 350 - line 353

Hope to provide AsyncCall , internal coroutines cannot be used

it's code

func TestBreaker22(t *testing.T) {
	brk := NewBreakerWithOptions(&Options{
		ShouldTrip: RateTripFunc(0.6, 100),
	})
	flag := 0
	start := time.Now().UnixNano()
	for i := 0; i < 200; i++ {
		err := brk.Call(func() error {
			return testDoSomething(flag)
		}, 0)

		last := atomic.LoadInt64(&brk.lastFailure)
		since := brk.Clock.Now().Sub(time.Unix(0, last))
		trip := atomic.LoadInt32(&brk.tripped)
		fmt.Printf("i:%d, trip:%d, last:%d, since:%+v, backOff:%+v, flag:%d, error:%s\n", i, trip, last, since, brk.nextBackOff, flag, err)

		if err == nil && brk.tripped == 0 {
			fmt.Printf("i:%d, trip:%d, duration:%d\n", i, brk.tripped, time.Now().UnixNano()-start)
			//break
		}
		if err != nil && strings.Contains(err.Error(), "breaker open") {
			flag = 1
			//time.Sleep(200 * time.Millisecond)
		}
	}
}

func testDoSomething(flag int) error {
	if flag == 0 {
		return fmt.Errorf("bad request")
	} else {
		return nil
	}
}

How do I calculate the recovery time?

This library suffers from go issue #599 when compiled for ARM using the gc compiler. The gccgo compiler appears to take care of it, but that limits the go version to <=1.6.1 for official gccgo releases. The following padding & reordering of the Breaker struct fixes the issue.

`// Breaker is the base of a circuit breaker. It maintains failure and success counters
// as well as the event subscribers.
type Breaker struct {
// BackOff is the backoff policy that is used when determining if the breaker should
// attempt to retry. A breaker created with NewBreaker will use an exponential backoff
// policy by default.
BackOff backoff.BackOff

// ShouldTrip is a TripFunc that determines whether a Fail() call should trip the breaker.
// A breaker created with NewBreaker will not have a ShouldTrip by default, and thus will
// never automatically trip.
ShouldTrip TripFunc

// Clock is used for controlling time in tests.
Clock clock.Clock

_              [4]byte // pad to fix golang issue #599
consecFailures int64
lastFailure    int64 // stored as nanoseconds since the Unix epoch
halfOpens      int64
counts         *window
nextBackOff    time.Duration
tripped        int32
broken         int32
eventReceivers []chan BreakerEvent
listeners      []chan ListenerEvent
backoffLock    sync.Mutex

}`

It is unclear the relationships between the following options when setting a circuit breaker: WindowTime, WindowBuckets, and Backoff. It looks like WindowTime is divided up into WindowBuckets for circuit polling, but then somehow the Backoff strategy plays into it. It is unclear how this relationship works out.

Calling Ready() on a half-open breaker will reset the breaker to open. I'm writing a test to better understand how Breaker behaves in a half-open state. I'm using Breaker.Ready() to detect when the Breaker transitions to half-open.

...
// Breaker trips to open
...

ready := cb.Ready()
fmt.Print("\nLet circuit transition to half-open\n\n")
for {
    if ready {
        break
    }
    time.Sleep(10 * time.Millisecond)
    ready = cb.Ready()
}

fmt.Printf("\nReady() == %v; Tripped == %v\n", ready, cb.Tripped())
fmt.Printf("Call Ready() again after it returns true, Ready() == %v; Tripped() == %v\n\n", 
               cb.Ready(), cb.Tripped())

When run it prints

Ready() == true; Tripped == true
Call Ready() again after it returns true, Ready() == false; Tripped() == true

Since Call() also invokes Ready(), using Ready() as I did above causes the subsequent call to Call() to fail since the circuit state is now open.

So, is there a way to test a Breaker to see if it's half-open without calling Ready()? Also, given Ready()'s side effects, perhaps it shouldn't be public?

UPDATE: After re-reading the README file I have a better understanding of the intended use of Ready() to allow the user to implement their own version of Call(). Even so, it would be nice to call out in the godoc that Ready() should not be used in conjunction with Call().

In my opinion this can be replaced by tracking success/failure counts during Success()/Failure() calls.

I have made a pull request for this performance enhancement, let me know what you think, thanks!

Add a breaker that trips based on frequency of errors rather than a count

No reaction to pull requests.

When adding a listener (buffered) channel with AddListener(), and consuming the events, the breaker can block forever. The test code in the linked gist can reproduce the issue with a reasonable chance:

https://gist.github.com/aryszka/115fea73da80422a6d46ff058c8dcb0b

If the latency of circuit remote call is higher than 1s，then One single success request would cause state switch HalfOpen to Closed
easily！

While the best solution is to detect Consecutive Success Request is reached the target, then switch switch HalfOpen to Closed 。

Here is the unit test you can produce this problem。

func TestThresholdBreakerResets(t *testing.T) {
	called := 0
	circuit := func() error {
		if called == 1 {
			called++
			return nil
		} else {
			called++
			return fmt.Errorf("mock error")
		}
	}
	remoteCall := 0
	c := clock.NewMock()
    
    //  Breaker 阈值设置为1，错误数大于1 则触发 Breaker Open
	cb := NewThresholdBreaker(1)
	cb.Clock = c
    
    // 第一次 call circuit
	err := cb.Call(circuit, 0)
	if err == nil {
		t.Fatal("Expected cb to return an error")
	}
    remoteCall++
	fmt.Printf("remoteCall:%d, err:%v, state:%d \n", remoteCall, err, cb.state())
    
    // 第一次 call 后，第二次 会返回 breaker open 错误
	err = cb.Call(circuit, 0)
	if err == nil {
		t.Fatal("Expected cb to return an error")
	}
	if err != ErrBreakerOpen {
		t.Fatal("Expected cb to return ErrBreakerOpen")
	}
    remoteCall++
	fmt.Printf("remoteCall:%d, err:%v, state:%d \n", remoteCall, err, cb.state())

    // 模拟等待已经超过指数退避时间，
    // 当第三次 call 后， 验证触发了 breaker 到 halfopen，然后再到 closed；
    // 第四次返回 circuit 的 mock error
    // 第五次返回 breaker open，此时又触发了 breaker open
	c.Add(cb.nextBackOff + 100000)
	for i := 0; i < 4; i++ {
		err = cb.Call(circuit, 0)
		remoteCall++
	    fmt.Printf("remoteCall:%d, err:%v, state:%d \n", remoteCall, err, cb.state())
	}
}

When getLatestBucket is called, buckets not in current window slide should be all reset.

....

w := newWindow(time.Millisecond*10,  2)

w.Fail()
time.Sleep(time.Millisecond * 20)
w.Fail()

fmt.Println(w.Failures())

... 

When run above code, the result should be 1, not 2. As window duration is 10ms, after sleep 20ms, all buckets are expired.

I think if a ResettingBreaker is manually tripped, it shouldn't re-evaluate after the timeout. It should sit until it's manually reset.

panel is created and the breaker added but you never do anything with it beyond that.

I think line 72 should look like this

brclient := &HTTPClient{Client: client, Panel: panel, timeout: timeout}

it is now at: github.com/cenk/backoff

A Panel could provide stats for the breakers it groups. For each breaker it could collect:

• Total failure count
• Failure rate
• Total trip count
• Trip rate
• Amount of time spent in a tripped state
• Number of attempted resets before success

The size/granularity of the sliding window for a breaker is hard-coded (https://github.com/rubyist/circuitbreaker/blob/master/circuitbreaker.go#L135). It would be nice to be able to configure that when creating a new breaker.

If I submit a PR for that, are you interested? I think if fields are added to Options, it can be done in a backwards-compatible manner.

We want to use circuit breaker instance in our healthcheck reporting. Would be nice if cb instance was able to report which errors triggered it to fail.
If that's something you consider useful for this library we can send a PR.

For consistency

Add some code to manually trip and reset a breaker. This would be handy for our good friends in ops.

when the breaker is open, the Call() func will return an error. any ideas to add a fallback func option for executing fallback func instead of return an error when the breaker is open?