From fde5f291fba8606d7538912c9c722ee6f688f53c Mon Sep 17 00:00:00 2001 From: Bryce Boe Date: Fri, 24 Jan 2025 15:42:09 -0800 Subject: [PATCH 1/2] Sleep at least half a second when no requests remain --- prawcore/rate_limit.py | 38 +++++++++------ tests/unit/test_rate_limit.py | 90 +++++++++++++++++++++-------------- 2 files changed, 76 insertions(+), 52 deletions(-) diff --git a/prawcore/rate_limit.py b/prawcore/rate_limit.py index a21bd95..6ca618f 100644 --- a/prawcore/rate_limit.py +++ b/prawcore/rate_limit.py @@ -21,11 +21,12 @@ class RateLimiter: """ + NANOSECONDS = 1_000_000_000 + def __init__(self, *, window_size: int): """Create an instance of the RateLimit class.""" - self.remaining: float | None = None - self.next_request_timestamp: float | None = None - self.reset_timestamp: float | None = None + self.remaining: int | None = None + self.next_request_timestamp_ns: int | None = None self.used: int | None = None self.window_size: int = window_size @@ -53,9 +54,12 @@ def call( def delay(self): """Sleep for an amount of time to remain under the rate limit.""" - if self.next_request_timestamp is None: + if self.next_request_timestamp_ns is None: return - sleep_seconds = self.next_request_timestamp - time.time() + sleep_seconds = ( + float(self.next_request_timestamp_ns - time.monotonic_ns()) + / self.NANOSECONDS + ) if sleep_seconds <= 0: return message = f"Sleeping: {sleep_seconds:0.2f} seconds prior to call" @@ -78,29 +82,33 @@ def update(self, response_headers: Mapping[str, str]): self.used += 1 return - now = time.time() + self.remaining = int(float(response_headers["x-ratelimit-remaining"])) + self.used = int(response_headers["x-ratelimit-used"]) + now_ns = time.monotonic_ns() seconds_to_reset = int(response_headers["x-ratelimit-reset"]) - self.remaining = float(response_headers["x-ratelimit-remaining"]) - self.used = int(response_headers["x-ratelimit-used"]) - self.reset_timestamp = now + seconds_to_reset if self.remaining <= 0: - self.next_request_timestamp = self.reset_timestamp + self.next_request_timestamp_ns = now_ns + max( + self.NANOSECONDS / 2, seconds_to_reset * self.NANOSECONDS + ) return - self.next_request_timestamp = min( - self.reset_timestamp, - now + self.next_request_timestamp_ns = ( + now_ns + min( + seconds_to_reset, max( seconds_to_reset - ( self.window_size - - (self.window_size / (self.remaining + self.used) * self.used) + - self.window_size + / (float(self.remaining) + self.used) + * self.used ), 0, ), 10, - ), + ) + * self.NANOSECONDS ) diff --git a/tests/unit/test_rate_limit.py b/tests/unit/test_rate_limit.py index cb561bd..ba38afe 100644 --- a/tests/unit/test_rate_limit.py +++ b/tests/unit/test_rate_limit.py @@ -14,7 +14,7 @@ class TestRateLimiter(UnitTest): @pytest.fixture def rate_limiter(self): rate_limiter = RateLimiter(window_size=600) - rate_limiter.next_request_timestamp = 100 + rate_limiter.next_request_timestamp_ns = 100 * RateLimiter.NANOSECONDS return rate_limiter @staticmethod @@ -25,93 +25,109 @@ def _headers(remaining, used, reset): "x-ratelimit-reset": str(reset), } - @patch("time.time") + @patch("time.monotonic_ns") @patch("time.sleep") - def test_delay(self, mock_sleep, mock_time, rate_limiter): - mock_time.return_value = 1 + def test_delay(self, mock_sleep, mock_monotonic_ns, rate_limiter): + mock_monotonic_ns.return_value = 1 * RateLimiter.NANOSECONDS rate_limiter.delay() - assert mock_time.called + assert mock_monotonic_ns.called mock_sleep.assert_called_with(99) - @patch("time.time") + @patch("time.monotonic_ns") @patch("time.sleep") def test_delay__no_sleep_when_time_in_past( - self, mock_sleep, mock_time, rate_limiter + self, mock_sleep, mock_monotonic_ns, rate_limiter ): - mock_time.return_value = 101 + mock_monotonic_ns.return_value = 101 * RateLimiter.NANOSECONDS rate_limiter.delay() - assert mock_time.called + assert mock_monotonic_ns.called assert not mock_sleep.called @patch("time.sleep") def test_delay__no_sleep_when_time_is_not_set(self, mock_sleep, rate_limiter): + rate_limiter.next_request_timestamp_ns = None rate_limiter.delay() assert not mock_sleep.called - @patch("time.time") + @patch("time.monotonic_ns") @patch("time.sleep") def test_delay__no_sleep_when_times_match( - self, mock_sleep, mock_time, rate_limiter + self, mock_sleep, mock_monotonic_ns, rate_limiter ): - mock_time.return_value = 100 + mock_monotonic_ns.return_value = 100 * RateLimiter.NANOSECONDS rate_limiter.delay() - assert mock_time.called + assert mock_monotonic_ns.called assert not mock_sleep.called - @patch("time.time") - def test_update__compute_delay_with_no_previous_info(self, mock_time, rate_limiter): - mock_time.return_value = 100 + @patch("time.monotonic_ns") + def test_update__compute_delay_with_no_previous_info( + self, mock_monotonic_ns, rate_limiter + ): + mock_monotonic_ns.return_value = 100 * RateLimiter.NANOSECONDS rate_limiter.update(self._headers(60, 100, 60)) assert rate_limiter.remaining == 60 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp == 100 + assert rate_limiter.next_request_timestamp_ns == 100 * RateLimiter.NANOSECONDS - @patch("time.time") - def test_update__compute_delay_with_single_client(self, mock_time, rate_limiter): - rate_limiter.remaining = 61 + @patch("time.monotonic_ns") + def test_update__compute_delay_with_single_client( + self, mock_monotonic_ns, rate_limiter + ): rate_limiter.window_size = 150 - mock_time.return_value = 100 + mock_monotonic_ns.return_value = 100 * RateLimiter.NANOSECONDS rate_limiter.update(self._headers(50, 100, 60)) assert rate_limiter.remaining == 50 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp == 110 + assert rate_limiter.next_request_timestamp_ns == 110 * RateLimiter.NANOSECONDS - @patch("time.time") - def test_update__compute_delay_with_six_clients(self, mock_time, rate_limiter): + @patch("time.monotonic_ns") + def test_update__compute_delay_with_six_clients( + self, mock_monotonic_ns, rate_limiter + ): rate_limiter.remaining = 66 rate_limiter.window_size = 180 - mock_time.return_value = 100 + mock_monotonic_ns.return_value = 100 * RateLimiter.NANOSECONDS rate_limiter.update(self._headers(60, 100, 72)) assert rate_limiter.remaining == 60 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp == 104.5 + assert rate_limiter.next_request_timestamp_ns == 104.5 * RateLimiter.NANOSECONDS - @patch("time.time") + @patch("time.monotonic_ns") def test_update__delay_full_time_with_negative_remaining( - self, mock_time, rate_limiter + self, mock_monotonic_ns, rate_limiter ): - mock_time.return_value = 37 - rate_limiter.remaining = -1 + mock_monotonic_ns.return_value = 37 * RateLimiter.NANOSECONDS rate_limiter.update(self._headers(0, 100, 13)) assert rate_limiter.remaining == 0 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp == 50 + assert rate_limiter.next_request_timestamp_ns == 50 * RateLimiter.NANOSECONDS - @patch("time.time") - def test_update__delay_full_time_with_zero_remaining(self, mock_time, rate_limiter): - mock_time.return_value = 37 - rate_limiter.remaining = 0 + @patch("time.monotonic_ns") + def test_update__delay_full_time_with_zero_remaining( + self, mock_monotonic_ns, rate_limiter + ): + mock_monotonic_ns.return_value = 37 * RateLimiter.NANOSECONDS rate_limiter.update(self._headers(0, 100, 13)) assert rate_limiter.remaining == 0 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp == 50 + assert rate_limiter.next_request_timestamp_ns == 50 * RateLimiter.NANOSECONDS + + @patch("time.monotonic_ns") + def test_update__delay_full_time_with_zero_remaining_and_no_sleep_time( + self, mock_monotonic_ns, rate_limiter + ): + mock_monotonic_ns.return_value = 37 * RateLimiter.NANOSECONDS + rate_limiter.update(self._headers(0, 100, 0)) + assert rate_limiter.remaining == 0 + assert rate_limiter.used == 100 + assert rate_limiter.next_request_timestamp_ns == 37.5 * RateLimiter.NANOSECONDS def test_update__no_change_without_headers(self, rate_limiter): prev = copy(rate_limiter) rate_limiter.update({}) assert prev.remaining == rate_limiter.remaining assert prev.used == rate_limiter.used - assert rate_limiter.next_request_timestamp == prev.next_request_timestamp + assert rate_limiter.next_request_timestamp_ns == prev.next_request_timestamp_ns def test_update__values_change_without_headers(self, rate_limiter): rate_limiter.remaining = 10 From ca482573caf48eca73ca5e08ad092c981bad0181 Mon Sep 17 00:00:00 2001 From: Bryce Boe Date: Fri, 24 Jan 2025 16:44:51 -0800 Subject: [PATCH 2/2] Replace uses of time.time with time.monotonic(_ns) --- CHANGES.rst | 19 ++++++++++++++----- prawcore/auth.py | 15 ++++++++++----- prawcore/const.py | 1 + prawcore/rate_limit.py | 11 +++++------ prawcore/sessions.py | 4 ++-- tests/unit/test_rate_limit.py | 33 +++++++++++++++++---------------- 6 files changed, 49 insertions(+), 34 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 90ca57f..854ebe8 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -9,6 +9,18 @@ Unreleased **Changed** - Drop support for Python 3.8, which was end-of-life on 2024-10-07. +- :class:`RateLimiter` attribute ``next_request_timestamp`` has been removed and + replaced with ``next_request_timestamp_ns``. + +**Fixed** + +- Add a half-second delay when there are no more requests in the rate limit window and + the window has zero seconds remaining to avoid a semi-rare case where Reddit will + return a 429 response resulting in a :class:`TooManyRequests` exception. + +**Removed** + +- Remove :class:`RateLimiter` attribute ``reset_timestamp``. 2.4.0 (2023/10/01) ------------------ @@ -114,9 +126,6 @@ Unreleased - Updated rate limit algorithm to more intelligently rate limit when there are extra requests remaining. - -**Removed** - - Drop python 2.7 support. 1.0.1 (2019-02-05) @@ -150,9 +159,9 @@ changes will need to be introduced in the near future. - ``ReadTimeout`` is automatically retried like the server errors. -**Removed** +**Changed** -- Removed support for Python 3.3 as it is no longer supported by requests. +- Drop support for Python 3.3 as it is no longer supported by requests. 0.13.0 (2017-12-16) ------------------- diff --git a/prawcore/auth.py b/prawcore/auth.py index 2c977bc..101daf4 100644 --- a/prawcore/auth.py +++ b/prawcore/auth.py @@ -142,13 +142,13 @@ def __init__(self, authenticator: BaseAuthenticator): self._validate_authenticator() def _clear_access_token(self): - self._expiration_timestamp: float + self._expiration_timestamp_ns: int self.access_token: str | None = None self.scopes: set[str] | None = None def _request_token(self, **data: Any): url = self._authenticator._requestor.reddit_url + const.ACCESS_TOKEN_PATH - pre_request_time = time.time() + pre_request_timestamp_ns = time.monotonic_ns() response = self._authenticator._post(url=url, **data) payload = response.json() if "error" in payload: # Why are these OKAY responses? @@ -156,7 +156,9 @@ def _request_token(self, **data: Any): response, payload["error"], payload.get("error_description") ) - self._expiration_timestamp = pre_request_time - 10 + payload["expires_in"] + self._expiration_timestamp_ns = ( + pre_request_timestamp_ns + (payload["expires_in"] + 10) * const.NANOSECONDS + ) self.access_token = payload["access_token"] if "refresh_token" in payload: self.refresh_token = payload["refresh_token"] @@ -181,7 +183,8 @@ def is_valid(self) -> bool: """ return ( - self.access_token is not None and time.time() < self._expiration_timestamp + self.access_token is not None + and time.monotonic_ns() < self._expiration_timestamp_ns ) def revoke(self): @@ -338,7 +341,9 @@ def __init__( """ super().__init__(authenticator) - self._expiration_timestamp = time.time() + expires_in + self._expiration_timestamp_ns = ( + time.monotonic_ns() + expires_in * const.NANOSECONDS + ) self.access_token = access_token self.scopes = set(scope.split(" ")) diff --git a/prawcore/const.py b/prawcore/const.py index 1173efc..e034eab 100644 --- a/prawcore/const.py +++ b/prawcore/const.py @@ -4,6 +4,7 @@ ACCESS_TOKEN_PATH = "/api/v1/access_token" # noqa: S105 AUTHORIZATION_PATH = "/api/v1/authorize" # noqa: S105 +NANOSECONDS = 1_000_000_000 # noqa: S105 REVOKE_TOKEN_PATH = "/api/v1/revoke_token" # noqa: S105 TIMEOUT = float( os.environ.get( diff --git a/prawcore/rate_limit.py b/prawcore/rate_limit.py index 6ca618f..59f924c 100644 --- a/prawcore/rate_limit.py +++ b/prawcore/rate_limit.py @@ -11,6 +11,8 @@ from requests.models import Response +from prawcore.const import NANOSECONDS + log = logging.getLogger(__package__) @@ -21,8 +23,6 @@ class RateLimiter: """ - NANOSECONDS = 1_000_000_000 - def __init__(self, *, window_size: int): """Create an instance of the RateLimit class.""" self.remaining: int | None = None @@ -57,8 +57,7 @@ def delay(self): if self.next_request_timestamp_ns is None: return sleep_seconds = ( - float(self.next_request_timestamp_ns - time.monotonic_ns()) - / self.NANOSECONDS + float(self.next_request_timestamp_ns - time.monotonic_ns()) / NANOSECONDS ) if sleep_seconds <= 0: return @@ -90,7 +89,7 @@ def update(self, response_headers: Mapping[str, str]): if self.remaining <= 0: self.next_request_timestamp_ns = now_ns + max( - self.NANOSECONDS / 2, seconds_to_reset * self.NANOSECONDS + NANOSECONDS / 2, seconds_to_reset * NANOSECONDS ) return @@ -110,5 +109,5 @@ def update(self, response_headers: Mapping[str, str]): ), 10, ) - * self.NANOSECONDS + * NANOSECONDS ) diff --git a/prawcore/sessions.py b/prawcore/sessions.py index 3e2a28c..dca273d 100644 --- a/prawcore/sessions.py +++ b/prawcore/sessions.py @@ -109,7 +109,7 @@ def _log_request( params: dict[str, int], url: str, ): - log.debug("Fetching: %s %s at %s", method, url, time.time()) + log.debug("Fetching: %s %s at %s", method, url, time.monotonic()) log.debug("Data: %s", pformat(data)) log.debug("Params: %s", pformat(params)) @@ -201,7 +201,7 @@ def _make_request( response.headers.get("x-ratelimit-reset"), response.headers.get("x-ratelimit-remaining"), response.headers.get("x-ratelimit-used"), - time.time(), + time.monotonic(), ) return response, None except RequestException as exception: diff --git a/tests/unit/test_rate_limit.py b/tests/unit/test_rate_limit.py index ba38afe..8a0ac09 100644 --- a/tests/unit/test_rate_limit.py +++ b/tests/unit/test_rate_limit.py @@ -5,6 +5,7 @@ import pytest +from prawcore.const import NANOSECONDS from prawcore.rate_limit import RateLimiter from . import UnitTest @@ -14,7 +15,7 @@ class TestRateLimiter(UnitTest): @pytest.fixture def rate_limiter(self): rate_limiter = RateLimiter(window_size=600) - rate_limiter.next_request_timestamp_ns = 100 * RateLimiter.NANOSECONDS + rate_limiter.next_request_timestamp_ns = 100 * NANOSECONDS return rate_limiter @staticmethod @@ -28,7 +29,7 @@ def _headers(remaining, used, reset): @patch("time.monotonic_ns") @patch("time.sleep") def test_delay(self, mock_sleep, mock_monotonic_ns, rate_limiter): - mock_monotonic_ns.return_value = 1 * RateLimiter.NANOSECONDS + mock_monotonic_ns.return_value = 1 * NANOSECONDS rate_limiter.delay() assert mock_monotonic_ns.called mock_sleep.assert_called_with(99) @@ -38,7 +39,7 @@ def test_delay(self, mock_sleep, mock_monotonic_ns, rate_limiter): def test_delay__no_sleep_when_time_in_past( self, mock_sleep, mock_monotonic_ns, rate_limiter ): - mock_monotonic_ns.return_value = 101 * RateLimiter.NANOSECONDS + mock_monotonic_ns.return_value = 101 * NANOSECONDS rate_limiter.delay() assert mock_monotonic_ns.called assert not mock_sleep.called @@ -54,7 +55,7 @@ def test_delay__no_sleep_when_time_is_not_set(self, mock_sleep, rate_limiter): def test_delay__no_sleep_when_times_match( self, mock_sleep, mock_monotonic_ns, rate_limiter ): - mock_monotonic_ns.return_value = 100 * RateLimiter.NANOSECONDS + mock_monotonic_ns.return_value = 100 * NANOSECONDS rate_limiter.delay() assert mock_monotonic_ns.called assert not mock_sleep.called @@ -63,22 +64,22 @@ def test_delay__no_sleep_when_times_match( def test_update__compute_delay_with_no_previous_info( self, mock_monotonic_ns, rate_limiter ): - mock_monotonic_ns.return_value = 100 * RateLimiter.NANOSECONDS + mock_monotonic_ns.return_value = 100 * NANOSECONDS rate_limiter.update(self._headers(60, 100, 60)) assert rate_limiter.remaining == 60 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp_ns == 100 * RateLimiter.NANOSECONDS + assert rate_limiter.next_request_timestamp_ns == 100 * NANOSECONDS @patch("time.monotonic_ns") def test_update__compute_delay_with_single_client( self, mock_monotonic_ns, rate_limiter ): rate_limiter.window_size = 150 - mock_monotonic_ns.return_value = 100 * RateLimiter.NANOSECONDS + mock_monotonic_ns.return_value = 100 * NANOSECONDS rate_limiter.update(self._headers(50, 100, 60)) assert rate_limiter.remaining == 50 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp_ns == 110 * RateLimiter.NANOSECONDS + assert rate_limiter.next_request_timestamp_ns == 110 * NANOSECONDS @patch("time.monotonic_ns") def test_update__compute_delay_with_six_clients( @@ -86,41 +87,41 @@ def test_update__compute_delay_with_six_clients( ): rate_limiter.remaining = 66 rate_limiter.window_size = 180 - mock_monotonic_ns.return_value = 100 * RateLimiter.NANOSECONDS + mock_monotonic_ns.return_value = 100 * NANOSECONDS rate_limiter.update(self._headers(60, 100, 72)) assert rate_limiter.remaining == 60 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp_ns == 104.5 * RateLimiter.NANOSECONDS + assert rate_limiter.next_request_timestamp_ns == 104.5 * NANOSECONDS @patch("time.monotonic_ns") def test_update__delay_full_time_with_negative_remaining( self, mock_monotonic_ns, rate_limiter ): - mock_monotonic_ns.return_value = 37 * RateLimiter.NANOSECONDS + mock_monotonic_ns.return_value = 37 * NANOSECONDS rate_limiter.update(self._headers(0, 100, 13)) assert rate_limiter.remaining == 0 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp_ns == 50 * RateLimiter.NANOSECONDS + assert rate_limiter.next_request_timestamp_ns == 50 * NANOSECONDS @patch("time.monotonic_ns") def test_update__delay_full_time_with_zero_remaining( self, mock_monotonic_ns, rate_limiter ): - mock_monotonic_ns.return_value = 37 * RateLimiter.NANOSECONDS + mock_monotonic_ns.return_value = 37 * NANOSECONDS rate_limiter.update(self._headers(0, 100, 13)) assert rate_limiter.remaining == 0 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp_ns == 50 * RateLimiter.NANOSECONDS + assert rate_limiter.next_request_timestamp_ns == 50 * NANOSECONDS @patch("time.monotonic_ns") def test_update__delay_full_time_with_zero_remaining_and_no_sleep_time( self, mock_monotonic_ns, rate_limiter ): - mock_monotonic_ns.return_value = 37 * RateLimiter.NANOSECONDS + mock_monotonic_ns.return_value = 37 * NANOSECONDS rate_limiter.update(self._headers(0, 100, 0)) assert rate_limiter.remaining == 0 assert rate_limiter.used == 100 - assert rate_limiter.next_request_timestamp_ns == 37.5 * RateLimiter.NANOSECONDS + assert rate_limiter.next_request_timestamp_ns == 37.5 * NANOSECONDS def test_update__no_change_without_headers(self, rate_limiter): prev = copy(rate_limiter)