cenkalti/backoff

Incorrect and unused `Is()` function

Hades32 opened this issue · 4 comments

While looking through the code I noticed this:

backoff/retry.go

Lines 99 to 102 in a78d380

func (e *PermanentError) Is(target error) bool {
_, ok := target.(*PermanentError)
return ok
}

Go doesn't support (yet) the Is(err) function for use in errors.Is(), and if it did then this implementation is fast but wrong for all wrapped errors.

Can we remove this? I think it's confusing and might break in the future

seh commented

@Hades32, can you clarify the "wrong for all wrapped errors" part? The current implementation of the errors.Is function will progress beyond an error's Is method that doesn't match the target error and inspect the wrapped errors revealed by an error's Unwrap method.

kvij commented

I think you misunderstand how custom errors are to be implemented. This implementation is correct and needed for errors.Is() to function (as PermanentError isn't a comparable). You are not supposed to call this method yourself. Nor should you call the Unwrap method. They exist to adhere to the interfaces the errors package uses.

I haven't retested this in recent versions, but my point was that when I wrap an errors errors in my own code, and an error is such a permanent error, then this comparison would not work, since it would happen before the unwrap.
In my own code I simply have my own errors that adhere to the interface for permanent errors. That seems to be the best solution anyway

kvij commented

As you can see from the comment of @seh you are wrong about that.
errors.Is() is performing an == check if it is a comparable. Then it moves on checking if it implements the Is(error) bool interface and calls that. This should just perform the check specific to this type (exactly as implemented in this lib). If it does not match it performs an unwrap if the error was wrapped and the next itteration of the loop ensues.
Meaning it does the same "fast" checks a level deeper and so on.
The traversing is impleented in errors.Is not in the Is method.