Improve #1084 to run rate limited error retry handler correctly (#1094)

Author: seratch

integration_tests/web/test_admin_rate_limit_retries.py

@@ -0,0 +1,44 @@
+import logging
+import os
+import unittest
+
+import pytest
+
+from integration_tests.env_variable_names import (
+    SLACK_SDK_TEST_GRID_ORG_ADMIN_USER_TOKEN,
+)
+from integration_tests.helpers import async_test, is_not_specified
+from slack_sdk.http_retry import RateLimitErrorRetryHandler
+from slack_sdk.http_retry.builtin_async_handlers import AsyncRateLimitErrorRetryHandler
+from slack_sdk.web import WebClient
+from slack_sdk.web.async_client import AsyncWebClient
+
+
+class TestWebClient(unittest.TestCase):
+    """Runs integration tests with real Slack API"""
+
+    def setUp(self):
+        self.logger = logging.getLogger(__name__)
+        self.org_admin_token = os.environ[SLACK_SDK_TEST_GRID_ORG_ADMIN_USER_TOKEN]
+        self.sync_client: WebClient = WebClient(token=self.org_admin_token)
+        self.sync_client.retry_handlers.append(RateLimitErrorRetryHandler(max_retry_count=2))
+        self.async_client: AsyncWebClient = AsyncWebClient(token=self.org_admin_token)
+        self.async_client.retry_handlers.append(
+            AsyncRateLimitErrorRetryHandler(max_retry_count=2)
+        )
+
+    def tearDown(self):
+        pass
+
+    @pytest.mark.skipif(condition=is_not_specified(), reason="execution can take long")
+    def test_sync(self):
+        client = self.sync_client
+        for response in client.admin_users_session_list(limit=1):
+            self.assertIsNotNone(response.get("active_sessions"))
+
+    @pytest.mark.skipif(condition=is_not_specified(), reason="execution can take long")
+    @async_test
+    async def test_async(self):
+        client = self.async_client
+        async for response in await client.admin_users_session_list(limit=1):
+            self.assertIsNotNone(response.get("active_sessions"))

slack_sdk/http_retry/builtin_async_handlers.py

@@ -70,7 +70,7 @@ async def prepare_for_next_attempt_async(
         if response is None:
             raise error
 
-        state.increment_current_attempt()
+        state.next_attempt_requested = True
         retry_after_header_name: Optional[str] = None
         for k in response.headers.keys():
             if k.lower() == "retry-after":
@@ -85,6 +85,7 @@ async def prepare_for_next_attempt_async(
                 int(response.headers.get(retry_after_header_name)[0]) + random.random()
             )
         await asyncio.sleep(duration)
+        state.increment_current_attempt()
 
 
 def async_default_handlers() -> List[AsyncRetryHandler]:

slack_sdk/http_retry/builtin_handlers.py

@@ -39,6 +39,10 @@ def _can_retry(
         if error is None:
             return False
 
+        if isinstance(error, URLError):
+            if response is not None:
+                return False  # status 40x
+
         for error_type in self.error_types_to_do_retries:
             if isinstance(error, error_type):
                 return True
@@ -69,7 +73,7 @@ def prepare_for_next_attempt(
         if response is None:
             raise error
 
-        state.increment_current_attempt()
+        state.next_attempt_requested = True
         retry_after_header_name: Optional[str] = None
         for k in response.headers.keys():
             if k.lower() == "retry-after":
@@ -84,3 +88,4 @@ def prepare_for_next_attempt(
                 int(response.headers.get(retry_after_header_name)[0]) + random.random()
             )
         time.sleep(duration)
+        state.increment_current_attempt()

slack_sdk/http_retry/response.py

@@ -12,12 +12,12 @@ class HttpResponse:
     def __init__(
         self,
         *,
-        status_code: int,
+        status_code: Union[int, str],
         headers: Dict[str, Union[str, List[str]]],
         body: Optional[Dict[str, Any]] = None,
         data: Optional[bytes] = None,
     ):
-        self.status_code = status_code
+        self.status_code = int(status_code)
         self.headers = {
             k: v if isinstance(v, list) else [v] for k, v in headers.items()
         }

slack_sdk/web/async_internal_utils.py

@@ -143,6 +143,15 @@ def convert_params(values: dict) -> dict:
                                     res,
                                 )
 
+                    if logger.level <= logging.DEBUG:
+                        body = data if isinstance(data, dict) else "(binary)"
+                        logger.debug(
+                            "Received the following response - "
+                            f"status: {res.status}, "
+                            f"headers: {dict(res.headers)}, "
+                            f"body: {body}"
+                        )
+
                     if res.status == 429:
                         for handler in retry_handlers:
                             if await handler.can_retry_async(

slack_sdk/web/base_client.py

@@ -522,10 +522,24 @@ def _perform_urllib_http_request_internal(
             if resp.headers.get_content_type() == "application/gzip":
                 # admin.analytics.getFile
                 body: bytes = resp.read()
+                if self._logger.level <= logging.DEBUG:
+                    self._logger.debug(
+                        "Received the following response - "
+                        f"status: {resp.code}, "
+                        f"headers: {dict(resp.headers)}, "
+                        f"body: (binary)"
+                    )
                 return {"status": resp.code, "headers": resp.headers, "body": body}
 
             charset = resp.headers.get_content_charset() or "utf-8"
             body: str = resp.read().decode(charset)  # read the response body here
+            if self._logger.level <= logging.DEBUG:
+                self._logger.debug(
+                    "Received the following response - "
+                    f"status: {resp.code}, "
+                    f"headers: {dict(resp.headers)}, "
+                    f"body: {body}"
+                )
             return {"status": resp.code, "headers": resp.headers, "body": body}
         raise SlackRequestError(f"Invalid URL detected: {url}")
 

slack_sdk/web/slack_response.py

@@ -186,14 +186,6 @@ def validate(self):
         Raises:
             SlackApiError: The request to the Slack API failed.
         """
-        if self._logger.level <= logging.DEBUG:
-            body = self.data if isinstance(self.data, dict) else "(binary)"
-            self._logger.debug(
-                "Received the following response - "
-                f"status: {self.status_code}, "
-                f"headers: {dict(self.headers)}, "
-                f"body: {body}"
-            )
         if (
             self.status_code == 200
             and self.data