Skip to content

Commit

Permalink
fix: Resolve issue where num_retries would have no effect (#1244)
Browse files Browse the repository at this point in the history
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
- [x] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/google-api-python-client/issues/new/choose) before writing your code!  That way we can discuss the change, evaluate designs, and agree on the general idea
- [x] Ensure the tests and linter pass
- [x] Code coverage does not decrease (if any source code was changed)
- [x] Appropriate docs were updated (if necessary)

Fixes #1243 馃
  • Loading branch information
parthea committed Mar 15, 2021
1 parent 22807c9 commit c518472
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 8 deletions.
21 changes: 18 additions & 3 deletions googleapiclient/http.py
Expand Up @@ -94,6 +94,8 @@ def _should_retry_response(resp_status, content):
Returns:
True if the response should be retried, otherwise False.
"""
reason = None

# Retry on 5xx errors.
if resp_status >= 500:
return True
Expand All @@ -113,9 +115,22 @@ def _should_retry_response(resp_status, content):
try:
data = json.loads(content.decode("utf-8"))
if isinstance(data, dict):
reason = data["error"].get("status")
if reason is None:
reason = data["error"]["errors"][0]["reason"]
# There are many variations of the error json so we need
# to determine the keyword which has the error detail. Make sure
# that the order of the keywords below isn't changed as it can
# break user code. If the "errors" key exists, we must use that
# first.
# See Issue #1243
# https://github.com/googleapis/google-api-python-client/issues/1243
error_detail_keyword = next((kw for kw in ["errors", "status", "message"] if kw in data["error"]), "")

if error_detail_keyword:
reason = data["error"][error_detail_keyword]

if isinstance(reason, list) and len(reason) > 0:
reason = reason[0]
if "reason" in reason:
reason = reason["reason"]
else:
reason = data[0]["error"]["errors"]["reason"]
except (UnicodeDecodeError, ValueError, KeyError):
Expand Down
25 changes: 20 additions & 5 deletions tests/test_http.py
Expand Up @@ -401,7 +401,7 @@ def test_media_io_base_next_chunk_retries(self):
({"status": "500"}, ""),
({"status": "503"}, ""),
({"status": "200", "location": "location"}, ""),
({"status": "403"}, USER_RATE_LIMIT_EXCEEDED_RESPONSE),
({"status": "403"}, USER_RATE_LIMIT_EXCEEDED_RESPONSE_NO_STATUS),
({"status": "403"}, RATE_LIMIT_EXCEEDED_RESPONSE),
({"status": "429"}, ""),
({"status": "200"}, "{}"),
Expand Down Expand Up @@ -834,7 +834,7 @@ def test_media_io_base_download_unknown_media_size(self):
--batch_foobarbaz--"""


USER_RATE_LIMIT_EXCEEDED_RESPONSE = """{
USER_RATE_LIMIT_EXCEEDED_RESPONSE_NO_STATUS = """{
"error": {
"errors": [
{
Expand All @@ -848,6 +848,20 @@ def test_media_io_base_download_unknown_media_size(self):
}
}"""

USER_RATE_LIMIT_EXCEEDED_RESPONSE_WITH_STATUS = """{
"error": {
"errors": [
{
"domain": "usageLimits",
"reason": "userRateLimitExceeded",
"message": "User Rate Limit Exceeded"
}
],
"code": 403,
"message": "User Rate Limit Exceeded",
"status": "PERMISSION_DENIED"
}
}"""

RATE_LIMIT_EXCEEDED_RESPONSE = """{
"error": {
Expand Down Expand Up @@ -981,10 +995,11 @@ def test_retry_connection_errors_resumable(self):
self.assertEqual({u"foo": u"bar"}, response)

def test_retry(self):
num_retries = 5
resp_seq = [({"status": "500"}, "")] * (num_retries - 3)
num_retries = 6
resp_seq = [({"status": "500"}, "")] * (num_retries - 4)
resp_seq.append(({"status": "403"}, RATE_LIMIT_EXCEEDED_RESPONSE))
resp_seq.append(({"status": "403"}, USER_RATE_LIMIT_EXCEEDED_RESPONSE))
resp_seq.append(({"status": "403"}, USER_RATE_LIMIT_EXCEEDED_RESPONSE_NO_STATUS))
resp_seq.append(({"status": "403"}, USER_RATE_LIMIT_EXCEEDED_RESPONSE_WITH_STATUS))
resp_seq.append(({"status": "429"}, ""))
resp_seq.append(({"status": "200"}, "{}"))

Expand Down

0 comments on commit c518472

Please sign in to comment.