diff --git a/adaptive.go b/adaptive.go index 7124e07..d578b4d 100644 --- a/adaptive.go +++ b/adaptive.go @@ -152,14 +152,18 @@ func (t *AdaptiveThrottle) Throttle( err = fn(ctx) now = t.nowTime() + var re errRejected switch { case err == nil: t.accept(priority, now) - case errors.Is(err, errRejected{}): - // Unwrap error to return the original error to the caller - err = err.(errRejected).inner - - fallthrough + case errors.As(err, &re): + // Explicitly marked as a rejection via RejectedError. Detect the + // wrapper by type rather than value: errors.Is(err, errRejected{}) + // would pass a nil-inner target whose Error() panics when a chain + // link's Is method inspects it. Unwrap to return the original error + // to the caller. + err = re.inner + t.reject(priority, now) case t.checkIsRejected(err): t.reject(priority, now) default: @@ -395,14 +399,18 @@ func Throttle[T any]( res, err = throttledFn(ctx) now = at.nowTime() + var re errRejected switch { case err == nil: at.accept(priority, now) - case errors.Is(err, errRejected{}): - // Unwrap error to return the original error to the caller - err = err.(errRejected).inner - - fallthrough + case errors.As(err, &re): + // Explicitly marked as a rejection via RejectedError. Detect the + // wrapper by type rather than value: errors.Is(err, errRejected{}) + // would pass a nil-inner target whose Error() panics when a chain + // link's Is method inspects it. Unwrap to return the original error + // to the caller. + err = re.inner + at.reject(priority, now) case at.checkIsRejected(err): at.reject(priority, now) default: @@ -453,14 +461,18 @@ func WithAdaptiveThrottle[T any]( res, err = throttledFn() now = at.nowTime() + var re errRejected switch { case err == nil: at.accept(priority, now) - case errors.Is(err, errRejected{}): - // Unwrap error to return the original error to the caller - err = err.(errRejected).inner - - fallthrough + case errors.As(err, &re): + // Explicitly marked as a rejection via RejectedError. Detect the + // wrapper by type rather than value: errors.Is(err, errRejected{}) + // would pass a nil-inner target whose Error() panics when a chain + // link's Is method inspects it. Unwrap to return the original error + // to the caller. + err = re.inner + at.reject(priority, now) case at.checkIsRejected(err): at.reject(priority, now) default: @@ -476,8 +488,19 @@ func WithAdaptiveThrottle[T any]( // Any error that indicates that the backend is unhealthy should be wrapped with // `RejectedError`. But other errors, such as bad requests, authentication failures, // pre-condition failures, etc., should not be wrapped with `RejectedError`. -func RejectedError(err error) error { return errRejected{inner: err} } +// +// A nil error is returned unchanged: wrapping "no error" as a rejection is +// meaningless. This also keeps errRejected's invariant that inner is never nil. +func RejectedError(err error) error { + if err == nil { + return nil + } + + return errRejected{inner: err} +} +// errRejected marks an error as a rejection. inner is never nil: it is only +// constructed by RejectedError, which returns nil for a nil error. type errRejected struct{ inner error } func (err errRejected) Error() string { return err.inner.Error() } diff --git a/adaptive_test.go b/adaptive_test.go index 1852650..d69ed7d 100644 --- a/adaptive_test.go +++ b/adaptive_test.go @@ -564,3 +564,50 @@ func (alwaysShed) Uint64() uint64 { return 0 } type neverShed struct{} func (neverShed) Uint64() uint64 { return math.MaxUint64 } + +// msgComparingErr mimics error types (common in third-party libraries) whose Is +// method inspects the target via Error(). Such a type, in the returned error's +// chain, used to detonate the value-based errors.Is(err, errRejected{}) guard by +// calling errRejected{}.Error() on a nil inner. +type msgComparingErr struct{ message string } + +func (e *msgComparingErr) Error() string { return e.message } +func (e *msgComparingErr) Is(target error) bool { return e.message == target.Error() } + +// TestWithAdaptiveThrottleMsgComparingErrorDoesNotPanic verifies that an error +// whose Is method inspects the target is classified without panicking, and is +// returned to the caller unchanged (a bare error is not a rejection). +func TestWithAdaptiveThrottleMsgComparingErrorDoesNotPanic(t *testing.T) { + at := NewAdaptiveThrottle(4) + in := &msgComparingErr{message: "boom"} + _, err := WithAdaptiveThrottle(at, High, func() (int, error) { return 0, in }) + if !errors.Is(err, in) { + t.Fatalf("got %v; want original error returned unchanged", err) + } +} + +// TestWithAdaptiveThrottleRejectedMsgComparingErrorUnwraps verifies that such an +// error wrapped in RejectedError is classified as a rejection and unwrapped back +// to the original error for the caller. +func TestWithAdaptiveThrottleRejectedMsgComparingErrorUnwraps(t *testing.T) { + at := NewAdaptiveThrottle(4) + in := &msgComparingErr{message: "backend down"} + _, err := WithAdaptiveThrottle(at, High, func() (int, error) { return 0, RejectedError(in) }) + if err != in { + t.Fatalf("got %v; want unwrapped original error", err) + } +} + +// TestRejectedErrorNil verifies that RejectedError(nil) returns nil, so a +// successful call is never accounted as a rejection. +func TestRejectedErrorNil(t *testing.T) { + if err := RejectedError(nil); err != nil { + t.Fatalf("RejectedError(nil) = %v; want nil", err) + } + + at := NewAdaptiveThrottle(4) + res, err := WithAdaptiveThrottle(at, High, func() (int, error) { return 42, RejectedError(nil) }) + if err != nil || res != 42 { + t.Fatalf("got (res=%d, err=%v); want (42, nil)", res, err) + } +} diff --git a/v2/adaptive.go b/v2/adaptive.go index 53218f8..a34de57 100644 --- a/v2/adaptive.go +++ b/v2/adaptive.go @@ -349,16 +349,17 @@ func Throttle[T any]( res, err = throttledFn(ctx) now = at.now() - switch { + switch re, rejected := errors.AsType[errRejected](err); { + case rejected: + // Explicitly marked as a rejection via RejectedError. Detect the + // wrapper by type rather than value: errors.Is(err, errRejected{}) + // would pass a nil-inner target whose Error() panics when a chain + // link's Is method inspects it. Unwrap to return the original error + // to the caller. + err = re.inner + at.reject(priority, now) case err == nil: at.accept(priority, now) - case errors.Is(err, errRejected{}): - // Unwrap error to return the original error to the caller. - if re, ok := errors.AsType[errRejected](err); ok { - err = re.inner - } - - fallthrough case at.isRejectedError(err): at.reject(priority, now) default: @@ -378,8 +379,19 @@ func Throttle[T any]( // Any error that indicates that the backend is unhealthy should be wrapped with // `RejectedError`. But other errors, such as bad requests, authentication failures, // pre-condition failures, etc., should not be wrapped with `RejectedError`. -func RejectedError(err error) error { return errRejected{inner: err} } +// +// A nil error is returned unchanged: wrapping "no error" as a rejection is +// meaningless. This also keeps errRejected's invariant that inner is never nil. +func RejectedError(err error) error { + if err == nil { + return nil + } + + return errRejected{inner: err} +} +// errRejected marks an error as a rejection. inner is never nil: it is only +// constructed by RejectedError, which returns nil for a nil error. type errRejected struct{ inner error } func (err errRejected) Error() string { return err.inner.Error() } @@ -390,7 +402,6 @@ func (err errRejected) Is(target error) bool { return ok } - func clamp(lo, x, hi float64) float64 { return max(lo, min(x, hi)) } type ( diff --git a/v2/adaptive_test.go b/v2/adaptive_test.go index 9fa41bd..09622f5 100644 --- a/v2/adaptive_test.go +++ b/v2/adaptive_test.go @@ -564,3 +564,56 @@ func (alwaysShed) Uint64() uint64 { return 0 } type neverShed struct{} func (neverShed) Uint64() uint64 { return math.MaxUint64 } + +// msgComparingErr mimics error types (common in third-party libraries) whose Is +// method inspects the target via Error(). Such a type, in the returned error's +// chain, used to detonate the value-based errors.Is(err, errRejected{}) guard by +// calling errRejected{}.Error() on a nil inner. +type msgComparingErr struct{ message string } + +func (e *msgComparingErr) Error() string { return e.message } +func (e *msgComparingErr) Is(target error) bool { return e.message == target.Error() } + +// TestThrottleMsgComparingErrorDoesNotPanic verifies that an error whose Is +// method inspects the target is classified without panicking, and is returned to +// the caller unchanged (a bare error is not a rejection). +func TestThrottleMsgComparingErrorDoesNotPanic(t *testing.T) { + at := NewAdaptiveThrottle(4) + in := &msgComparingErr{message: "boom"} + _, err := Throttle(context.Background(), at, High, + func(ctx context.Context) (int, error) { return 0, in }, + ) + if !errors.Is(err, in) { + t.Fatalf("got %v; want original error returned unchanged", err) + } +} + +// TestThrottleRejectedMsgComparingErrorUnwraps verifies that such an error +// wrapped in RejectedError is classified as a rejection and unwrapped back to the +// original error for the caller. +func TestThrottleRejectedMsgComparingErrorUnwraps(t *testing.T) { + at := NewAdaptiveThrottle(4) + in := &msgComparingErr{message: "backend down"} + _, err := Throttle(context.Background(), at, High, + func(ctx context.Context) (int, error) { return 0, RejectedError(in) }, + ) + if err != in { + t.Fatalf("got %v; want unwrapped original error", err) + } +} + +// TestRejectedErrorNil verifies that RejectedError(nil) returns nil, so a +// successful call is never accounted as a rejection. +func TestRejectedErrorNil(t *testing.T) { + if err := RejectedError(nil); err != nil { + t.Fatalf("RejectedError(nil) = %v; want nil", err) + } + + at := NewAdaptiveThrottle(4) + res, err := Throttle(context.Background(), at, High, + func(ctx context.Context) (int, error) { return 42, RejectedError(nil) }, + ) + if err != nil || res != 42 { + t.Fatalf("got (res=%d, err=%v); want (42, nil)", res, err) + } +}