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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: retry auth.TransportError errors #418

Merged
merged 7 commits into from Apr 26, 2021
Merged

fix: retry auth.TransportError errors #418

merged 7 commits into from Apr 26, 2021

Conversation

@frankyn
Copy link
Member

@frankyn frankyn commented Apr 21, 2021

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:

  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)

Fixes #414 馃

@google-cla google-cla bot added the cla: yes label Apr 21, 2021
@frankyn frankyn changed the title fix: retry auth.TransportError's fix: retry auth.TransportError errors Apr 21, 2021
@frankyn frankyn marked this pull request as ready for review Apr 21, 2021
@frankyn frankyn requested a review from Apr 21, 2021
@frankyn frankyn requested a review from as a code owner Apr 21, 2021
@frankyn frankyn requested a review from tswast Apr 21, 2021
@frankyn
Copy link
Member Author

@frankyn frankyn commented Apr 21, 2021

@tswast I saw you had the same issue in bigquery. Could you take a look? I think this is a reasonable add given that this error only occurs in this case, but It might be better to restrict to specific types of connection errors.

@@ -14,19 +14,21 @@

import requests

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

@tritone tritone Apr 21, 2021

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

Copy link
Contributor

@tswast tswast Apr 21, 2021

We call it core_exceptions in BQ

requests.ConnectionError,
exceptions_auth.TransportError,
Copy link
Contributor

@tritone tritone Apr 21, 2021

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

@tswast tswast Apr 21, 2021

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

@tritone tritone Apr 21, 2021

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

@frankyn frankyn Apr 21, 2021

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

@andrewsg andrewsg Apr 22, 2021

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

Copy link
Contributor

@tritone tritone Apr 22, 2021

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

Copy link
Member Author

@frankyn frankyn Apr 22, 2021

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.

@@ -14,18 +14,18 @@

import requests

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

@tritone tritone Apr 22, 2021

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

return exc.code in _ADDITIONAL_RETRYABLE_STATUS_CODES
elif isinstance(exc, Exception):
return _should_retry(exc.args[0])
Copy link
Contributor

@tritone tritone Apr 22, 2021

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

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.

@frankyn frankyn requested review from tritone and andrewsg Apr 22, 2021
Copy link
Contributor

@andrewsg andrewsg left a comment

Thanks!

@frankyn frankyn merged commit 23a8db8 into master Apr 26, 2021
3 checks passed
@frankyn frankyn deleted the retry-auth-exceptions branch Apr 26, 2021
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
* fix: retry auth.TransportError's

* unwrap exceptions

* lint

* revert exceptions alias

* scope unwrapping and add unit test

* lint

* fix unit test
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
* fix: retry auth.TransportError's

* unwrap exceptions

* lint

* revert exceptions alias

* scope unwrapping and add unit test

* lint

* fix unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants