From 52a733d604528fa86d05321bb74241a43aea4211 Mon Sep 17 00:00:00 2001 From: Peter Lamut Date: Wed, 22 Jan 2020 19:07:03 +0000 Subject: [PATCH 1/4] feat: distinguish transport and execution time timeouts (#424) In Requests transport treat ``timeout`` stricly as a transport timeout, while the total allowed method execution time can be set with an optional ``max_allowed_time`` argument. --- google/auth/transport/requests.py | 63 +++++++++++++++++++------------ tests/transport/test_requests.py | 51 ++++++++++++++++++------- 2 files changed, 76 insertions(+), 38 deletions(-) diff --git a/google/auth/transport/requests.py b/google/auth/transport/requests.py index 1caec0b47..ce78e63a8 100644 --- a/google/auth/transport/requests.py +++ b/google/auth/transport/requests.py @@ -239,18 +239,38 @@ def __init__( # credentials.refresh). self._auth_request = auth_request - def request(self, method, url, data=None, headers=None, timeout=None, **kwargs): + def request( + self, + method, + url, + data=None, + headers=None, + max_allowed_time=None, + timeout=None, + **kwargs + ): """Implementation of Requests' request. Args: - timeout (Optional[Union[float, Tuple[float, float]]]): The number - of seconds to wait before raising a ``Timeout`` exception. If - multiple requests are made under the hood, ``timeout`` is - interpreted as the approximate total time of **all** requests. - - If passed as a tuple ``(connect_timeout, read_timeout)``, the - smaller of the values is taken as the total allowed time across - all requests. + timeout (Optional[Union[float, Tuple[float, float]]]): + The amount of time in seconds to wait for the server response + with each individual request. + + Can also be passed as a tuple (connect_timeout, read_timeout). + See :meth:`requests.Session.request` documentation for details. + + max_allowed_time (Optional[float]): + If the method runs longer than this, a ``Timeout`` exception is + automatically raised. Unlike the ``timeout` parameter, this + value applies to the total method execution time, even if + multiple requests are made under the hood. + + Mind that it is not guaranteed that the timeout error is raised + at ``max_allowed_time`. It might take longer, for example, if + an underlying request takes a lot of time, but the request + itself does not timeout, e.g. if a large file is being + transmitted. The timout error will be raised after such + request completes. """ # pylint: disable=arguments-differ # Requests has a ton of arguments to request, but only two @@ -273,11 +293,13 @@ def request(self, method, url, data=None, headers=None, timeout=None, **kwargs): else functools.partial(self._auth_request, timeout=timeout) ) - with TimeoutGuard(timeout) as guard: + remaining_time = max_allowed_time + + with TimeoutGuard(remaining_time) as guard: self.credentials.before_request(auth_request, method, url, request_headers) - timeout = guard.remaining_timeout + remaining_time = guard.remaining_timeout - with TimeoutGuard(timeout) as guard: + with TimeoutGuard(remaining_time) as guard: response = super(AuthorizedSession, self).request( method, url, @@ -286,7 +308,7 @@ def request(self, method, url, data=None, headers=None, timeout=None, **kwargs): timeout=timeout, **kwargs ) - timeout = guard.remaining_timeout + remaining_time = guard.remaining_timeout # If the response indicated that the credentials needed to be # refreshed, then refresh the credentials and re-attempt the @@ -305,14 +327,6 @@ def request(self, method, url, data=None, headers=None, timeout=None, **kwargs): self._max_refresh_attempts, ) - if self._refresh_timeout is not None: - if timeout is None: - timeout = self._refresh_timeout - elif isinstance(timeout, numbers.Number): - timeout = min(timeout, self._refresh_timeout) - else: - timeout = tuple(min(x, self._refresh_timeout) for x in timeout) - # Do not apply the timeout unconditionally in order to not override the # _auth_request's default timeout. auth_request = ( @@ -321,17 +335,18 @@ def request(self, method, url, data=None, headers=None, timeout=None, **kwargs): else functools.partial(self._auth_request, timeout=timeout) ) - with TimeoutGuard(timeout) as guard: + with TimeoutGuard(remaining_time) as guard: self.credentials.refresh(auth_request) - timeout = guard.remaining_timeout + remaining_time = guard.remaining_timeout # Recurse. Pass in the original headers, not our modified set, but - # do pass the adjusted timeout (i.e. the remaining time). + # do pass the adjusted max allowed time (i.e. the remaining total time). return self.request( method, url, data=data, headers=headers, + max_allowed_time=remaining_time, timeout=timeout, _credential_refresh_attempt=_credential_refresh_attempt + 1, **kwargs diff --git a/tests/transport/test_requests.py b/tests/transport/test_requests.py index 00269740f..8f73d4bd5 100644 --- a/tests/transport/test_requests.py +++ b/tests/transport/test_requests.py @@ -220,7 +220,25 @@ def test_request_refresh(self): assert adapter.requests[1].url == self.TEST_URL assert adapter.requests[1].headers["authorization"] == "token1" - def test_request_timeout(self, frozen_time): + def test_request_max_allowed_time_timeout_error(self, frozen_time): + tick_one_second = functools.partial(frozen_time.tick, delta=1.0) + + credentials = mock.Mock( + wraps=TimeTickCredentialsStub(time_tick=tick_one_second) + ) + adapter = TimeTickAdapterStub( + time_tick=tick_one_second, responses=[make_response(status=http_client.OK)] + ) + + authed_session = google.auth.transport.requests.AuthorizedSession(credentials) + authed_session.mount(self.TEST_URL, adapter) + + # Because a request takes a full mocked second, max_allowed_time shorter + # than that will cause a timeout error. + with pytest.raises(requests.exceptions.Timeout): + authed_session.request("GET", self.TEST_URL, max_allowed_time=0.9) + + def test_request_max_allowed_time_w_transport_timeout_no_error(self, frozen_time): tick_one_second = functools.partial(frozen_time.tick, delta=1.0) credentials = mock.Mock( @@ -237,12 +255,12 @@ def test_request_timeout(self, frozen_time): authed_session = google.auth.transport.requests.AuthorizedSession(credentials) authed_session.mount(self.TEST_URL, adapter) - # Because at least two requests have to be made, and each takes one - # second, the total timeout specified will be exceeded. - with pytest.raises(requests.exceptions.Timeout): - authed_session.request("GET", self.TEST_URL, timeout=1.9) + # A short configured transport timeout does not affect max_allowed_time. + # The latter is not adjusted to it and is only concerned with the actual + # execution time. The call below should thus not raise a timeout error. + authed_session.request("GET", self.TEST_URL, timeout=0.5, max_allowed_time=3.1) - def test_request_timeout_w_refresh_timeout(self, frozen_time): + def test_request_max_allowed_time_w_refresh_timeout_no_error(self, frozen_time): tick_one_second = functools.partial(frozen_time.tick, delta=1.0) credentials = mock.Mock( @@ -257,15 +275,17 @@ def test_request_timeout_w_refresh_timeout(self, frozen_time): ) authed_session = google.auth.transport.requests.AuthorizedSession( - credentials, refresh_timeout=1.9 + credentials, refresh_timeout=1.1 ) authed_session.mount(self.TEST_URL, adapter) - # The timeout is long, but the short refresh timeout will prevail. - with pytest.raises(requests.exceptions.Timeout): - authed_session.request("GET", self.TEST_URL, timeout=60) + # A short configured refresh timeout does not affect max_allowed_time. + # The latter is not adjusted to it and is only concerned with the actual + # execution time. The call below should thus not raise a timeout error + # (and `timeout` does not come into play either, as it's very long). + authed_session.request("GET", self.TEST_URL, timeout=60, max_allowed_time=3.1) - def test_request_timeout_w_refresh_timeout_and_tuple_timeout(self, frozen_time): + def test_request_timeout_w_refresh_timeout_timeout_error(self, frozen_time): tick_one_second = functools.partial(frozen_time.tick, delta=1.0) credentials = mock.Mock( @@ -284,7 +304,10 @@ def test_request_timeout_w_refresh_timeout_and_tuple_timeout(self, frozen_time): ) authed_session.mount(self.TEST_URL, adapter) - # The shortest timeout will prevail and cause a Timeout error, despite - # other timeouts being quite long. + # An UNAUTHORIZED response triggers a refresh (an extra request), thus + # the final request that otherwise succeeds results in a timeout error + # (all three requests together last 3 mocked seconds). with pytest.raises(requests.exceptions.Timeout): - authed_session.request("GET", self.TEST_URL, timeout=(100, 2.9)) + authed_session.request( + "GET", self.TEST_URL, timeout=60, max_allowed_time=2.9 + ) From 1b9de8dfbe4523f3170e47985ab523cb7865de48 Mon Sep 17 00:00:00 2001 From: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com> Date: Wed, 22 Jan 2020 16:09:31 -0800 Subject: [PATCH 2/4] docs: link to docs on googleapis, edit old links (#432) * Link to docs on googleapis.dev, not readthedocs * Edit old repo links to use org googleapis, not GoogleCloudPlatform --- README.rst | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/README.rst b/README.rst index afc47880f..c9c411f08 100644 --- a/README.rst +++ b/README.rst @@ -1,13 +1,11 @@ Google Auth Python Library ========================== -|docs| |pypi| +|pypi| This library simplifies using Google's various server-to-server authentication mechanisms to access Google APIs. -.. |docs| image:: https://readthedocs.org/projects/google-auth/badge/?version=latest - :target: https://google-auth.readthedocs.io/en/latest/ .. |pypi| image:: https://img.shields.io/pypi/v/google-auth.svg :target: https://pypi.python.org/pypi/google-auth @@ -35,7 +33,7 @@ Python == 2.7. Python 2.7 support will be removed on January 1, 2020. Documentation ------------- -Google Auth Python Library has usage and reference documentation at `google-auth.readthedocs.io `_. +Google Auth Python Library has usage and reference documentation at https://googleapis.dev/python/google-auth/latest/index.html. Current Maintainers ------------------- @@ -55,11 +53,11 @@ Contributions to this library are always welcome and highly encouraged. See `CONTRIBUTING.rst`_ for more information on how to get started. -.. _CONTRIBUTING.rst: https://github.com/GoogleCloudPlatform/google-auth-library-python/blob/master/CONTRIBUTING.rst +.. _CONTRIBUTING.rst: https://github.com/googleapis/google-auth-library-python/blob/master/CONTRIBUTING.rst License ------- Apache 2.0 - See `the LICENSE`_ for more information. -.. _the LICENSE: https://github.com/GoogleCloudPlatform/google-auth-library-python/blob/master/LICENSE +.. _the LICENSE: https://github.com/googleapis/google-auth-library-python/blob/master/LICENSE From d274a3a2b3f913bc2cab4ca51f9c7fdef94b8f31 Mon Sep 17 00:00:00 2001 From: Peter Lamut Date: Thu, 23 Jan 2020 22:43:59 +0000 Subject: [PATCH 3/4] feat: add non-None default timeout to AuthorizedSession.request() (#435) Closes #434. This PR adds a non-None default timeout to `AuthorizedSession.request()` to prevent requests from hanging indefinitely in a default case. Should help with googleapis/google-cloud-python#10182 --- google/auth/transport/requests.py | 12 ++++++++++-- tests/transport/test_requests.py | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/google/auth/transport/requests.py b/google/auth/transport/requests.py index ce78e63a8..1c709d4f7 100644 --- a/google/auth/transport/requests.py +++ b/google/auth/transport/requests.py @@ -42,6 +42,8 @@ _LOGGER = logging.getLogger(__name__) +_DEFAULT_TIMEOUT = 120 # in seconds + class _Response(transport.Response): """Requests transport response adapter. @@ -141,7 +143,13 @@ def __init__(self, session=None): self.session = session def __call__( - self, url, method="GET", body=None, headers=None, timeout=120, **kwargs + self, + url, + method="GET", + body=None, + headers=None, + timeout=_DEFAULT_TIMEOUT, + **kwargs ): """Make an HTTP request using requests. @@ -246,7 +254,7 @@ def request( data=None, headers=None, max_allowed_time=None, - timeout=None, + timeout=_DEFAULT_TIMEOUT, **kwargs ): """Implementation of Requests' request. diff --git a/tests/transport/test_requests.py b/tests/transport/test_requests.py index 8f73d4bd5..f0321c81b 100644 --- a/tests/transport/test_requests.py +++ b/tests/transport/test_requests.py @@ -177,6 +177,21 @@ def test_constructor_with_auth_request(self): assert authed_session._auth_request == auth_request + def test_request_default_timeout(self): + credentials = mock.Mock(wraps=CredentialsStub()) + response = make_response() + adapter = AdapterStub([response]) + + authed_session = google.auth.transport.requests.AuthorizedSession(credentials) + authed_session.mount(self.TEST_URL, adapter) + + patcher = mock.patch("google.auth.transport.requests.requests.Session.request") + with patcher as patched_request: + authed_session.request("GET", self.TEST_URL) + + expected_timeout = google.auth.transport.requests._DEFAULT_TIMEOUT + assert patched_request.call_args.kwargs.get("timeout") == expected_timeout + def test_request_no_refresh(self): credentials = mock.Mock(wraps=CredentialsStub()) response = make_response() From 6c6c981162a0014004ecb8cc60c0e62327288569 Mon Sep 17 00:00:00 2001 From: "release-please[bot]" <55107282+release-please[bot]@users.noreply.github.com> Date: Thu, 23 Jan 2020 15:21:49 -0800 Subject: [PATCH 4/4] chore: release 1.11.0 (#430) --- CHANGELOG.md | 8 ++++++++ setup.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc1d040d1..1613f0687 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ [1]: https://pypi.org/project/google-auth/#history +## [1.11.0](https://www.github.com/googleapis/google-auth-library-python/compare/v1.10.2...v1.11.0) (2020-01-23) + + +### Features + +* add non-None default timeout to AuthorizedSession.request() ([#435](https://www.github.com/googleapis/google-auth-library-python/issues/435)) ([d274a3a](https://www.github.com/googleapis/google-auth-library-python/commit/d274a3a2b3f913bc2cab4ca51f9c7fdef94b8f31)), closes [#434](https://www.github.com/googleapis/google-auth-library-python/issues/434) [googleapis/google-cloud-python#10182](https://www.github.com/googleapis/google-cloud-python/issues/10182) +* distinguish transport and execution time timeouts ([#424](https://www.github.com/googleapis/google-auth-library-python/issues/424)) ([52a733d](https://www.github.com/googleapis/google-auth-library-python/commit/52a733d604528fa86d05321bb74241a43aea4211)), closes [#423](https://github.com/googleapis/google-auth-library-python/issues/423) + ### [1.10.2](https://www.github.com/googleapis/google-auth-library-python/compare/v1.10.1...v1.10.2) (2020-01-18) diff --git a/setup.py b/setup.py index 708a5d164..bbed85e50 100644 --- a/setup.py +++ b/setup.py @@ -30,7 +30,7 @@ with io.open("README.rst", "r") as fh: long_description = fh.read() -version = "1.10.2" +version = "1.11.0" setup( name="google-auth",