diff --git a/gobreaker.go b/gobreaker.go index 7503a27..17b4a5c 100644 --- a/gobreaker.go +++ b/gobreaker.go @@ -128,6 +128,12 @@ type CircuitBreaker struct { generation uint64 counts Counts expiry time.Time + + // pendingNotify holds a state-change notification to be dispatched after + // the mutex is released. Calling onStateChange while the mutex is held + // causes a deadlock if the callback calls any CircuitBreaker method + // (e.g. Counts). See https://github.com/sony/gobreaker/issues/37 + pendingNotify *[2]State // [from, to]; nil means no pending notification } // TwoStepCircuitBreaker is like CircuitBreaker but instead of surrounding a function @@ -205,10 +211,18 @@ func (cb *CircuitBreaker) Name() string { // State returns the current state of the CircuitBreaker. func (cb *CircuitBreaker) State() State { cb.mutex.Lock() - defer cb.mutex.Unlock() now := time.Now() state, _ := cb.currentState(now) + + notify := cb.pendingNotify + cb.pendingNotify = nil + + cb.mutex.Unlock() + + if notify != nil { + cb.onStateChange(cb.name, notify[0], notify[1]) + } return state } @@ -275,35 +289,57 @@ func (tscb *TwoStepCircuitBreaker) Allow() (done func(success bool), err error) func (cb *CircuitBreaker) beforeRequest() (uint64, error) { cb.mutex.Lock() - defer cb.mutex.Unlock() now := time.Now() state, generation := cb.currentState(now) + var err error if state == StateOpen { - return generation, ErrOpenState + err = ErrOpenState } else if state == StateHalfOpen && cb.counts.Requests >= cb.maxRequests { - return generation, ErrTooManyRequests + err = ErrTooManyRequests + } else { + cb.counts.onRequest() } - cb.counts.onRequest() - return generation, nil + // Capture and clear the pending notification while still holding the lock + // so that concurrent goroutines don't steal each other's notifications. + notify := cb.pendingNotify + cb.pendingNotify = nil + + cb.mutex.Unlock() + + // Dispatch outside the mutex to prevent deadlock when the callback calls + // back into the CircuitBreaker (e.g. Counts). See issue #37. + if notify != nil { + cb.onStateChange(cb.name, notify[0], notify[1]) + } + return generation, err } func (cb *CircuitBreaker) afterRequest(before uint64, success bool) { cb.mutex.Lock() - defer cb.mutex.Unlock() now := time.Now() state, generation := cb.currentState(now) - if generation != before { - return + if generation == before { + if success { + cb.onSuccess(state, now) + } else { + cb.onFailure(state, now) + } } - if success { - cb.onSuccess(state, now) - } else { - cb.onFailure(state, now) + // Capture and clear the pending notification while still holding the lock. + notify := cb.pendingNotify + cb.pendingNotify = nil + + cb.mutex.Unlock() + + // Dispatch outside the mutex to prevent deadlock when the callback calls + // back into the CircuitBreaker (e.g. Counts). See issue #37. + if notify != nil { + cb.onStateChange(cb.name, notify[0], notify[1]) } } @@ -355,11 +391,15 @@ func (cb *CircuitBreaker) setState(state State, now time.Time) { cb.toNewGeneration(now) + // Record the transition for deferred dispatch outside the mutex. + // We overwrite any previous pending notification: only the latest + // transition matters when multiple happen within a single locked section. if cb.onStateChange != nil { - cb.onStateChange(cb.name, prev, state) + cb.pendingNotify = &[2]State{prev, state} } } + func (cb *CircuitBreaker) toNewGeneration(now time.Time) { cb.generation++ cb.counts.clear() diff --git a/gobreaker_test.go b/gobreaker_test.go index e1b5bc7..9a40183 100644 --- a/gobreaker_test.go +++ b/gobreaker_test.go @@ -366,6 +366,46 @@ func TestCustomIsSuccessful(t *testing.T) { } +// TestOnStateChangeNoDeadlock verifies that calling CircuitBreaker methods +// (e.g. Counts, State) from within the OnStateChange callback does not +// deadlock. Before the fix, OnStateChange was called while the internal +// mutex was held; any re-entrant call into the breaker would dead-lock. +// See https://github.com/sony/gobreaker/issues/37 +func TestOnStateChangeNoDeadlock(t *testing.T) { + done := make(chan struct{}) + + var cb *CircuitBreaker + cb = NewCircuitBreaker(Settings{ + Name: "deadlock-test", + MaxRequests: 1, + Timeout: time.Duration(10) * time.Millisecond, + ReadyToTrip: func(counts Counts) bool { return counts.ConsecutiveFailures >= 1 }, + OnStateChange: func(name string, from, to State) { + // Calling cb.Counts() here used to dead-lock because the mutex + // was still held when OnStateChange fired. + _ = cb.Counts() + _ = cb.State() + }, + }) + + // Trigger closed→open transition in a goroutine so that a timeout-based + // test harness can detect if we dead-lock. + go func() { + defer close(done) + _ = fail(cb) + // Trigger open→half-open by advancing past the timeout. + time.Sleep(time.Duration(20) * time.Millisecond) + _ = cb.State() + }() + + select { + case <-done: + // Success: no deadlock + case <-time.After(time.Duration(2) * time.Second): + t.Fatal("deadlock detected: OnStateChange blocked when calling Counts()/State()") + } +} + func TestCircuitBreakerInParallel(t *testing.T) { runtime.GOMAXPROCS(runtime.NumCPU())