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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: retry auth.TransportError errors #418

Merged
merged 7 commits into from Apr 26, 2021
Merged

fix: retry auth.TransportError errors #418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4f213d9
fix: retry auth.TransportError's
frankyn Apr 21, 2021
943898d
unwrap exceptions
frankyn Apr 21, 2021
180954a
lint
frankyn Apr 22, 2021
aeee95b
revert exceptions alias
frankyn Apr 22, 2021
0a1d9e9
scope unwrapping and add unit test
frankyn Apr 22, 2021
020bac3
lint
frankyn Apr 22, 2021
7bf2c39
fix unit test
frankyn Apr 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 10 additions & 7 deletions google/cloud/storage/retry.py
View file
Open in desktop
Expand Up @@ -14,18 +14,19 @@

import requests

from google.api_core import exceptions
from google.api_core import exceptions as api_exceptions
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer has to be aliased since you're not importing the auth exceptions as well.

from google.api_core import retry
from google.auth import exceptions as auth_exceptions

import json


_RETRYABLE_TYPES = (
exceptions.TooManyRequests, # 429
exceptions.InternalServerError, # 500
exceptions.BadGateway, # 502
exceptions.ServiceUnavailable, # 503
exceptions.GatewayTimeout, # 504
api_exceptions.TooManyRequests, # 429
api_exceptions.InternalServerError, # 500
api_exceptions.BadGateway, # 502
api_exceptions.ServiceUnavailable, # 503
api_exceptions.GatewayTimeout, # 504
requests.ConnectionError,
)

Expand All @@ -37,8 +38,10 @@ def _should_retry(exc):
"""Predicate for determining when to retry."""
if isinstance(exc, _RETRYABLE_TYPES):
return True
elif isinstance(exc, exceptions.GoogleAPICallError):
elif isinstance(exc, api_exceptions.GoogleAPICallError):
return exc.code in _ADDITIONAL_RETRYABLE_STATUS_CODES
elif isinstance(exc, auth_exceptions.TransportError):
return _should_retry(exc.args[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

This is clever! Just wanted to check--

  1. Will exc.args always have at least one item?
  2. Can you speak to how you were able to test/repro that this catches the issue in question?

Copy link
Member Author

@frankyn frankyn Apr 22, 2021 •
edited

Choose a reason for hiding this comment

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

Re: Point 1)
So args per Python documentation represents the tuple of constructor parameters:

The tuple of arguments given to the exception constructor. Some built-in exceptions (like OSError) expect a certain number of arguments and assign a special meaning to the elements of this tuple, while others are usually called only with a single string giving an error message.

My understanding is given this information and the way auth.TransportError is created the underlying Exception should be the first parameter of the construction. I think I could go a step further though and only unwrap when we know it's an auth.TransportError so it's scoped to that specific error.

Re: Point 2)
As for repro, I think the correct thing to do here is add a unit test. What I did was introduce the error in auth package which followed the exception format in issue #414. I'll make a follow-up change.

else:
return False

Expand Down
11 changes: 10 additions & 1 deletion tests/unit/test_retry.py
View file
Open in desktop
Expand Up @@ -25,7 +25,16 @@ def _call_fut(self, exc):

return retry._should_retry(exc)

def test_w_retryable_types(self):
def test_w_retryable_transport_error(self):
from google.cloud.storage import retry
from google.auth.exceptions import TransportError as eTransportError
from requests import ConnectionError as rConnectionError

caught_exc = rConnectionError("Remote end closed connection unexpected")
exc = eTransportError(caught_exc)
self.assertTrue(retry._should_retry(exc))

def test_w_wrapped_type(self):
from google.cloud.storage import retry

for exc_type in retry._RETRYABLE_TYPES:
Expand Down