fix: correctly handle exceptions >400 #79
Conversation
There was a problem hiding this comment.
I'm concerned about the behavior change of a specific use case that this change will cause
- default client configuration
- one url configured
Before this change, on 429 we would actually not retry
After this change we will retry the same url twice on 429, which I would argue is strictly worse
|
re high-level concern: I don't think that's correct @ryanmcnamara , it would immediately retry because it would hit the |
|
Ah.. I see that.. makes sense Really do want to improve that, but obviously out of scope here for this pr |
| // If we get a nil response, we can assume there is a problem with host and can move on to the next. | ||
| nextURI, offset = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier) | ||
| } else if shouldThrottle, _ := internal.IsThrottleResponse(resp); shouldThrottle { | ||
| if retryOther, _ := internal.IsThrottleResponse(resp, err); retryOther { |
There was a problem hiding this comment.
These if blocks are are reordered now.. I guess there is no functionality change though? Can you confirm?
There was a problem hiding this comment.
no, this does matter now because the resp == nil case is true for both this statement and the following one. The test I added will fail if the order changes
|
👍 |
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 5 of 7 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ryanmcnamara and @whickman)
|
Released 0.4.1 |
Before this PR
error handling for codes >=400 was incorrect because of the error decoding middleware
After this PR
==COMMIT_MSG==
Fixes #78
Extracts status code from the returned error and acts on that. This limits more complex behavior, such as respecting the retry-after header.
==COMMIT_MSG==
Possible downsides?
The longer-term solution, which in my opinion is preferable, is to move retry logic into middleware below the error decoding middleware and out of the client directly. This will allow it to inspect the response before it gets filtered by the error decoding middleware.
Unfortunately, this is not possible given that we currently support arbitrary error decoders and retry behavior is dependent on whether an error is returned or not. Doing error decoding after retrying will therefore be a behavior break and will cause user-defined "errors" to no longer be retried.
This change is