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

Changes from 1 commit
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
16 changes: 9 additions & 7 deletions google/cloud/storage/retry.py
View file
Open in desktop
Expand Up @@ -14,19 +14,21 @@

import requests

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

Choose a reason for hiding this comment

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

Nit: maybe api_exceptions and auth_exceptions? Or just name the auth one and don't bother aliasing API.

Copy link
Contributor

Choose a reason for hiding this comment

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

We call it core_exceptions in BQ

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

import json


_RETRYABLE_TYPES = (
exceptions.TooManyRequests, # 429
exceptions.InternalServerError, # 500
exceptions.BadGateway, # 502
exceptions.ServiceUnavailable, # 503
exceptions.GatewayTimeout, # 504
exceptions_api.TooManyRequests, # 429
exceptions_api.InternalServerError, # 500
exceptions_api.BadGateway, # 502
exceptions_api.ServiceUnavailable, # 503
exceptions_api.GatewayTimeout, # 504
requests.ConnectionError,
exceptions_auth.TransportError,
Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://github.com/googleapis/google-auth-library-python/blob/36e6f0feb41e901effc854a4f8c907deb9998f21/google/auth/exceptions.py#L22, TransportError is "Used to indicate an error occurred during an HTTP request". Looks like it can be thrown in a bunch of places. Is this too broad? Do we need to subclass this on the auth library side to narrow it down, or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like there are any subclasses we can use.

https://github.com/googleapis/google-auth-library-python/blob/f1fee1f4c3d511d9e6ecbc1c0397e743bf2583db/google/auth/transport/requests.py#L187-L189

Maybe we could look at the inner exception, but TransportError does seem to be as specific as it gets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was wondering whether we should possibly add subclasses in the auth library to allow more differentiation. @frankyn could you speak more to what kind of problems will cause this exception to be raised?

Copy link
Member Author

Choose a reason for hiding this comment

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

When I dug into this, auth.TransportError is only used within the auth package. However, as you mentioned it could include any of the following exceptions for at least requests: https://docs.python-requests.org/en/master/_modules/requests/exceptions/ but we only want to retry on ConnectionError.

Taking a note from the Go implementation, I'm sending another commit to instead unwrap the inner exception given we already retry on requests.ConnectionError.

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit strange that we are seeing requests.ConnectionError bare, but we're also seeing it wrapped by auth.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I was wondering about this-- would this happen because ConnectionError is raised during different kinds of requests for example?

Copy link
Member Author

Choose a reason for hiding this comment

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

We still have a feature request to wrap ConnectionError like ResetConnection in api_core, however that doesn't resolve it for the auth library because it does its own wrapping.

)

# Some retriable errors don't have their own custom exception in api_core.
Expand All @@ -37,7 +39,7 @@ 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, exceptions_api.GoogleAPICallError):
return exc.code in _ADDITIONAL_RETRYABLE_STATUS_CODES
else:
return False
Expand Down