Avoid context canceled errors (#1789)

* Avoid context canceled errors

Return 499 Client Closed Request when the client has closed the request before the server could send a response

Signed-off-by: Seena Fallah <seenafallah@gmail.com>

middleware/proxy_1_11.go

@@ -3,21 +3,43 @@
 package middleware
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"net/http/httputil"
+	"strings"
 
 	"github.com/labstack/echo/v4"
 )
 
+// StatusCodeContextCanceled is a custom HTTP status code for situations
+// where a client unexpectedly closed the connection to the server.
+// As there is no standard error code for "client closed connection", but
+// various well-known HTTP clients and server implement this HTTP code we use
+// 499 too instead of the more problematic 5xx, which does not allow to detect this situation
+const StatusCodeContextCanceled = 499
+
 func proxyHTTP(tgt *ProxyTarget, c echo.Context, config ProxyConfig) http.Handler {
 	proxy := httputil.NewSingleHostReverseProxy(tgt.URL)
 	proxy.ErrorHandler = func(resp http.ResponseWriter, req *http.Request, err error) {
 		desc := tgt.URL.String()
 		if tgt.Name != "" {
 			desc = fmt.Sprintf("%s(%s)", tgt.Name, tgt.URL.String())
 		}
-		c.Set("_error", echo.NewHTTPError(http.StatusBadGateway, fmt.Sprintf("remote %s unreachable, could not forward: %v", desc, err)))
+		// If the client canceled the request (usually by closing the connection), we can report a
+		// client error (4xx) instead of a server error (5xx) to correctly identify the situation.
+		// The Go standard library (at of late 2020) wraps the exported, standard
+		// context.Canceled error with unexported garbage value requiring a substring check, see
+		// https://github.com/golang/go/blob/6965b01ea248cabb70c3749fd218b36089a21efb/src/net/net.go#L416-L430
+		if err == context.Canceled || strings.Contains(err.Error(), "operation was canceled") {
+			httpError := echo.NewHTTPError(StatusCodeContextCanceled, fmt.Sprintf("client closed connection: %v", err))
+			httpError.Internal = err
+			c.Set("_error", httpError)
+		} else {
+			httpError := echo.NewHTTPError(http.StatusBadGateway, fmt.Sprintf("remote %s unreachable, could not forward: %v", desc, err))
+			httpError.Internal = err
+			c.Set("_error", httpError)
+		}
 	}
 	proxy.Transport = config.Transport
 	proxy.ModifyResponse = config.ModifyResponse

middleware/proxy_1_11_test.go

@@ -3,10 +3,13 @@
 package middleware
 
 import (
+	"context"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"sync"
 	"testing"
+	"time"
 
 	"github.com/labstack/echo/v4"
 	"github.com/stretchr/testify/assert"
@@ -51,3 +54,33 @@ func TestProxy_1_11(t *testing.T) {
 	assert.Equal(t, "/api/users", req.URL.Path)
 	assert.Equal(t, http.StatusBadGateway, rec.Code)
 }
+
+func TestClientCancelConnectionResultsHTTPCode499(t *testing.T) {
+	var timeoutStop sync.WaitGroup
+	timeoutStop.Add(1)
+	HTTPTarget := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		timeoutStop.Wait() // wait until we have canceled the request
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer HTTPTarget.Close()
+	targetURL, _ := url.Parse(HTTPTarget.URL)
+	target := &ProxyTarget{
+		Name: "target",
+		URL:  targetURL,
+	}
+	rb := NewRandomBalancer(nil)
+	assert.True(t, rb.AddTarget(target))
+	e := echo.New()
+	e.Use(Proxy(rb))
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	ctx, cancel := context.WithCancel(req.Context())
+	req = req.WithContext(ctx)
+	go func() {
+		time.Sleep(10 * time.Millisecond)
+		cancel()
+	}()
+	e.ServeHTTP(rec, req)
+	timeoutStop.Done()
+	assert.Equal(t, 499, rec.Code)
+}