Repository: zeromicro/go-zero
Pull request #3897 (2024-02-06 08:02:18)

feat: optimize circuit breaker algorithm

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/zeromicro/go-zero/pull/3897?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`2ccef5b`)](https://app.codecov.io/gh/zeromicro/go-zero/commit/2ccef5bb4f92b8a5b41b2e5a986fcae65e5685c3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro) 94.41% compared to head [(`1ba7d46`)](https://app.codecov.io/gh/zeromicro/go-zero/pull/3897?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro) 94.43%.

<details><summary>Additional details and impacted files</summary>



| [Files](https://app.codecov.io/gh/zeromicro/go-zero/pull/3897?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro) | Coverage Δ | |
|---|---|---|
| [core/breaker/googlebreaker.go](https://app.codecov.io/gh/zeromicro/go-zero/pull/3897?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro#diff-Y29yZS9icmVha2VyL2dvb2dsZWJyZWFrZXIuZ28=) | `100.00% <100.00%> (ø)` | |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/zeromicro/go-zero/pull/3897/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro)

</details>

Patch:
diff --git a/core/breaker/breaker.go b/core/breaker/breaker.go
index 9fc4a483..3da681cd 100644
--- a/core/breaker/breaker.go
+++ b/core/breaker/breaker.go
@@ -53,16 +53,19 @@ type (
 		// DoWithFallback runs the fallback if the Breaker rejects the request.
 		// If a panic occurs in the request, the Breaker handles it as an error
 		// and causes the same panic again.
-		DoWithFallback(req func() error, fallback func(err error) error) error
+		DoWithFallback(req func() error, fallback Fallback) error
 
 		// DoWithFallbackAcceptable runs the given request if the Breaker accepts it.
 		// DoWithFallbackAcceptable runs the fallback if the Breaker rejects the request.
 		// If a panic occurs in the request, the Breaker handles it as an error
 		// and causes the same panic again.
 		// acceptable checks if it's a successful call, even if the error is not nil.
-		DoWithFallbackAcceptable(req func() error, fallback func(err error) error, acceptable Acceptable) error
+		DoWithFallbackAcceptable(req func() error, fallback Fallback, acceptable Acceptable) error
 	}
 
+	// Fallback is the func to be called if the request is rejected.
+	Fallback func(err error) error
+
 	// Option defines the method to customize a Breaker.
 	Option func(breaker *circuitBreaker)
 
@@ -86,12 +89,12 @@ type (
 
 	internalThrottle interface {
 		allow() (internalPromise, error)
-		doReq(req func() error, fallback func(err error) error, acceptable Acceptable) error
+		doReq(req func() error, fallback Fallback, acceptable Acceptable) error
 	}
 
 	throttle interface {
 		allow() (Promise, error)
-		doReq(req func() error, fallback func(err error) error, acceptable Acceptable) error
+		doReq(req func() error, fallback Fallback, acceptable Acceptable) error
 	}
 )
 
@@ -122,11 +125,11 @@ func (cb *circuitBreaker) DoWithAcceptable(req func() error, acceptable Acceptab
 	return cb.throttle.doReq(req, nil, acceptable)
 }
 
-func (cb *circuitBreaker) DoWithFallback(req func() error, fallback func(err error) error) error {
+func (cb *circuitBreaker) DoWithFallback(req func() error, fallback Fallback) error {
 	return cb.throttle.doReq(req, fallback, defaultAcceptable)
 }
 
-func (cb *circuitBreaker) DoWithFallbackAcceptable(req func() error, fallback func(err error) error,
+func (cb *circuitBreaker) DoWithFallbackAcceptable(req func() error, fallback Fallback,
 	acceptable Acceptable) error {
 	return cb.throttle.doReq(req, fallback, acceptable)
 }
@@ -168,7 +171,7 @@ func (lt loggedThrottle) allow() (Promise, error) {
 	}, lt.logError(err)
 }
 
-func (lt loggedThrottle) doReq(req func() error, fallback func(err error) error, acceptable Acceptable) error {
+func (lt loggedThrottle) doReq(req func() error, fallback Fallback, acceptable Acceptable) error {
 	return lt.logError(lt.internalThrottle.doReq(req, fallback, func(err error) bool {
 		accept := acceptable(err)
 		if !accept && err != nil {
diff --git a/core/breaker/breakers.go b/core/breaker/breakers.go
index be55d55a..314965c4 100644
--- a/core/breaker/breakers.go
+++ b/core/breaker/breakers.go
@@ -22,14 +22,14 @@ func DoWithAcceptable(name string, req func() error, acceptable Acceptable) erro
 }
 
 // DoWithFallback calls Breaker.DoWithFallback on the Breaker with given name.
-func DoWithFallback(name string, req func() error, fallback func(err error) error) error {
+func DoWithFallback(name string, req func() error, fallback Fallback) error {
 	return do(name, func(b Breaker) error {
 		return b.DoWithFallback(req, fallback)
 	})
 }
 
 // DoWithFallbackAcceptable calls Breaker.DoWithFallbackAcceptable on the Breaker with given name.
-func DoWithFallbackAcceptable(name string, req func() error, fallback func(err error) error,
+func DoWithFallbackAcceptable(name string, req func() error, fallback Fallback,
 	acceptable Acceptable) error {
 	return do(name, func(b Breaker) error {
 		return b.DoWithFallbackAcceptable(req, fallback, acceptable)
diff --git a/core/breaker/googlebreaker.go b/core/breaker/googlebreaker.go
index a8432e30..a15dfba8 100644
--- a/core/breaker/googlebreaker.go
+++ b/core/breaker/googlebreaker.go
@@ -60,8 +60,9 @@ func (b *googleBreaker) allow() (internalPromise, error) {
 	}, nil
 }
 
-func (b *googleBreaker) doReq(req func() error, fallback func(err error) error, acceptable Acceptable) error {
+func (b *googleBreaker) doReq(req func() error, fallback Fallback, acceptable Acceptable) error {
 	if err := b.accept(); err != nil {
+		b.markFailure()
 		if fallback != nil {
 			return fallback(err)
 		}
@@ -69,18 +70,19 @@ func (b *googleBreaker) doReq(req func() error, fallback func(err error) error,
 		return err
 	}
 
+	var success bool
 	defer func() {
-		if e := recover(); e != nil {
+		// if req() panic, success is false, mark as failure
+		if success {
+			b.markSuccess()
+		} else {
 			b.markFailure()
-			panic(e)
 		}
 	}()
 
 	err := req()
 	if acceptable(err) {
-		b.markSuccess()
-	} else {
-		b.markFailure()
+		success = true
 	}
 
 	return err
diff --git a/core/breaker/nopbreaker.go b/core/breaker/nopbreaker.go
index a1b2e7cc..cd030b63 100644
--- a/core/breaker/nopbreaker.go
+++ b/core/breaker/nopbreaker.go
@@ -24,12 +24,11 @@ func (b nopBreaker) DoWithAcceptable(req func() error, _ Acceptable) error {
 	return req()
 }
 
-func (b nopBreaker) DoWithFallback(req func() error, _ func(err error) error) error {
+func (b nopBreaker) DoWithFallback(req func() error, _ Fallback) error {
 	return req()
 }
 
-func (b nopBreaker) DoWithFallbackAcceptable(req func() error, _ func(err error) error,
-	_ Acceptable) error {
+func (b nopBreaker) DoWithFallbackAcceptable(req func() error, _ Fallback, _ Acceptable) error {
 	return req()
 }
 


Test patch:
diff --git a/core/breaker/googlebreaker_test.go b/core/breaker/googlebreaker_test.go
index 7a8c65b8..85d76660 100644
--- a/core/breaker/googlebreaker_test.go
+++ b/core/breaker/googlebreaker_test.go
@@ -206,7 +206,7 @@ func BenchmarkGoogleBreakerAllow(b *testing.B) {
 	breaker := getGoogleBreaker()
 	b.ResetTimer()
 	for i := 0; i <= b.N; i++ {
-		breaker.accept()
+		_ = breaker.accept()
 		if i%2 == 0 {
 			breaker.markSuccess()
 		} else {
@@ -215,6 +215,16 @@ func BenchmarkGoogleBreakerAllow(b *testing.B) {
 	}
 }
 
+func BenchmarkGoogleBreakerDoReq(b *testing.B) {
+	breaker := getGoogleBreaker()
+	b.ResetTimer()
+	for i := 0; i <= b.N; i++ {
+		_ = breaker.doReq(func() error {
+			return nil
+		}, nil, defaultAcceptable)
+	}
+}
+
 func markSuccess(b *googleBreaker, count int) {
 	for i := 0; i < count; i++ {
 		p, err := b.allow()
diff --git a/core/stores/mon/collection_test.go b/core/stores/mon/collection_test.go
index ac65bbd9..2adc27f0 100644
--- a/core/stores/mon/collection_test.go
+++ b/core/stores/mon/collection_test.go
@@ -603,11 +603,11 @@ func (d *dropBreaker) DoWithAcceptable(_ func() error, _ breaker.Acceptable) err
 	return errDummy
 }
 
-func (d *dropBreaker) DoWithFallback(_ func() error, _ func(err error) error) error {
+func (d *dropBreaker) DoWithFallback(_ func() error, _ breaker.Fallback) error {
 	return nil
 }
 
-func (d *dropBreaker) DoWithFallbackAcceptable(_ func() error, _ func(err error) error,
+func (d *dropBreaker) DoWithFallbackAcceptable(_ func() error, _ breaker.Fallback,
 	_ breaker.Acceptable) error {
 	return nil
 }