Repository: labstack/echo (go)

Issue:
The 'RouteNotFound' have a bug
### Issue Description

### Checklist

- [ ] Dependencies installed
- [ ] No typos
- [ ] Searched existing issues and docs

### Expected behaviour

### Actual behaviour

### Steps to reproduce

### Working code to debug

```go
package main

import "github.com/labstack/echo/v4"

func main() {
  e := echo.New()
  e.HTTPErrorHandler = HTTPErrorHandler
  
  e.GET("/test1", test1Handler)
  e.GET("/test2", test2Handler)
  e.RouteNotFound("/*", indexHandler)
  
  g := e.Group("/admin")
  g.GET("/test1", adminTest1Handler)
  g.GET("/test2", adminTest2Handler)
  g.RouteNotFound("/*", adminIndexHandler)
}
```

### Version/commit

Get path `/test3` and will goto `indexHandler`, bug get `/admin/test3` goto `HTTPErrorHandler`.  The `/admin/test3` goto `adminIndexHandler` is right.

The `echo` code have some test, but the bug is not checked.

Pull request:
Fix group.RouteNotFound not working when group has attached middlewares
Fix group.RouteNotFound not working when group has attached middlewares.

Problems is/was that `g.Use` registers special catch all routes with `g.Any` and those routes have priority over route registered by `g.NotFoundHandler`. 
Solution is to register these special routes also with `NotFoundHandler` so if you register custom one - it will override special catch all.

Fixes #2401
For history sake: somewhat relates to #1981 , #2256 , #1728

Patch:
diff --git a/group.go b/group.go
index 28ce0dd..749a5ca 100644
--- a/group.go
+++ b/group.go
@@ -23,10 +23,12 @@ func (g *Group) Use(middleware ...MiddlewareFunc) {
 	if len(g.middleware) == 0 {
 		return
 	}
-	// Allow all requests to reach the group as they might get dropped if router
-	// doesn't find a match, making none of the group middleware process.
-	g.Any("", NotFoundHandler)
-	g.Any("/*", NotFoundHandler)
+	// group level middlewares are different from Echo `Pre` and `Use` middlewares (those are global). Group level middlewares
+	// are only executed if they are added to the Router with route.
+	// So we register catch all route (404 is a safe way to emulate route match) for this group and now during routing the
+	// Router would find route to match our request path and therefore guarantee the middleware(s) will get executed.
+	g.RouteNotFound("", NotFoundHandler)
+	g.RouteNotFound("/*", NotFoundHandler)
 }
 
 // CONNECT implements `Echo#CONNECT()` for sub-routes within the Group.
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/group_test.go b/group_test.go
index 01c304d..d22f564 100644
--- a/group_test.go
+++ b/group_test.go
@@ -184,3 +184,73 @@ func TestGroup_RouteNotFound(t *testing.T) {
 		})
 	}
 }
+
+func TestGroup_RouteNotFoundWithMiddleware(t *testing.T) {
+	var testCases = []struct {
+		name           string
+		givenCustom404 bool
+		whenURL        string
+		expectBody     interface{}
+		expectCode     int
+	}{
+		{
+			name:           "ok, custom 404 handler is called with middleware",
+			givenCustom404: true,
+			whenURL:        "/group/test3",
+			expectBody:     "GET /group/*",
+			expectCode:     http.StatusNotFound,
+		},
+		{
+			name:           "ok, default group 404 handler is called with middleware",
+			givenCustom404: false,
+			whenURL:        "/group/test3",
+			expectBody:     "{\"message\":\"Not Found\"}\n",
+			expectCode:     http.StatusNotFound,
+		},
+		{
+			name:           "ok, (no slash) default group 404 handler is called with middleware",
+			givenCustom404: false,
+			whenURL:        "/group",
+			expectBody:     "{\"message\":\"Not Found\"}\n",
+			expectCode:     http.StatusNotFound,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+
+			okHandler := func(c Context) error {
+				return c.String(http.StatusOK, c.Request().Method+" "+c.Path())
+			}
+			notFoundHandler := func(c Context) error {
+				return c.String(http.StatusNotFound, c.Request().Method+" "+c.Path())
+			}
+
+			e := New()
+			e.GET("/test1", okHandler)
+			e.RouteNotFound("/*", notFoundHandler)
+
+			g := e.Group("/group")
+			g.GET("/test1", okHandler)
+
+			middlewareCalled := false
+			g.Use(func(next HandlerFunc) HandlerFunc {
+				return func(c Context) error {
+					middlewareCalled = true
+					return next(c)
+				}
+			})
+			if tc.givenCustom404 {
+				g.RouteNotFound("/*", notFoundHandler)
+			}
+
+			req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil)
+			rec := httptest.NewRecorder()
+
+			e.ServeHTTP(rec, req)
+
+			assert.True(t, middlewareCalled)
+			assert.Equal(t, tc.expectCode, rec.Code)
+			assert.Equal(t, tc.expectBody, rec.Body.String())
+		})
+	}
+}
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")
+}