Merge pull request #192 from praw-dev/last_request

Fix occasional bug with 0 requests remaining and 0 seconds before reset

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)
 -------------------

prawcore/auth.py

@@ -142,21 +142,23 @@ 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?
             raise OAuthException(
                 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(" "))
 

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(

prawcore/rate_limit.py

@@ -11,6 +11,8 @@
 
     from requests.models import Response
 
+from prawcore.const import NANOSECONDS
+
 log = logging.getLogger(__package__)
 
 
@@ -23,9 +25,8 @@ class RateLimiter:
 
     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,11 @@ 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()) / NANOSECONDS
+        )
         if sleep_seconds <= 0:
             return
         message = f"Sleeping: {sleep_seconds:0.2f} seconds prior to call"
@@ -78,29 +81,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(
+                NANOSECONDS / 2, seconds_to_reset * 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,
-            ),
+            )
+            * NANOSECONDS
         )

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:

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 = 100
+        rate_limiter.next_request_timestamp_ns = 100 * NANOSECONDS
         return rate_limiter
 
     @staticmethod
@@ -25,93 +26,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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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