Skip to content

Commit

Permalink
Reinforcing errutil and fixed ugly bug which caused empty []error to …
Browse files Browse the repository at this point in the history 
…te treated as error, (#3836)

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 <bwplotka@gmail.com>
  • Loading branch information
@bwplotka
bwplotka committed Feb 26, 2021
1 parent b2137b1 commit 2027fb3
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 45 deletions.
2 changes: 1 addition & 1 deletion pkg/block/fetcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
4 changes: 2 additions & 2 deletions pkg/compact/compact.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
14 changes: 7 additions & 7 deletions pkg/compact/compact_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")

}

Expand All @@ -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) {
Expand Down
42 changes: 22 additions & 20 deletions pkg/errutil/multierror.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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()
}
4 changes: 2 additions & 2 deletions pkg/receive/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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++
Expand Down Expand Up @@ -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}
}
Expand Down
26 changes: 13 additions & 13 deletions pkg/receive/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestDetermineWriteErrorCause(t *testing.T) {
},
{
name: "nil multierror",
err: errutil.MultiError([]error{}),
err: errutil.NonNilMultiError([]error{}),
},
{
name: "matching simple",
Expand All @@ -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"),
Expand All @@ -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"),
}),
Expand All @@ -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"),
Expand All @@ -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"),
Expand All @@ -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"),
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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"),
Expand All @@ -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"),
Expand Down

0 comments on commit 2027fb3

Please sign in to comment.