Reinforcing errutil and fixed ugly bug which caused empty []error to …

…te treated as error, Previous multi-error implementation could cause very ugly bug of returnig empty multi-error that should be treated as success not error by API, but if .Err() is not invoked it will be used as non nil error. Once we merge this, we can do cleaner solution that slighly change nesting behaviour: #3833 There were 9 places where we had this bug in handler due to MultiError lib allowing to do so. Signed-off-by: Bartlomiej Plotka <[email protected]>
thanos-io · Feb 25, 2021 · 85ccd6a · 85ccd6a
1 parent 46d0106
commit 85ccd6a
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 45 deletions.
diff --git a/pkg/block/fetcher.go b/pkg/block/fetcher.go
@@ -455,7 +455,7 @@ func (f *BaseFetcher) fetch(ctx context.Context, metrics *fetcherMetrics, filter
 	metrics.submit()
 
 	if len(resp.metaErrs) > 0 {
-		return metas, resp.partial, errors.Wrap(resp.metaErrs, "incomplete view")
+		return metas, resp.partial, errors.Wrap(resp.metaErrs.Err(), "incomplete view")
 	}
 
 	level.Info(f.logger).Log("msg", "successfully synchronized block metadata", "duration", time.Since(start).String(), "cached", len(f.cached), "returned", len(metas), "partial", len(resp.partial))

diff --git a/pkg/compact/compact.go b/pkg/compact/compact.go
@@ -543,7 +543,7 @@ func (e HaltError) Error() string {
 // IsHaltError returns true if the base error is a HaltError.
 // If a multierror is passed, any halt error will return true.
 func IsHaltError(err error) bool {
-	if multiErr, ok := errors.Cause(err).(errutil.MultiError); ok {
+	if multiErr, ok := errors.Cause(err).(errutil.NonNilMultiError); ok {
 		for _, err := range multiErr {
 			if _, ok := errors.Cause(err).(HaltError); ok {
 				return true
@@ -576,7 +576,7 @@ func (e RetryError) Error() string {
 // IsRetryError returns true if the base error is a RetryError.
 // If a multierror is passed, all errors must be retriable.
 func IsRetryError(err error) bool {
-	if multiErr, ok := errors.Cause(err).(errutil.MultiError); ok {
+	if multiErr, ok := errors.Cause(err).(errutil.NonNilMultiError); ok {
 		for _, err := range multiErr {
 			if _, ok := errors.Cause(err).(RetryError); !ok {
 				return false

diff --git a/pkg/compact/compact_test.go b/pkg/compact/compact_test.go
@@ -33,11 +33,11 @@ func TestHaltMultiError(t *testing.T) {
 	nonHaltErr := errors.New("not a halt error")
 
 	errs := errutil.MultiError{nonHaltErr}
-	testutil.Assert(t, !IsHaltError(errs), "should not be a halt error")
+	testutil.Assert(t, !IsHaltError(errs.Err()), "should not be a halt error")
 
 	errs.Add(haltErr)
-	testutil.Assert(t, IsHaltError(errs), "if any halt errors are present this should return true")
-	testutil.Assert(t, IsHaltError(errors.Wrap(errs, "wrap")), "halt error with wrap")
+	testutil.Assert(t, IsHaltError(errs.Err()), "if any halt errors are present this should return true")
+	testutil.Assert(t, IsHaltError(errors.Wrap(errs.Err(), "wrap")), "halt error with wrap")
 
 }
 
@@ -46,15 +46,15 @@ func TestRetryMultiError(t *testing.T) {
 	nonRetryErr := errors.New("not a retry error")
 
 	errs := errutil.MultiError{nonRetryErr}
-	testutil.Assert(t, !IsRetryError(errs), "should not be a retry error")
+	testutil.Assert(t, !IsRetryError(errs.Err()), "should not be a retry error")
 
 	errs = errutil.MultiError{retryErr}
-	testutil.Assert(t, IsRetryError(errs), "if all errors are retriable this should return true")
+	testutil.Assert(t, IsRetryError(errs.Err()), "if all errors are retriable this should return true")
 
-	testutil.Assert(t, IsRetryError(errors.Wrap(errs, "wrap")), "retry error with wrap")
+	testutil.Assert(t, IsRetryError(errors.Wrap(errs.Err(), "wrap")), "retry error with wrap")
 
 	errs = errutil.MultiError{nonRetryErr, retryErr}
-	testutil.Assert(t, !IsRetryError(errs), "mixed errors should return false")
+	testutil.Assert(t, !IsRetryError(errs.Err()), "mixed errors should return false")
 }
 
 func TestRetryError(t *testing.T) {

diff --git a/pkg/errutil/multierror.go b/pkg/errutil/multierror.go
@@ -12,30 +12,12 @@ import (
 // Errors used to construct it.
 type MultiError []error
 
-// Returns a concatenated string of the contained errors.
-func (es MultiError) Error() string {
-	var buf bytes.Buffer
-
-	if len(es) > 1 {
-		fmt.Fprintf(&buf, "%d errors: ", len(es))
-	}
-
-	for i, err := range es {
-		if i != 0 {
-			buf.WriteString("; ")
-		}
-		buf.WriteString(err.Error())
-	}
-
-	return buf.String()
-}
-
 // Add adds the error to the error list if it is not nil.
 func (es *MultiError) Add(err error) {
 	if err == nil {
 		return
 	}
-	if merr, ok := err.(MultiError); ok {
+	if merr, ok := err.(NonNilMultiError); ok {
 		*es = append(*es, merr...)
 	} else {
 		*es = append(*es, err)
@@ -47,5 +29,25 @@ func (es MultiError) Err() error {
 	if len(es) == 0 {
 		return nil
 	}
-	return es
+	return NonNilMultiError(es)
+}
+
+type NonNilMultiError MultiError
+
+// Returns a concatenated string of the contained errors.
+func (es NonNilMultiError) Error() string {
+	var buf bytes.Buffer
+
+	if len(es) > 1 {
+		fmt.Fprintf(&buf, "%d errors: ", len(es))
+	}
+
+	for i, err := range es {
+		if i != 0 {
+			buf.WriteString("; ")
+		}
+		buf.WriteString(err.Error())
+	}
+
+	return buf.String()
 }
diff --git a/pkg/receive/handler.go b/pkg/receive/handler.go
@@ -563,7 +563,7 @@ func (h *Handler) fanoutForward(pctx context.Context, tenant string, replicas ma
 			return fctx.Err()
 		case err, more := <-ec:
 			if !more {
-				return errs
+				return errs.Err()
 			}
 			if err == nil {
 				success++
@@ -692,7 +692,7 @@ func determineWriteErrorCause(err error, threshold int) error {
 	}
 
 	unwrappedErr := errors.Cause(err)
-	errs, ok := unwrappedErr.(errutil.MultiError)
+	errs, ok := unwrappedErr.(errutil.NonNilMultiError)
 	if !ok {
 		errs = []error{unwrappedErr}
 	}

diff --git a/pkg/receive/handler_test.go b/pkg/receive/handler_test.go
@@ -44,7 +44,7 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "nil multierror",
-			err:  errutil.MultiError([]error{}),
+			err:  errutil.NonNilMultiError([]error{}),
 		},
 		{
 			name:      "matching simple",
@@ -54,15 +54,15 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "non-matching multierror",
-			err: errutil.MultiError([]error{
+			err: errutil.NonNilMultiError([]error{
 				errors.New("foo"),
 				errors.New("bar"),
 			}),
 			exp: errors.New("2 errors: foo; bar"),
 		},
 		{
 			name: "nested non-matching multierror",
-			err: errors.Wrap(errutil.MultiError([]error{
+			err: errors.Wrap(errutil.NonNilMultiError([]error{
 				errors.New("foo"),
 				errors.New("bar"),
 			}), "baz"),
@@ -71,9 +71,9 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "deep nested non-matching multierror",
-			err: errors.Wrap(errutil.MultiError([]error{
+			err: errors.Wrap(errutil.NonNilMultiError([]error{
 				errors.New("foo"),
-				errutil.MultiError([]error{
+				errutil.NonNilMultiError([]error{
 					errors.New("bar"),
 					errors.New("qux"),
 				}),
@@ -83,7 +83,7 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "matching multierror",
-			err: errutil.MultiError([]error{
+			err: errutil.NonNilMultiError([]error{
 				storage.ErrOutOfOrderSample,
 				errors.New("foo"),
 				errors.New("bar"),
@@ -93,7 +93,7 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "matching but below threshold multierror",
-			err: errutil.MultiError([]error{
+			err: errutil.NonNilMultiError([]error{
 				storage.ErrOutOfOrderSample,
 				errors.New("foo"),
 				errors.New("bar"),
@@ -103,7 +103,7 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "matching multierror many",
-			err: errutil.MultiError([]error{
+			err: errutil.NonNilMultiError([]error{
 				storage.ErrOutOfOrderSample,
 				errConflict,
 				status.Error(codes.AlreadyExists, "conflict"),
@@ -115,7 +115,7 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "matching multierror many, one above threshold",
-			err: errutil.MultiError([]error{
+			err: errutil.NonNilMultiError([]error{
 				storage.ErrOutOfOrderSample,
 				errConflict,
 				tsdb.ErrNotReady,
@@ -128,7 +128,7 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "matching multierror many, both above threshold, conflict have precedence",
-			err: errutil.MultiError([]error{
+			err: errutil.NonNilMultiError([]error{
 				storage.ErrOutOfOrderSample,
 				errConflict,
 				tsdb.ErrNotReady,
@@ -142,7 +142,7 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "nested matching multierror",
-			err: errors.Wrap(errors.Wrap(errutil.MultiError([]error{
+			err: errors.Wrap(errors.Wrap(errutil.NonNilMultiError([]error{
 				storage.ErrOutOfOrderSample,
 				errors.New("foo"),
 				errors.New("bar"),
@@ -152,8 +152,8 @@ func TestDetermineWriteErrorCause(t *testing.T) {
 		},
 		{
 			name: "deep nested matching multierror",
-			err: errors.Wrap(errutil.MultiError([]error{
-				errutil.MultiError([]error{
+			err: errors.Wrap(errutil.NonNilMultiError([]error{
+				errutil.NonNilMultiError([]error{
 					errors.New("qux"),
 					status.Error(codes.AlreadyExists, "conflict"),
 					status.Error(codes.AlreadyExists, "conflict"),