From 6061a6bdade50a04d394e7ce95b136508dc78cc0 Mon Sep 17 00:00:00 2001 From: Bartlomiej Plotka Date: Thu, 25 Feb 2021 17:47:57 +0100 Subject: [PATCH] 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: https://github.com/thanos-io/thanos/pull/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 --- pkg/block/fetcher.go | 2 +- pkg/compact/compact.go | 4 ++-- pkg/errutil/multierror.go | 42 +++++++++++++++++++------------------ pkg/receive/handler.go | 4 ++-- pkg/receive/handler_test.go | 26 +++++++++++------------ 5 files changed, 40 insertions(+), 38 deletions(-) diff --git a/pkg/block/fetcher.go b/pkg/block/fetcher.go index fd0abe3b489..c0e04dbe8e3 100644 --- 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 index e27478deda8..18d92080a65 100644 --- 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.NonNilError); 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.NonNilError); ok { for _, err := range multiErr { if _, ok := errors.Cause(err).(RetryError); !ok { return false diff --git a/pkg/errutil/multierror.go b/pkg/errutil/multierror.go index e51bf4554e9..a6d5438d934 100644 --- 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.(NonNilError); 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 NonNilError(es) +} + +type NonNilError MultiError + +// Returns a concatenated string of the contained errors. +func (es NonNilError) 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 index 392ecabde1e..d8b9078819d 100644 --- 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.NonNilError) if !ok { errs = []error{unwrappedErr} } diff --git a/pkg/receive/handler_test.go b/pkg/receive/handler_test.go index 4a06fdd2865..012c83f457a 100644 --- 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.NonNilError([]error{}), }, { name: "matching simple", @@ -54,7 +54,7 @@ func TestDetermineWriteErrorCause(t *testing.T) { }, { name: "non-matching multierror", - err: errutil.MultiError([]error{ + err: errutil.NonNilError([]error{ errors.New("foo"), errors.New("bar"), }), @@ -62,7 +62,7 @@ func TestDetermineWriteErrorCause(t *testing.T) { }, { name: "nested non-matching multierror", - err: errors.Wrap(errutil.MultiError([]error{ + err: errors.Wrap(errutil.NonNilError([]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.NonNilError([]error{ errors.New("foo"), - errutil.MultiError([]error{ + errutil.NonNilError([]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.NonNilError([]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.NonNilError([]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.NonNilError([]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.NonNilError([]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.NonNilError([]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.NonNilError([]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.NonNilError([]error{ + errutil.NonNilError([]error{ errors.New("qux"), status.Error(codes.AlreadyExists, "conflict"), status.Error(codes.AlreadyExists, "conflict"),