Merge branch 'issue_1244' into 'master'

Fix sleep length must be non-negative

Closes #1244

See merge request allianceauth/allianceauth!1222

allianceauth/services/modules/discord/discord_client/client.py

@@ -46,6 +46,9 @@ DURATION_CONTINGENCY = 500
 # time until next reset is below this threshold
 WAIT_THRESHOLD = 250
 
+# Minimum wait duration when doing a blocking wait
+MINIMUM_BLOCKING_WAIT = 50
+
 # If the rate limit resets soon we will wait it out and then retry to
 # either get a remaining request from our cached counter 
 # or again wait out a short reset time and retry again.
@@ -605,7 +608,10 @@ class DiscordClient:
                 value=RATE_LIMIT_MAX_REQUESTS, 
                 px=RATE_LIMIT_RESETS_AFTER + DURATION_CONTINGENCY
             )                        
-            resets_in = self._redis.pttl(self._KEY_GLOBAL_RATE_LIMIT_REMAINING)
+            resets_in = max(
+                MINIMUM_BLOCKING_WAIT, 
+                self._redis.pttl(self._KEY_GLOBAL_RATE_LIMIT_REMAINING)
+            )
             if requests_remaining >= 0:
                 logger.debug(
                     '%s: Got one of %d remaining requests until reset in %s ms',

allianceauth/services/modules/discord/discord_client/tests/test_client.py

@@ -50,6 +50,12 @@ mock_redis = MagicMock(**{
 })
 
 
+# default mock function to simulate sleep
+def my_sleep(value):
+    if value < 0:
+        raise ValueError('sleep length must be non-negative')
+
+
 class DiscordClient2(DiscordClient):
     """Variant that overwrites lua wrappers with dummies for easier testing"""
 
@@ -1034,6 +1040,42 @@ class TestRateLimitMechanic(TestCase):
         requests_mocker.post(
             f'{API_BASE_URL}guilds/{TEST_GUILD_ID}/roles', json=self.my_role
         )        
+        mock_sleep.side_effect = my_sleep
+        my_mock_redis = MagicMock(**{'pttl.side_effect': my_redis_pttl_2})
+        mock_redis_decr_or_set.side_effect = my_redis_decr_or_set        
+        client = DiscordClient(TEST_BOT_TOKEN, my_mock_redis)
+        
+        result = client.create_guild_role(
+            guild_id=TEST_GUILD_ID, role_name=self.my_role['name']
+        )
+        self.assertDictEqual(result, self.my_role)
+        self.assertTrue(mock_sleep.called)
+
+    @patch(MODULE_PATH + '.sleep')
+    def test_wait_if_reset_happens_soon_and_sleep_must_not_be_negative(
+        self, requests_mocker, mock_sleep, mock_redis_decr_or_set
+    ):        
+        counter = 0
+                
+        def my_redis_pttl_2(name: str):
+            if name == DiscordClient._KEY_GLOBAL_BACKOFF_UNTIL:
+                return -1
+            else:
+                return -1
+        
+        def my_redis_decr_or_set(**kwargs):
+            nonlocal counter
+            counter += 1
+
+            if counter < 2:
+                return -1
+            else:
+                return 5
+
+        requests_mocker.post(
+            f'{API_BASE_URL}guilds/{TEST_GUILD_ID}/roles', json=self.my_role
+        )        
+        mock_sleep.side_effect = my_sleep
         my_mock_redis = MagicMock(**{'pttl.side_effect': my_redis_pttl_2})
         mock_redis_decr_or_set.side_effect = my_redis_decr_or_set
         client = DiscordClient(TEST_BOT_TOKEN, my_mock_redis)
@@ -1075,6 +1117,7 @@ class TestRateLimitMechanic(TestCase):
         requests_mocker.post(
             f'{API_BASE_URL}guilds/{TEST_GUILD_ID}/roles', json=self.my_role
         )        
+        mock_sleep.side_effect = my_sleep
         my_mock_redis = MagicMock(**{'pttl.side_effect': my_redis_pttl_2})
         mock_redis_decr_or_set.return_value = -1
         client = DiscordClient(TEST_BOT_TOKEN, my_mock_redis)
@@ -1209,6 +1252,7 @@ class TestBackoffHandling(TestCase):
             f'{API_BASE_URL}guilds/{TEST_GUILD_ID}/roles', json=self.my_role
         )
         retry_after = 50
+        mock_sleep.side_effect = my_sleep
         my_mock_redis = MagicMock(**{'pttl.return_value': retry_after})
         mock_redis_decr_or_set.return_value = 5
         client = DiscordClient(TEST_BOT_TOKEN, my_mock_redis)