feat: add timeout parameter to AuthorizedSession.request() #406
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `request.Request` class also accepts a timeout as a pair (connect_timeout, read_timeout), and some downstream libraries use this form. This commit makes sure that the timeout logic correctly handles timeouts as a two-tuple. See also: https://2.python-requests.org/en/master/user/advanced/#timeouts
googlebot
added
the
cla: yes
Merged
5 tasks
busunkim96
approved these changes
Dec 11, 2019
Contributor
busunkim96
left a comment
busunkim96
left a comment
There was a problem hiding this comment.
LGTM. @tswast would you also be able to review this?
tswast
approved these changes
Dec 12, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #396.
This PR is a replacement for #397 that caused a bug in at least one downstream librariy. It contains additional logic that correctly handles
timeoutspassed as a tuple (therequestslibrary support two different timeout forms - link).The first commit is identical to the original PR, and all new logic is contained in the commit(s) that follow.
MIND:
A downstream library that is timeout-sensitive might still be affected by the change, because the new explicit timeout parameter changes the semantics of
AuthorizedSession.request(). The timeout represents the total allowed time for all requests made behind the scenes (e.g. credentials refresh attempts), as opposed to a previous per-request timeout.The rationale behind this, however, is that libraries such as BigQuery use
AuthorizedSessionas their transport, and considerauth_session.request(...)a single operation, even though the latter might consist of multiple requests.P.S.: On the other hand, and if I read the code correctly, any exiting
timeoutargument (passed via**kwargs) already applied to the "main" request only, and not to the credentials-related requests. The initialself.credentials.before_request()call had a hard-coded timeout of 120 seconds, while any subsequent credentials refresh calls usedself._refresh_timeout, which is actuallyNoneby default.