Repository: labstack/echo
Pull request #2409 (2023-02-23 10:12:50)

Fixes the concurrency issue of calling the `Next()` proxy target on RRB
Round Robin Balancer (RRB) `Next()` implementation did not properly use synchronization mechanisms to ensure right values are visible between concurrently executed code entering the same critical section.

Previous use of an atomic add to update the index value without also using an atomic load to read it is incorrect use of atomic synchronization (stale values are read in go-routines).

The index value, obviously, depends on the size of the `targets` slice. 
If between index calculation and getting a value by index from the slice a target was removed and the index pointed to the last element then a panic due to out of bounds will be the result. 

Hence, the logic must be guarded with the semaphore.

codecov[bot]: # [Codecov](https://codecov.io/gh/labstack/echo/pull/2409?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) Report
Base: **92.84**% // Head: **92.85**% // Increases project coverage by **`+0.00%`** :tada:
> Coverage data is based on head [(`dae2ac7`)](https://codecov.io/gh/labstack/echo/pull/2409?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) compared to base [(`47844c9`)](https://codecov.io/gh/labstack/echo/commit/47844c9b7f83e5bf4efbe1f449bf2a155f465da8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack).
> Patch coverage: 100.00% of modified lines in pull request are covered.

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


```diff
@@           Coverage Diff           @@
##           master    #2409   +/-   ##
=======================================
  Coverage   92.84%   92.85%           
=======================================
  Files          39       39           
  Lines        4503     4508    +5     
=======================================
+ Hits         4181     4186    +5     
  Misses        234      234           
  Partials       88       88           
```


| [Impacted Files](https://codecov.io/gh/labstack/echo/pull/2409?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) | Coverage Δ | |
|---|---|---|
| [middleware/proxy.go](https://codecov.io/gh/labstack/echo/pull/2409?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack#diff-bWlkZGxld2FyZS9wcm94eS5nbw==) | `69.67% <100.00%> (+1.01%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/labstack/echo/pull/2409?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack).

aldas: @bbasic thanks. would it be too much to ask to tweak some other things there also?

1) wider mutex scope in `(b *commonBalancer) AddTarge` method
2) probably `b.random = rand.New(rand.NewSource(int64(time.Now().Nanosecond())))` could be initialized in `func NewRandomBalancer`

Patch:
diff --git a/middleware/proxy.go b/middleware/proxy.go
index d2cd2aa..74f49de 100644
--- a/middleware/proxy.go
+++ b/middleware/proxy.go
@@ -12,7 +12,6 @@ import (
 	"regexp"
 	"strings"
 	"sync"
-	"sync/atomic"
 	"time"
 
 	"github.com/labstack/echo/v4"
@@ -79,19 +78,20 @@ type (
 
 	commonBalancer struct {
 		targets []*ProxyTarget
-		mutex   sync.RWMutex
+		mutex   sync.Mutex
 	}
 
 	// RandomBalancer implements a random load balancing technique.
 	randomBalancer struct {
-		*commonBalancer
+		commonBalancer
 		random *rand.Rand
 	}
 
 	// RoundRobinBalancer implements a round-robin load balancing technique.
 	roundRobinBalancer struct {
-		*commonBalancer
-		i uint32
+		commonBalancer
+		// tracking the index on `targets` slice for the next `*ProxyTarget` to be used
+		i int
 	}
 )
 
@@ -143,32 +143,37 @@ func proxyRaw(t *ProxyTarget, c echo.Context) http.Handler {
 
 // NewRandomBalancer returns a random proxy balancer.
 func NewRandomBalancer(targets []*ProxyTarget) ProxyBalancer {
-	b := &randomBalancer{commonBalancer: new(commonBalancer)}
+	b := randomBalancer{}
 	b.targets = targets
-	return b
+	b.random = rand.New(rand.NewSource(int64(time.Now().Nanosecond())))
+	return &b
 }
 
 // NewRoundRobinBalancer returns a round-robin proxy balancer.
 func NewRoundRobinBalancer(targets []*ProxyTarget) ProxyBalancer {
-	b := &roundRobinBalancer{commonBalancer: new(commonBalancer)}
+	b := roundRobinBalancer{}
 	b.targets = targets
-	return b
+	return &b
 }
 
-// AddTarget adds an upstream target to the list.
+// AddTarget adds an upstream target to the list and returns `true`.
+//
+// However, if a target with the same name already exists then the operation is aborted returning `false`.
 func (b *commonBalancer) AddTarget(target *ProxyTarget) bool {
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
 	for _, t := range b.targets {
 		if t.Name == target.Name {
 			return false
 		}
 	}
-	b.mutex.Lock()
-	defer b.mutex.Unlock()
 	b.targets = append(b.targets, target)
 	return true
 }
 
-// RemoveTarget removes an upstream target from the list.
+// RemoveTarget removes an upstream target from the list by name.
+//
+// Returns `true` on success, `false` if no target with the name is found.
 func (b *commonBalancer) RemoveTarget(name string) bool {
 	b.mutex.Lock()
 	defer b.mutex.Unlock()
@@ -182,20 +187,36 @@ func (b *commonBalancer) RemoveTarget(name string) bool {
 }
 
 // Next randomly returns an upstream target.
+//
+// Note: `nil` is returned in case upstream target list is empty.
 func (b *randomBalancer) Next(c echo.Context) *ProxyTarget {
-	if b.random == nil {
-		b.random = rand.New(rand.NewSource(int64(time.Now().Nanosecond())))
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+	if len(b.targets) == 0 {
+		return nil
+	} else if len(b.targets) == 1 {
+		return b.targets[0]
 	}
-	b.mutex.RLock()
-	defer b.mutex.RUnlock()
 	return b.targets[b.random.Intn(len(b.targets))]
 }
 
 // Next returns an upstream target using round-robin technique.
+//
+// Note: `nil` is returned in case upstream target list is empty.
 func (b *roundRobinBalancer) Next(c echo.Context) *ProxyTarget {
-	b.i = b.i % uint32(len(b.targets))
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+	if len(b.targets) == 0 {
+		return nil
+	} else if len(b.targets) == 1 {
+		return b.targets[0]
+	}
+	// reset the index if out of bounds
+	if b.i >= len(b.targets) {
+		b.i = 0
+	}
 	t := b.targets[b.i]
-	atomic.AddUint32(&b.i, 1)
+	b.i++
 	return t
 }
 
diff --git a/middleware/recover.go b/middleware/recover.go
index 7b61285..36d41aa 100644
--- a/middleware/recover.go
+++ b/middleware/recover.go
@@ -38,6 +38,11 @@ type (
 		// LogErrorFunc defines a function for custom logging in the middleware.
 		// If it's set you don't need to provide LogLevel for config.
 		LogErrorFunc LogErrorFunc
+
+		// DisableErrorHandler disables the call to centralized HTTPErrorHandler.
+		// The recovered error is then passed back to upstream middleware, instead of swallowing the error.
+		// Optional. Default value false.
+		DisableErrorHandler bool `yaml:"disable_error_handler"`
 	}
 )
 
@@ -50,6 +55,7 @@ var (
 		DisablePrintStack: false,
 		LogLevel:          0,
 		LogErrorFunc:      nil,
+		DisableErrorHandler: false,
 	}
 )
 
@@ -71,7 +77,7 @@ func RecoverWithConfig(config RecoverConfig) echo.MiddlewareFunc {
 	}
 
 	return func(next echo.HandlerFunc) echo.HandlerFunc {
-		return func(c echo.Context) error {
+		return func(c echo.Context) (returnErr error) {
 			if config.Skipper(c) {
 				return next(c)
 			}
@@ -113,7 +119,12 @@ func RecoverWithConfig(config RecoverConfig) echo.MiddlewareFunc {
 							c.Logger().Print(msg)
 						}
 					}
-					c.Error(err)
+
+					if(!config.DisableErrorHandler) {
+						c.Error(err)
+					} else {
+						returnErr = err
+					}
 				}
 			}()
 			return next(c)


Test patch:
diff --git a/middleware/proxy_test.go b/middleware/proxy_test.go
index a1b7f2c..122ddde 100644
--- a/middleware/proxy_test.go
+++ b/middleware/proxy_test.go
@@ -122,7 +122,7 @@ func TestProxy(t *testing.T) {
 }
 
 type testProvider struct {
-	*commonBalancer
+	commonBalancer
 	target *ProxyTarget
 	err    error
 }
@@ -143,7 +143,7 @@ func TestTargetProvider(t *testing.T) {
 	url1, _ := url.Parse(t1.URL)
 
 	e := echo.New()
-	tp := &testProvider{commonBalancer: new(commonBalancer)}
+	tp := &testProvider{}
 	tp.target = &ProxyTarget{Name: "target 1", URL: url1}
 	e.Use(Proxy(tp))
 	rec := httptest.NewRecorder()
@@ -158,7 +158,7 @@ func TestFailNextTarget(t *testing.T) {
 	assert.Nil(t, err)
 
 	e := echo.New()
-	tp := &testProvider{commonBalancer: new(commonBalancer)}
+	tp := &testProvider{}
 	tp.target = &ProxyTarget{Name: "target 1", URL: url1}
 	tp.err = echo.NewHTTPError(http.StatusInternalServerError, "method could not select target")
 
@@ -422,3 +422,12 @@ func TestClientCancelConnectionResultsHTTPCode499(t *testing.T) {
 	timeoutStop.Done()
 	assert.Equal(t, 499, rec.Code)
 }
+
+// Assert balancer with empty targets does return `nil` on `Next()`
+func TestProxyBalancerWithNoTargets(t *testing.T) {
+	rb := NewRandomBalancer(nil)
+	assert.Nil(t, rb.Next(nil))
+
+	rrb := NewRoundRobinBalancer([]*ProxyTarget{})
+	assert.Nil(t, rrb.Next(nil))
+}
diff --git a/middleware/recover_test.go b/middleware/recover_test.go
index b27f3b4..3e0d35d 100644
--- a/middleware/recover_test.go
+++ b/middleware/recover_test.go
@@ -23,7 +23,8 @@ func TestRecover(t *testing.T) {
 	h := Recover()(echo.HandlerFunc(func(c echo.Context) error {
 		panic("test")
 	}))
-	h(c)
+	err := h(c)
+	assert.NoError(t, err)
 	assert.Equal(t, http.StatusInternalServerError, rec.Code)
 	assert.Contains(t, buf.String(), "PANIC RECOVER")
 }
@@ -163,3 +164,23 @@ func TestRecoverWithConfig_LogErrorFunc(t *testing.T) {
 		assert.Contains(t, output, `"level":"ERROR"`)
 	})
 }
+
+func TestRecoverWithDisabled_ErrorHandler(t *testing.T) {
+	e := echo.New()
+	buf := new(bytes.Buffer)
+	e.Logger.SetOutput(buf)
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	rec := httptest.NewRecorder()
+	c := e.NewContext(req, rec)
+
+	config := DefaultRecoverConfig
+	config.DisableErrorHandler = true
+	h := RecoverWithConfig(config)(echo.HandlerFunc(func(c echo.Context) error {
+		panic("test")
+	}))
+	err := h(c)
+
+	assert.Equal(t, http.StatusOK, rec.Code)
+	assert.Contains(t, buf.String(), "PANIC RECOVER")
+	assert.EqualError(t, err, "test")
+}