hashicorp / hashicorp/go-retryablehttp

Preserve url.Error/http.httpError Timeout

Open
#142 0 comments 7 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
2.3k
Forks
298
PR merge metrics
No merged PRs in 30d

Description

Similar to https://github.com/hashicorp/go-retryablehttp/issues/129 but instead of context.Deadline it is for url.Error/http.httpError Timeout().

When a timeout occurs, expecting the error returned to have `url.Error` with a `Timeout() == true`. Because https://github.com/hashicorp/go-retryablehttp/blob/master/client.go#L693 wraps the error from `client.Do` and then Go std then puts this error into a `url.Error` type (https://cs.opensource.google/go/go/+/refs/tags/go1.17.1:src/net/http/client.go;l=621-636) the top level `url.Error`'s `Timeout() == false`. This means you need to unwrap the top level error, then call `errors.As` to get a `url.Error` with a `Timeout() == true`.

To reproduce:
```go
package main

import (
"context"
"errors"
"net/http"
"net/http/httptest"
"net/url"
"time"

"github.com/hashicorp/go-retryablehttp"
)

func main() {
var (
ctx = context.Background()
timeout = time.Millisecond
retryableClient = retryablehttp.NewClient()
ts = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// always exceed deadline
time.Sleep(timeout * 2)
}))
)
defer ts.Close()

retryableClient.Logger = nil
retryableClient.RetryMax = 1
retryableClient.Backoff = retryablehttp.DefaultBackoff
retryableClient.HTTPClient.Timeout = timeout
retryableClient.RetryWaitMin = time.Millisecond
retryableClient.RetryWaitMax = time.Millisecond
retryableClient.CheckRetry = func(ctx context.Context, resp *http.Response, err error) (bool, error) {
return true, nil
}

client := retryableClient.StandardClient()

req, err := http.NewRequestWithContext(ctx, http.MethodGet, ts.URL, nil)
if err != nil {
panic(err)
}

_, err = client.Do(req)
var urlErr *url.Error
if errors.As(err, &urlErr) {
if !urlErr.Timeout() {
panic("urlErr.Timeout() is false")
}
} else {
panic("not url.Error")
}
}
```

Possible solution would be to add some Error type that preserves the original `url.Error`. Though I am sure this could be handled another way.
```go
// Error reports an error and an optional message along with it.
type Error struct {
Err error
Msg string
}

func (e *Error) Error() string {
if e.Msg != "" {
return fmt.Sprintf("%s: %v", e.Msg, e.Err)
}
return e.Err.Error()
}

func (e *Error) Unwrap() error {
return errors.Unwrap(e.Err)
}

// Timeout will check if the e.Err fulfills "Timeout() bool" interface
// This is important for when client.Do returns a url.Error/http.httpError
// because of a timeout.
func (e *Error) Timeout() bool {
t, ok := e.Err.(interface {
Timeout() bool
})
return ok && t.Timeout()
}
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.