WithRetry capability #73
Conversation
|
@gavv Any problem with the approach? |
|
Hi, being a bit busy currently with other projects. I'll find time for httpexpect in week or two. Sorry for inconvenience and thanks for the PR! |
|
No worries, thank you :) |
| for i := 0; i < r.maxRetry; i++ { | ||
| resp, err = r.config.Client.Do(r.http) |
There was a problem hiding this comment.
The problem here is that, in general, http.Request is not reusable after calling http.Client.Do, even if it failed. In particular, http.Request.Body is io.Reader, and http.Client may read the whole body, so that the retry will see an EOF, like the body is empty.
This problem doesn't appear with requests without body. We should add a test with a POST request with non-empty body which will catch this bug.
The typical way to solve this problem is:
- read the whole body into buffer
- create a new reader from the buffer
- call Client.Do
- on failure, go to step (2)
Some optimizations are possible, e.g. if the client buffer is ReaderSeeker, we can just seek to the beginning before each retry.
Quick googling showed that there is also a package the handles this issue: https://github.com/hashicorp/go-retryablehttp
Maybe we can use it here? Or at least reproduce part of its logic.
| @@ -851,6 +853,18 @@ func (r *Request) WithMultipart() *Request { | |||
| return r | |||
| } | |||
|
|
|||
| // WithRetry allows to retry an HTTP request until a max retry value. | |||
| // It also accepts a retry delay that is performed before to retry on failure. | |||
| func (r *Request) WithRetry(maxRetry int, retryDelay time.Duration) *Request { | |||
There was a problem hiding this comment.
Is the retryDelay parameter useful in practice? (In your case or in general). Maybe instead remove it and use exponential back-off, to make the API a bit simpler?
| if err != nil { | ||
| time.Sleep(r.retryDelay) | ||
| continue | ||
| } |
There was a problem hiding this comment.
It usually makes sense to retry only temporary network errors, like:
if nerr, ok := err.(net.Error); ok && nerr.Temporary() {
Also, it makes sense to retry timeout errors (err.Timeout()), I don't remember whether timeout error is considered temporary or not.
In addition, it usually makes sense to retry some of HTTP status codes, likely all 5xx errors except 501. However, for httpexpect it doesn't always make sense, so probably we need a separate option for this.
I guess we need to allow the user to specify something like "retry policy" (what to retry and how).
There was a problem hiding this comment.
Not necessarily. Please check at my use-case in the description. It's not simply a transient retry.
There was a problem hiding this comment.
I see. But I guess this is not the most common use case, so maybe handle it as a special retry policy?
E.g. we could have the following policies:
- retry temporary network errors
- retry temporary network errors and temporary server errors (5xx) - likely should be default when retrying is enabled
- retry any errors
What exact error are you retrying in your case?
| resp http.Response | ||
| err error | ||
| } | ||
|
|
||
| func (c *mockClient) Do(req *http.Request) (*http.Response, error) { | ||
| c.req = req | ||
| if c.retries != nil { | ||
| currentRetry := c.retries[c.retryIndex] |
There was a problem hiding this comment.
Let's add an explicit panic with meaningful message when index is out of bounds.
| @@ -165,6 +167,53 @@ func TestRequestClient(t *testing.T) { | |||
| req3.chain.assertFailed(t) | |||
| } | |||
|
|
|||
| func TestRequestWithRetrySuccess(t *testing.T) { | |||
There was a problem hiding this comment.
Can we also check the resulting retryIndex in these tests? To ensure that httpexpect made the desired number of retry attempts.
|
I've added an issue: #85. |
First of all, thanks for this cool library.
In my context, we would have been interested in a retry capability before to assert that a test is failing. We are running a test, synchronizing an internal cache and then we trigger an HTTP request. As we don't have the possibility to query the internal cache, we don't know when the HTTP query will succeed. Hence, we had to add an ugly time.Sleep() before calling httpexpect library.
Instead, it would be way cleaner to be able to retry a failing query. That would be something similar to what we have for example in Gomega with the
Eventuallyassertion: https://onsi.github.io/gomega/#making-assertionsWithRetrywould accept a max retry value and a retry delay.