avast/retry-go

Wrapping unrecoverable error makes it impossible to detect

Nitjsefni7 opened this issue · 2 comments

When wrapping error already wrapped with retry.Unrevocerable() it is not going to be properly detected by retry.IsRecoverable().

Example:

package main

import (
	"errors"
	"fmt"

	"github.com/avast/retry-go/v4"
)

func main() {
     err := retry.Unrecoverable(errors.New("unrecoverable err"))
     errWr := fmt.Errorf("wrapping: %w", err)

     // This is going to be printed.
     if !retry.IsRecoverable(err) {
          fmt.Println("unrecoverable")
     }

     // This is not going to be printed.
     if !retry.IsRecoverable(errWr) {
          fmt.Println("wrapped unrecoverable")
     }
}

Suggestion - while checking if error is recoverable, continue to unwrap error if possible:

// IsRecoverable checks if error is an instance of `unrecoverableError`
func IsRecoverable(err error) bool {
	for {
		if _, isUnrecoverable := err.(unrecoverableError); isUnrecoverable {
			return false
		}

		if err = errors.Unwrap(err); err == nil {
			return true
		}
	}
}
JaSei commented

This should be fixed by 4.3.2 properly - can you please check it? Thanks

@JaSei it's still broken - the issue is this code, which should be replaced with:

// IsRecoverable checks if error is an instance of `unrecoverableError`
func IsRecoverable(err error) bool {
	return !errors.Is(err, unrecoverableError{})
}

// Adds support for errors.Is usage on unrecoverableError
func (unrecoverableError) Is(err error) bool {
	_, isUnrecoverable := err.(unrecoverableError)
	return isUnrecoverable
}

This way, go will be able to continuously call .Unwrap on errors to find the real error, and on the unrecoverableError, it can call the Is method to see that it is indeed an unrecoverableError.

To see this in action, compare the updated code to the existing code.