changed logic to respond with '500 Internal Server Error' if rpc erro…

…r with code 'Cancelled' happened. it's made to allow loki to retry the request to the querier instead of returning '499 The request was cancelled by the client.' back to the client. (grafana#5297)
KMiller-Grafana · Feb 4, 2022 · dfc89d2 · dfc89d2
1 parent 8a59aa6
commit dfc89d2
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 29 deletions.
diff --git a/pkg/util/errors.go b/pkg/util/errors.go
@@ -71,27 +71,13 @@ func (es MultiError) Err() error {
 
 // Is tells if all errors are the same as the target error.
 func (es MultiError) Is(target error) bool {
-	for _, err := range es {
-		if !errors.Is(err, target) {
-			return false
-		}
-	}
-	return true
-}
-
-// IsCancel tells if all errors are either context.Canceled or grpc codes.Canceled.
-func (es MultiError) IsCancel() bool {
 	if len(es) == 0 {
 		return false
 	}
 	for _, err := range es {
-		if errors.Is(err, context.Canceled) {
-			continue
-		}
-		if IsConnCanceled(err) {
-			continue
+		if !errors.Is(err, target) {
+			return false
 		}
-		return false
 	}
 	return true
 }

diff --git a/pkg/util/server/error.go b/pkg/util/server/error.go
@@ -56,7 +56,7 @@ func WriteError(err error, w http.ResponseWriter) {
 	)
 
 	me, ok := err.(util.MultiError)
-	if ok && me.IsCancel() {
+	if ok && me.Is(context.Canceled) {
 		JSONError(w, StatusClientClosedRequest, ErrClientCanceled)
 		return
 	}
@@ -68,7 +68,6 @@ func WriteError(err error, w http.ResponseWriter) {
 	s, isRPC := status.FromError(err)
 	switch {
 	case errors.Is(err, context.Canceled) ||
-		(isRPC && s.Code() == codes.Canceled) ||
 		(errors.As(err, &promErr) && errors.Is(promErr.Err, context.Canceled)):
 		JSONError(w, StatusClientClosedRequest, ErrClientCanceled)
 	case errors.Is(err, context.DeadlineExceeded) ||

diff --git a/pkg/util/server/error_test.go b/pkg/util/server/error_test.go
@@ -27,29 +27,57 @@ func Test_writeError(t *testing.T) {
 		name string
 
 		err            error
-		msg            string
+		expectedMsg    string
 		expectedStatus int
 	}{
 		{"cancelled", context.Canceled, ErrClientCanceled, StatusClientClosedRequest},
 		{"cancelled multi", util.MultiError{context.Canceled, context.Canceled}, ErrClientCanceled, StatusClientClosedRequest},
-		{"rpc cancelled", status.New(codes.Canceled, context.Canceled.Error()).Err(), ErrClientCanceled, StatusClientClosedRequest},
-		{"rpc cancelled multi", util.MultiError{status.New(codes.Canceled, context.Canceled.Error()).Err(), status.New(codes.Canceled, context.Canceled.Error()).Err()}, ErrClientCanceled, StatusClientClosedRequest},
-		{"mixed context and rpc cancelled", util.MultiError{context.Canceled, status.New(codes.Canceled, context.Canceled.Error()).Err()}, ErrClientCanceled, StatusClientClosedRequest},
-		{"mixed context, rpc cancelled and another", util.MultiError{errors.New("standard error"), context.Canceled, status.New(codes.Canceled, context.Canceled.Error()).Err()}, "3 errors: standard error; context canceled; rpc error: code = Canceled desc = context canceled", http.StatusInternalServerError},
+		{"rpc cancelled",
+			status.New(codes.Canceled, context.Canceled.Error()).Err(),
+			"rpc error: code = Canceled desc = context canceled",
+			http.StatusInternalServerError},
+		{"rpc cancelled multi",
+			util.MultiError{status.New(codes.Canceled, context.Canceled.Error()).Err(), status.New(codes.Canceled, context.Canceled.Error()).Err()},
+			"2 errors: rpc error: code = Canceled desc = context canceled; rpc error: code = Canceled desc = context canceled",
+			http.StatusInternalServerError},
+		{"mixed context and rpc cancelled",
+			util.MultiError{context.Canceled, status.New(codes.Canceled, context.Canceled.Error()).Err()},
+			"2 errors: context canceled; rpc error: code = Canceled desc = context canceled",
+			http.StatusInternalServerError},
+		{"mixed context, rpc cancelled and another",
+			util.MultiError{errors.New("standard error"), context.Canceled, status.New(codes.Canceled, context.Canceled.Error()).Err()},
+			"3 errors: standard error; context canceled; rpc error: code = Canceled desc = context canceled",
+			http.StatusInternalServerError},
 		{"cancelled storage", promql.ErrStorage{Err: context.Canceled}, ErrClientCanceled, StatusClientClosedRequest},
 		{"orgid", user.ErrNoOrgID, user.ErrNoOrgID.Error(), http.StatusBadRequest},
 		{"deadline", context.DeadlineExceeded, ErrDeadlineExceeded, http.StatusGatewayTimeout},
 		{"deadline multi", util.MultiError{context.DeadlineExceeded, context.DeadlineExceeded}, ErrDeadlineExceeded, http.StatusGatewayTimeout},
 		{"rpc deadline", status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()).Err(), ErrDeadlineExceeded, http.StatusGatewayTimeout},
-		{"rpc deadline multi", util.MultiError{status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()).Err(), status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()).Err()}, ErrDeadlineExceeded, http.StatusGatewayTimeout},
-		{"mixed context and rpc deadline", util.MultiError{context.DeadlineExceeded, status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()).Err()}, ErrDeadlineExceeded, http.StatusGatewayTimeout},
-		{"mixed context, rpc deadline and another", util.MultiError{errors.New("standard error"), context.DeadlineExceeded, status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()).Err()}, "3 errors: standard error; context deadline exceeded; rpc error: code = DeadlineExceeded desc = context deadline exceeded", http.StatusInternalServerError},
+		{"rpc deadline multi",
+			util.MultiError{status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()).Err(),
+				status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()).Err()},
+			ErrDeadlineExceeded,
+			http.StatusGatewayTimeout},
+		{"mixed context and rpc deadline",
+			util.MultiError{context.DeadlineExceeded, status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()).Err()},
+			ErrDeadlineExceeded,
+			http.StatusGatewayTimeout},
+		{"mixed context, rpc deadline and another",
+			util.MultiError{errors.New("standard error"), context.DeadlineExceeded, status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()).Err()},
+			"3 errors: standard error; context deadline exceeded; rpc error: code = DeadlineExceeded desc = context deadline exceeded",
+			http.StatusInternalServerError},
 		{"parse error", logqlmodel.ParseError{}, "parse error : ", http.StatusBadRequest},
 		{"httpgrpc", httpgrpc.Errorf(http.StatusBadRequest, errors.New("foo").Error()), "foo", http.StatusBadRequest},
 		{"internal", errors.New("foo"), "foo", http.StatusInternalServerError},
 		{"query error", chunk.ErrQueryMustContainMetricName, chunk.ErrQueryMustContainMetricName.Error(), http.StatusBadRequest},
-		{"wrapped query error", fmt.Errorf("wrapped: %w", chunk.ErrQueryMustContainMetricName), "wrapped: " + chunk.ErrQueryMustContainMetricName.Error(), http.StatusBadRequest},
-		{"multi mixed", util.MultiError{context.Canceled, context.DeadlineExceeded}, "2 errors: context canceled; context deadline exceeded", http.StatusInternalServerError},
+		{"wrapped query error",
+			fmt.Errorf("wrapped: %w", chunk.ErrQueryMustContainMetricName),
+			"wrapped: " + chunk.ErrQueryMustContainMetricName.Error(),
+			http.StatusBadRequest},
+		{"multi mixed",
+			util.MultiError{context.Canceled, context.DeadlineExceeded},
+			"2 errors: context canceled; context deadline exceeded",
+			http.StatusInternalServerError},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			rec := httptest.NewRecorder()
@@ -58,7 +86,7 @@ func Test_writeError(t *testing.T) {
 			json.NewDecoder(rec.Result().Body).Decode(res)
 			require.Equal(t, tt.expectedStatus, res.Code)
 			require.Equal(t, tt.expectedStatus, rec.Result().StatusCode)
-			require.Equal(t, tt.msg, res.Message)
+			require.Equal(t, tt.expectedMsg, res.Message)
 		})
 	}
 }