Fix retries #204
Fix retries #204
Conversation
Requests with POST bodies had their bodies consumed with the first request. If that request failed and was retried, the body was then empty. This caused a mismatch with the advertised Content-Length. Since this is a net/url error, not an aws.APIError, no further retries were attempted. As a result, no successful retries ever occurred. The fix is simple: Seek back to the beginning of the body before retrying.
This is more efficient than converting the string to a []byte.
| @@ -59,6 +59,9 @@ func ValidateResponseHandler(r *Request) { | |||
| } | |||
| r.Retryable = r.Service.ShouldRetry(r) | |||
| r.RetryDelay = r.Service.RetryRules(r) | |||
| if r.Retryable { | |||
| r.ResetReaderBody() | |||
There was a problem hiding this comment.
I'm thinking this logic might be bettered suited to a BeforeRetry handler. We already have AfterRetry and Retry handlers so I'm wondering if in this case it would make sense to create a BeforeRetry. What do you think?
|
Thanks for the PR @josharian! What do you think about the idea of using a new BeforeRetry handler instead of ValidateResponse handler for resetting the reader? |
|
In #196 the r.Retryable = ... and r.RetryDelay logic was moved to a RetryHandler. It might make sense for r.ResetReaderBody to be there instead a BeforeRetry handler. |
|
Thanks for the speedy review. Using a BeforeRetry handler sounds reasonable. Updated: dc19fa0. If you want a clean commit history, I am happy to squash all the commits if/when you're ready to merge. Regarding #196, I'm not really sure I understand what is going on there, and I'd like this fix to be orthogonal to it. (And hey, maybe this PR will make the build pass for #196.) |
|
In case it helps, here's a script to test retries by hitting API limits: https://gist.github.com/josharian/b77539174c0ac5a5ca13 |
|
Thanks @josharian, the script will definitely help. |
… retry attempts. Merged #196 and #204 together along with other changes to make sure rewind works in all cases, Made sure the ordering of handler's setting Request.Retryable vs the Service.ShouldRetry are consitent. A Request Handler setting Request.Retryable will prevent RetryHandler calling Service.ShouldRetry. The Service's delay value wills still be applied though.
|
Superceded by #211. |
Retries for requests with POST bodies were broken. This fixes them.
Also a tiny bit of cleanup in subsequent commits.