Skip to content
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
merged 4 commits into from
Oct 20, 2020

Conversation

dhadka
Copy link
Member

@dhadka dhadka commented Aug 17, 2020

Fixes cache to read status code contained in an HttpClientError thrown by http-client and handle retries correctly. Fixes #537

Why?

If using one of the standard get, put, post, etc. methods in http-client, it returns an IHttpClientResponse 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 an ITypedResponse<T> if successful. If the status code is >= 300, it will throw an Error...except for 404 which will return an ITypedResponse<T> with a null 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 generic Error 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 the retry method, which allows it to convert the Error object it receives from http-client back into a ITypedResponse<T>. The ITypedResponse<T> is populated with the status code contained in the Error object. All other fields are left empty (null). This lets the existing isSuccessfulStatusCode checks used by the cache module to work as expected.

@dhadka dhadka force-pushed the dhadka/fix-error-handling branch 2 times, most recently from ce4b145 to 0e3c328 Compare August 17, 2020 17:53
@dhadka dhadka force-pushed the dhadka/fix-error-handling branch 5 times, most recently from 0bd131b to 82a04e7 Compare August 17, 2020 21:04
}

isRetryable = true
errorMessage = error.message
Copy link
Contributor

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?

Copy link
Contributor

@aiqiaoy aiqiaoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Cache already exists" due to concurrency
4 participants