diff --git a/CHANGELOG.md b/CHANGELOG.md index 5982e2b9..cabf3897 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ All notable changes to the **Sinch Python SDK** are documented in this file. ### SDK +- **[feature]** `HTTPTransport` now automatically retries rate-limited (`HTTP 429`) responses up to 3 times with backoff for endpoints that opt in via the `HTTPEndpoint.IS_RETRYABLE` flag (`False` by default). When the server sends a `Retry-After` header it is honoured; otherwise an exponential backoff with full jitter is used. If all retries are exhausted the last response is returned to the caller (#159). - **[dependency]** Set up minimum version for `requests` to `>=2.0.0` to prevent pulling in versions with known vulnerabilities (#152). - **[fix]** Fixed a race condition in OAuth token creation and renewal under concurrent requests: `TokenManagerBase` now uses a lock with double-checked locking so the initial token is fetched exactly once, and a new `refresh_auth_token(used_token)` deduplicates concurrent renewals by only fetching when the stale token still matches the cached one (#156). - **[refactor]** `HTTPTransport` now prepares and authenticates requests in `request()`, so the new `send_request(request_data)` receives an already-prepared `HttpRequest` and acts as a pure I/O primitive, simplifying subclassing (#156). diff --git a/sinch/core/endpoint.py b/sinch/core/endpoint.py index 30b4b6f7..bd498d53 100644 --- a/sinch/core/endpoint.py +++ b/sinch/core/endpoint.py @@ -4,6 +4,7 @@ class HTTPEndpoint(ABC): ENDPOINT_URL = None + IS_RETRYABLE = False @property @abstractmethod diff --git a/sinch/core/ports/http_transport.py b/sinch/core/ports/http_transport.py index 6e97cf08..f61b05d0 100644 --- a/sinch/core/ports/http_transport.py +++ b/sinch/core/ports/http_transport.py @@ -1,5 +1,9 @@ +import random +import time import warnings from abc import ABC +from datetime import datetime, timezone +from email.utils import parsedate_to_datetime from platform import python_version from typing import Optional @@ -26,6 +30,11 @@ class HTTPTransport(ABC): ``send`` override path will be removed in 3.0. """ + MAX_RETRIES = 3 + RETRYABLE_STATUS_CODES = frozenset({429}) + BACKOFF_BASE_SECONDS = 1.0 + BACKOFF_GROWTH = 4 + def __init__(self, sinch): self.sinch = sinch self._legacy_send = self._uses_legacy_send() @@ -91,15 +100,114 @@ def request(self, endpoint: HTTPEndpoint) -> HTTPResponse: request_data = self.prepare_request(endpoint) request_data = self.authenticate(endpoint, request_data) - http_response = self.send_request(request_data) + + http_response = self._send_with_retries(endpoint, request_data) if self._should_refresh_token(endpoint, http_response): used_token = self._get_bearer_token_from_request(request_data) new_token = self.sinch.configuration.token_manager.refresh_auth_token(used_token) self._set_bearer_token(request_data, new_token.access_token) - http_response = self.send_request(request_data) + http_response = self._send_with_retries(endpoint, request_data) return endpoint.handle_response(http_response) + + + def _send_with_retries( + self, endpoint: HTTPEndpoint, request_data: Optional[HttpRequest] = None + ) -> HTTPResponse: + """ + Sends a request, retrying rate-limited responses (up to MAX_RETRIES, + with backoff) for endpoints that opt in via :attr:`HTTPEndpoint.IS_RETRYABLE`. + + :param endpoint: The endpoint being called. + :type endpoint: HTTPEndpoint + :param request_data: The prepared request, or ``None`` on the legacy ``send`` path. + :type request_data: Optional[HttpRequest] + :returns: The HTTP response from the last attempt. + :rtype: HTTPResponse + """ + num_retries = 0 + while True: + if request_data is None: + http_response = self.send(endpoint) + else: + http_response = self.send_request(request_data) + + if self._should_retry(endpoint, http_response, num_retries): + time.sleep(self._compute_backoff(http_response, num_retries)) + num_retries += 1 + else: + return http_response + + def _should_retry( + self, endpoint: HTTPEndpoint, http_response: HTTPResponse, num_retries: int + ) -> bool: + """ + Returns True when the endpoint opts into retries, the response is a + transient error and retries remain. + + :param endpoint: The endpoint being called. + :type endpoint: HTTPEndpoint + :param http_response: The response received. + :type http_response: HTTPResponse + :param num_retries: Number of retries already performed. + :type num_retries: int + :returns: Whether the request should be retried. + :rtype: bool + """ + if not endpoint.IS_RETRYABLE: + return False + if num_retries >= self.MAX_RETRIES: + return False + return http_response.status_code in self.RETRYABLE_STATUS_CODES + + def _compute_backoff(self, http_response: HTTPResponse, num_retries: int) -> float: + """ + Computes how long to wait before the next retry. + + :param http_response: The response received. + :type http_response: HTTPResponse + :param num_retries: Number of retries already performed. + :type num_retries: int + :returns: The delay in seconds. + :rtype: float + """ + + headers = {key.lower(): value for key, value in http_response.headers.items()} + retry_after_seconds = self._parse_retry_after(headers.get("retry-after")) + if retry_after_seconds is not None: + return retry_after_seconds + random.uniform(0, 0.25) + + max_delay = self.BACKOFF_BASE_SECONDS * (self.BACKOFF_GROWTH ** num_retries) + return random.uniform(0, max_delay) + + @staticmethod + def _parse_retry_after(value: Optional[str]) -> Optional[float]: + """ + Parses the Retry-After header into a + delay in seconds, or None if absent/unparseable. + + :param value: The raw header value. + :type value: Optional[str] + :returns: The delay in seconds, or None if absent/invalid. + :rtype: Optional[float] + """ + if not value: + return None + + try: + seconds = float(value) + return seconds if seconds >= 0 else None + except ValueError: + pass + + try: + retry_at = parsedate_to_datetime(value) + except (TypeError, ValueError): + return None + if retry_at.tzinfo is None: + retry_at = retry_at.replace(tzinfo=timezone.utc) + return max(0.0, (retry_at - datetime.now(timezone.utc)).total_seconds()) def authenticate(self, endpoint: HTTPEndpoint, request_data: HttpRequest) -> HttpRequest: @@ -215,12 +323,12 @@ def _legacy_request(self, endpoint: HTTPEndpoint) -> HTTPResponse: :rtype: HTTPResponse """ token_before = self.sinch.configuration.token_manager.token - http_response = self.send(endpoint) + http_response = self._send_with_retries(endpoint, request_data=None) if self._should_refresh_token(endpoint, http_response): used_token = token_before.access_token if token_before else None self.sinch.configuration.token_manager.refresh_auth_token(used_token) - http_response = self.send(endpoint) + http_response = self._send_with_retries(endpoint, request_data=None) return endpoint.handle_response(http_response) diff --git a/sinch/domains/authentication/endpoints/v1/oauth.py b/sinch/domains/authentication/endpoints/v1/oauth.py index b8b867c5..f45d5602 100644 --- a/sinch/domains/authentication/endpoints/v1/oauth.py +++ b/sinch/domains/authentication/endpoints/v1/oauth.py @@ -9,6 +9,7 @@ class OAuthEndpoint(HTTPEndpoint): ENDPOINT_URL = "{origin}/oauth2/token" HTTP_METHOD = HTTPMethods.POST.value HTTP_AUTHENTICATION = HTTPAuthentication.BASIC.value + IS_RETRYABLE = True def __init__(self): pass diff --git a/tests/unit/test_http_transport.py b/tests/unit/test_http_transport.py index 2636e9ce..2a39aa42 100644 --- a/tests/unit/test_http_transport.py +++ b/tests/unit/test_http_transport.py @@ -1,4 +1,6 @@ import json +import random +import time import pytest from unittest.mock import Mock from sinch.core.enums import HTTPAuthentication @@ -13,7 +15,7 @@ # Mock classes and fixtures -def _make_mock_endpoint(auth_type, error_on_4xx=False): +def _make_mock_endpoint(auth_type, error_on_4xx=False, is_retryable=False): """Create a MockEndpoint that satisfies the abstract property contract.""" class _Endpoint(HTTPEndpoint): @@ -45,6 +47,7 @@ def handle_response(self, response: HTTPResponse): ) return response + _Endpoint.IS_RETRYABLE = is_retryable return _Endpoint() @@ -92,6 +95,12 @@ def mock_sinch(): return sinch +@pytest.fixture +def no_sleep(mocker): + mocker.patch.object(random, "uniform", return_value=0.0) + return mocker.patch.object(time, "sleep") + + @pytest.fixture def base_request(): return HttpRequest( @@ -276,6 +285,148 @@ def test_no_refresh_on_401_without_www_authenticate(self, mock_sinch): token_manager.refresh_auth_token.assert_not_called() +class TestRetryWithBackoff: + """Tests for the automatic retry-with-backoff on rate-limited (429) responses.""" + + @staticmethod + def _rate_limited(headers=None): + return _requests_response(429, body={"error": "rate limited"}, headers=headers) + + def test_retries_on_429_then_succeeds(self, mock_sinch, no_sleep): + transport = HTTPTransportRequests(mock_sinch) + transport.http_session.request = Mock(side_effect=[ + self._rate_limited(), + self._rate_limited(), + _requests_response(200, body={"ok": True}), + ]) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value, is_retryable=True) + + result = transport.request(endpoint) + + assert result.status_code == 200 + assert transport.http_session.request.call_count == 3 + assert no_sleep.call_count == 2 + + def test_gives_up_and_returns_last_response_after_max_retries(self, mock_sinch, no_sleep): + transport = HTTPTransportRequests(mock_sinch) + transport.http_session.request = Mock(return_value=self._rate_limited()) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value, is_retryable=True) + + result = transport.request(endpoint) + + assert result.status_code == 429 + assert transport.http_session.request.call_count == HTTPTransport.MAX_RETRIES + 1 + assert no_sleep.call_count == HTTPTransport.MAX_RETRIES + + def test_no_retry_on_success(self, mock_sinch, no_sleep): + transport = HTTPTransportRequests(mock_sinch) + transport.http_session.request = Mock(return_value=_requests_response(200, body={"ok": True})) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value) + + result = transport.request(endpoint) + + assert result.status_code == 200 + assert transport.http_session.request.call_count == 1 + no_sleep.assert_not_called() + + def test_no_retry_on_non_retryable_status(self, mock_sinch, no_sleep): + transport = HTTPTransportRequests(mock_sinch) + transport.http_session.request = Mock(return_value=_requests_response(400, body={"error": "bad"})) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value) + + result = transport.request(endpoint) + + assert result.status_code == 400 + assert transport.http_session.request.call_count == 1 + no_sleep.assert_not_called() + + def test_honors_retry_after_header(self, mock_sinch, no_sleep): + transport = HTTPTransportRequests(mock_sinch) + transport.http_session.request = Mock(side_effect=[ + self._rate_limited(headers={"Retry-After": "7"}), + _requests_response(200, body={"ok": True}), + ]) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value, is_retryable=True) + + transport.request(endpoint) + + no_sleep.assert_called_once_with(7.0) + + def test_no_retry_when_endpoint_not_retryable(self, mock_sinch, no_sleep): + transport = HTTPTransportRequests(mock_sinch) + transport.http_session.request = Mock(return_value=self._rate_limited()) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value, is_retryable=False) + + result = transport.request(endpoint) + + assert result.status_code == 429 + assert transport.http_session.request.call_count == 1 + no_sleep.assert_not_called() + + +class TestShouldRetry: + def test_retries_429_while_attempts_remain(self, mock_sinch): + transport = HTTPTransportRequests(mock_sinch) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value, is_retryable=True) + response = HTTPResponse(status_code=429, headers={}, body={}) + + assert transport._should_retry(endpoint, response, num_retries=0) is True + + def test_stops_when_max_retries_reached(self, mock_sinch): + transport = HTTPTransportRequests(mock_sinch) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value, is_retryable=True) + response = HTTPResponse(status_code=429, headers={}, body={}) + + assert transport._should_retry(endpoint, response, num_retries=HTTPTransport.MAX_RETRIES) is False + + def test_does_not_retry_non_retryable_status(self, mock_sinch): + transport = HTTPTransportRequests(mock_sinch) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value, is_retryable=True) + response = HTTPResponse(status_code=200, headers={}, body={}) + + assert transport._should_retry(endpoint, response, num_retries=0) is False + + def test_does_not_retry_when_endpoint_not_retryable(self, mock_sinch): + transport = HTTPTransportRequests(mock_sinch) + endpoint = _make_mock_endpoint(HTTPAuthentication.BASIC.value, is_retryable=False) + response = HTTPResponse(status_code=429, headers={}, body={}) + + assert transport._should_retry(endpoint, response, num_retries=0) is False + + +class TestComputeBackoff: + def test_uses_retry_after_header_when_present(self, mock_sinch): + transport = HTTPTransportRequests(mock_sinch) + response = HTTPResponse(status_code=429, headers={"Retry-After": "5"}, body={}) + + backoff = transport._compute_backoff(response, num_retries=0) + + assert 5.0 <= backoff < 5.25 + + def test_exponential_growth_when_no_header(self, mock_sinch): + transport = HTTPTransportRequests(mock_sinch) + response = HTTPResponse(status_code=429, headers={}, body={}) + + assert 0.0 <= transport._compute_backoff(response, num_retries=0) <= 1.0 + assert 0.0 <= transport._compute_backoff(response, num_retries=1) <= 4.0 + assert 0.0 <= transport._compute_backoff(response, num_retries=2) <= 16.0 + + +class TestParseRetryAfter: + @pytest.mark.parametrize("value,expected", [ + ("5", 5.0), + ("0", 0.0), + ("-3", None), + ("abc", None), + ("", None), + (None, None), + ("Wed, 21 Oct 2015 07:28:00 GMT", 0.0), + ("Wed, 21 Oct 2015 07:28:00", 0.0), + ]) + def test_parse_retry_after(self, value, expected): + assert HTTPTransport._parse_retry_after(value) == expected + + class _LegacyTransport(HTTPTransport): """A pre-2.1 transport that overrides the deprecated ``send(endpoint)`` hook."""