-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle errors representing non-successful http responses in retry logic #558
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dhadka
force-pushed
the
dhadka/fix-error-handling
branch
2 times, most recently
from
August 17, 2020 17:53
ce4b145
to
0e3c328
Compare
dhadka
force-pushed
the
dhadka/fix-error-handling
branch
5 times, most recently
from
August 17, 2020 21:04
0bd131b
to
82a04e7
Compare
dhadka
force-pushed
the
dhadka/fix-error-handling
branch
from
August 18, 2020 01:28
82a04e7
to
de52c86
Compare
dhadka
force-pushed
the
dhadka/fix-error-handling
branch
from
October 12, 2020 18:29
be01bb2
to
c3c81d4
Compare
aiqiaoy
reviewed
Oct 16, 2020
} | ||
|
||
isRetryable = true | ||
errorMessage = error.message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should we log this errorMessage here since it gets converted to a generic Cache service responded with ${statusCode}
below if statusCode is set?
aiqiaoy
approved these changes
Oct 16, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
joshmgross
approved these changes
Oct 16, 2020
3e311
approved these changes
Nov 17, 2020
This was referenced Jun 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes cache to read status code contained in an
HttpClientError
thrown byhttp-client
and handle retries correctly. Fixes #537Why?
If using one of the standard
get
,put
,post
, etc. methods inhttp-client
, it returns anIHttpClientResponse
that contains a status code. To check if the request was successful or not, we must check the value of the status code.If using one of the JSON methods (
getJson
,putJson
,postJson
, etc.), it will return anITypedResponse<T>
if successful. If the status code is >= 300, it will throw an Error...except for 404 which will return anITypedResponse<T>
with anull
result.At some point, we updated the cache to use the JSON methods for several calls, but never updated the logic to expect an
Error
instead of a status code. As a result, the retry logic is not correctly handling errors. Specifically, errors such as a 409 conflict are not retryable. However, we treat a genericError
as retryable, as it could indicate a socket / network issue, and ultimately retry calls that will never succeed.Fix
This PR adds an optional
onError
handler to theretry
method, which allows it to convert theError
object it receives fromhttp-client
back into aITypedResponse<T>
. TheITypedResponse<T>
is populated with the status code contained in theError
object. All other fields are left empty (null
). This lets the existingisSuccessfulStatusCode
checks used by the cache module to work as expected.