Fix #1333 Enable using RetryHandler for 200 OK response patterns (#1334)

Author: seratch

integration_tests/web/test_admin_analytics.py

@@ -28,7 +28,7 @@ def tearDown(self):
     def test_sync(self):
         client = self.sync_client
 
-        response = client.admin_analytics_getFile(date="2020-10-20", type="member")
+        response = client.admin_analytics_getFile(date="2022-10-20", type="member")
         self.assertTrue(isinstance(response.data, bytes))
         self.assertIsNotNone(response.data)
 
@@ -44,7 +44,7 @@ def test_sync_error(self):
     def test_sync_public_channel(self):
         client = self.sync_client
 
-        response = client.admin_analytics_getFile(date="2020-10-20", type="public_channel")
+        response = client.admin_analytics_getFile(date="2022-10-20", type="public_channel")
         self.assertTrue(isinstance(response.data, bytes))
         self.assertIsNotNone(response.data)
 
@@ -59,7 +59,7 @@ def test_sync_public_channel_medata_only(self):
     async def test_async(self):
         client = self.async_client
 
-        response = await client.admin_analytics_getFile(date="2020-10-20", type="member")
+        response = await client.admin_analytics_getFile(date="2022-10-20", type="member")
         self.assertTrue(isinstance(response.data, bytes))
         self.assertIsNotNone(response.data)
 
@@ -77,7 +77,7 @@ async def test_async_error(self):
     async def test_async_public_channel(self):
         client = self.async_client
 
-        response = await client.admin_analytics_getFile(date="2020-10-20", type="public_channel")
+        response = await client.admin_analytics_getFile(date="2022-10-20", type="public_channel")
         self.assertTrue(isinstance(response.data, bytes))
         self.assertIsNotNone(response.data)
 
@@ -95,14 +95,14 @@ async def test_async_public_channel_metadata_only(self):
     def test_legacy(self):
         client = self.legacy_client
 
-        response = client.admin_analytics_getFile(date="2020-10-20", type="member")
+        response = client.admin_analytics_getFile(date="2022-10-20", type="member")
         self.assertTrue(isinstance(response.data, bytes))
         self.assertIsNotNone(response.data)
 
     def test_legacy_public_channel(self):
         client = self.legacy_client
 
-        response = client.admin_analytics_getFile(date="2020-10-20", type="public_channel")
+        response = client.admin_analytics_getFile(date="2022-10-20", type="public_channel")
         self.assertTrue(isinstance(response.data, bytes))
         self.assertIsNotNone(response.data)
 

integration_tests/web/test_issue_594.py

@@ -40,7 +40,7 @@ def test_issue_594(self):
             external_id=external_id,
             external_url=external_url,
             title="Good Old Slack Logo",
-            indexable_file_contents="Good Old Slack Logo",
+            indexable_file_contents="Good Old Slack Logo".encode("utf-8"),
             preview_image=image,
         )
         self.assertIsNotNone(creation)
@@ -76,7 +76,7 @@ def test_no_preview_image(self):
             external_id=external_id,
             external_url=external_url,
             title="Slack (Wikipedia)",
-            indexable_file_contents="Slack is a proprietary business communication platform developed by Slack Technologies.",
+            indexable_file_contents="Slack is a proprietary business communication platform developed by Slack Technologies.".encode("utf-8"),
         )
         self.assertIsNotNone(creation)
 

slack_sdk/web/async_internal_utils.py

@@ -147,24 +147,20 @@ def convert_params(values: dict) -> dict:
                             f"body: {body}"
                         )
 
-                    if res.status == 429:
-                        for handler in retry_handlers:
-                            if await handler.can_retry_async(
+                    for handler in retry_handlers:
+                        if await handler.can_retry_async(
+                            state=retry_state,
+                            request=retry_request,
+                            response=retry_response,
+                        ):
+                            if logger.level <= logging.DEBUG:
+                                logger.info(f"A retry handler found: {type(handler).__name__} " f"for {http_verb} {api_url}")
+                            await handler.prepare_for_next_attempt_async(
                                 state=retry_state,
                                 request=retry_request,
                                 response=retry_response,
-                            ):
-                                if logger.level <= logging.DEBUG:
-                                    logger.info(
-                                        f"A retry handler found: {type(handler).__name__} "
-                                        f"for {http_verb} {api_url} - rate_limited"
-                                    )
-                                await handler.prepare_for_next_attempt_async(
-                                    state=retry_state,
-                                    request=retry_request,
-                                    response=retry_response,
-                                )
-                                break
+                            )
+                            break
 
                     if retry_state.next_attempt_requested is False:
                         response = {

slack_sdk/web/base_client.py

@@ -396,7 +396,29 @@ def _perform_urllib_http_request(self, *, url: str, args: Dict[str, Dict[str, An
             try:
                 resp = self._perform_urllib_http_request_internal(url, req)
                 # The resp is a 200 OK response
-                return resp
+                if len(self.retry_handlers) > 0:
+                    retry_request = RetryHttpRequest.from_urllib_http_request(req)
+                    body_string = resp["body"] if isinstance(resp["body"], str) else None
+                    body_bytes = body_string.encode("utf-8") if body_string is not None else resp["body"]
+                    body = json.loads(body_string) if body_string is not None and body_string.startswith("{") else {}
+                    retry_response = RetryHttpResponse(
+                        status_code=resp["status"],
+                        headers=resp["headers"],
+                        body=body,
+                        data=body_bytes,
+                    )
+                    for handler in self.retry_handlers:
+                        if handler.can_retry(state=retry_state, request=retry_request, response=retry_response):
+                            if self._logger.level <= logging.DEBUG:
+                                self._logger.info(
+                                    f"A retry handler found: {type(handler).__name__} for {req.method} {req.full_url}"
+                                )
+                            handler.prepare_for_next_attempt(
+                                state=retry_state, request=retry_request, response=retry_response
+                            )
+                            break
+                if retry_state.next_attempt_requested is False:
+                    return resp
 
             except HTTPError as e:
                 # As adding new values to HTTPError#headers can be ignored, building a new dict object here

tests/slack_sdk/audit_logs/test_client_http_retry.py

@@ -31,7 +31,7 @@ def test_retries(self):
 
         self.assertEqual(2, retry_handler.call_count)
 
-    def test_rate_limited(self):
+    def test_ratelimited(self):
         client = AuditLogsClient(
             token="xoxp-ratelimited",
             base_url="http://localhost:8888/",

tests/slack_sdk/fatal_error_retry_handler.py

@@ -0,0 +1,28 @@
+from typing import Optional
+
+from slack_sdk.http_retry.interval_calculator import RetryIntervalCalculator
+from slack_sdk.http_retry.state import RetryState
+from slack_sdk.http_retry.request import HttpRequest
+from slack_sdk.http_retry.response import HttpResponse
+from slack_sdk.http_retry.handler import RetryHandler, default_interval_calculator
+
+
+class FatalErrorRetryHandler(RetryHandler):
+    def __init__(
+        self,
+        max_retry_count: int = 1,
+        interval_calculator: RetryIntervalCalculator = default_interval_calculator,
+    ):
+        super().__init__(max_retry_count, interval_calculator)
+        self.call_count = 0
+
+    def _can_retry(
+        self,
+        *,
+        state: RetryState,
+        request: HttpRequest,
+        response: Optional[HttpResponse],
+        error: Optional[Exception],
+    ) -> bool:
+        self.call_count += 1
+        return response is not None and response.status_code == 200 and response.body.get("error") == "fatal_error"

tests/slack_sdk/web/mock_web_api_server.py

@@ -28,7 +28,7 @@ class MockHandler(SimpleHTTPRequestHandler):
 
     error_html_response_body = '<!DOCTYPE html>\n<html lang="en">\n<head>\n\t<meta charset="utf-8">\n\t<title>Server Error | Slack</title>\n\t<meta name="author" content="Slack">\n\t<style></style>\n</head>\n<body>\n\t<nav class="top persistent">\n\t\t<a href="https://status.slack.com/" class="logo" data-qa="logo"></a>\n\t</nav>\n\t<div id="page">\n\t\t<div id="page_contents">\n\t\t\t<h1>\n\t\t\t\t<svg width="30px" height="27px" viewBox="0 0 60 54" class="warning_icon"><path d="" fill="#D94827"/></svg>\n\t\t\t\tServer Error\n\t\t\t</h1>\n\t\t\t<div class="card">\n\t\t\t\t<p>It seems like there’s a problem connecting to our servers, and we’re investigating the issue.</p>\n\t\t\t\t<p>Please <a href="https://status.slack.com/">check our Status page for updates</a>.</p>\n\t\t\t</div>\n\t\t</div>\n\t</div>\n\t<script type="text/javascript">\n\t\tif (window.desktop) {\n\t\t\tdocument.documentElement.className = \'desktop\';\n\t\t}\n\n\t\tvar FIVE_MINS = 5 * 60 * 1000;\n\t\tvar TEN_MINS = 10 * 60 * 1000;\n\n\t\tfunction randomBetween(min, max) {\n\t\t\treturn Math.floor(Math.random() * (max - (min + 1))) + min;\n\t\t}\n\n\t\twindow.setTimeout(function () {\n\t\t\twindow.location.reload(true);\n\t\t}, randomBetween(FIVE_MINS, TEN_MINS));\n\t</script>\n</body>\n</html>'
 
-    state = {"rate_limited_count": 0}
+    state = {"ratelimited_count": 0, "fatal_error_count": 0}
 
     def is_valid_user_agent(self):
         user_agent = self.headers["User-Agent"]
@@ -98,17 +98,37 @@ def _handle(self):
                     return
 
             header = self.headers["Authorization"]
-            if header is not None and "xoxp-" in header:
-                pattern = str(header).split("xoxp-", 1)[1]
+            if header is not None and ("xoxb-" in header or "xoxp-" in header):
+                pattern = ""
+                xoxb = str(header).split("xoxb-", 1)
+                if len(xoxb) > 1:
+                    pattern = xoxb[1]
+                else:
+                    xoxp = str(header).split("xoxp-", 1)
+                    pattern = xoxp[1]
+
                 if "remote_disconnected" in pattern:
                     # http.client.RemoteDisconnected
                     self.finish()
                     return
-                if "ratelimited" in pattern:
+
+                if pattern == "ratelimited" or (pattern == "ratelimited_only_once" and self.state["ratelimited_count"] == 0):
+                    self.state["ratelimited_count"] += 1
                     self.send_response(429)
                     self.send_header("retry-after", 1)
+                    self.send_header("content-type", "application/json;charset=utf-8")
+                    self.send_header("connection", "close")
+                    self.end_headers()
+                    self.wfile.write("""{"ok":false,"error":"ratelimited"}""".encode("utf-8"))
+                    self.wfile.close()
+                    return
+
+                if pattern == "fatal_error" or (pattern == "fatal_error_only_once" and self.state["fatal_error_count"] == 0):
+                    self.state["fatal_error_count"] += 1
+                    self.send_response(200)
                     self.set_common_headers()
-                    self.wfile.write("""{"ok": false, "error": "ratelimited"}""".encode("utf-8"))
+                    self.wfile.write("""{"ok":false,"error":"fatal_error"}""".encode("utf-8"))
+                    self.wfile.close()
                     return
 
             if self.is_valid_token() and self.is_valid_user_agent():
@@ -139,22 +159,10 @@ def _handle(self):
                     self.wfile.write("""{"ok":false}""".encode("utf-8"))
                     return
 
-                if pattern == "rate_limited" or (
-                    pattern == "rate_limited_only_once" and self.state["rate_limited_count"] == 0
-                ):
-                    self.state["rate_limited_count"] += 1
-                    self.send_response(429)
-                    self.send_header("retry-after", 1)
-                    self.send_header("content-type", "application/json;charset=utf-8")
-                    self.send_header("connection", "close")
-                    self.end_headers()
-                    self.wfile.write("""{"ok":false,"error":"rate_limited"}""".encode("utf-8"))
-                    self.wfile.close()
-                    return
-
                 if pattern == "timeout":
-                    time.sleep(2)
+                    time.sleep(3)
                     self.send_response(200)
+                    self.set_common_headers()
                     self.wfile.write("""{"ok":true}""".encode("utf-8"))
                     self.wfile.close()
                     return
@@ -248,7 +256,7 @@ def __init__(self, handler: Type[SimpleHTTPRequestHandler] = MockHandler):
 
     def run(self):
         self.handler.received_requests = {}
-        self.handler.state = {"rate_limited_count": 0}
+        self.handler.state = {"ratelimited_count": 0}
         self.server = HTTPServer(("localhost", 8888), self.handler)
         try:
             self.server.serve_forever(0.05)
@@ -257,7 +265,7 @@ def run(self):
 
     def stop(self):
         self.handler.received_requests = {}
-        self.handler.state = {"rate_limited_count": 0}
+        self.handler.state = {"ratelimited_count": 0}
         self.server.shutdown()
         self.join()
 

tests/slack_sdk/web/mock_web_api_server_http_retry.py

@@ -41,11 +41,11 @@ def _handle(self):
                 self.set_common_headers()
                 self.wfile.write("""{"ok":false}""".encode("utf-8"))
                 return
-            if pattern == "rate_limited":
+            if pattern == "ratelimited":
                 self.send_response(429)
                 self.send_header("retry-after", 1)
                 self.set_common_headers()
-                self.wfile.write("""{"ok":false,"error":"rate_limited"}""".encode("utf-8"))
+                self.wfile.write("""{"ok":false,"error":"ratelimited"}""".encode("utf-8"))
                 self.wfile.close()
                 return
 

tests/slack_sdk/web/test_web_client.py

@@ -88,7 +88,7 @@ def test_slack_api_error_is_raised_on_unsuccessful_responses(self):
             self.client.api_test()
 
     def test_slack_api_rate_limiting_exception_returns_retry_after(self):
-        self.client.token = "xoxb-rate_limited"
+        self.client.token = "xoxb-ratelimited"
         try:
             self.client.api_test()
         except err.SlackApiError as slack_api_error:

tests/slack_sdk/web/test_web_client_http_retry.py

@@ -7,6 +7,7 @@
     cleanup_mock_web_api_server,
     setup_mock_web_api_server,
 )
+from ..fatal_error_retry_handler import FatalErrorRetryHandler
 from ..my_retry_handler import MyRetryHandler
 
 
@@ -21,7 +22,7 @@ def test_remote_disconnected(self):
         retry_handler = MyRetryHandler(max_retry_count=2)
         client = WebClient(
             base_url="http://localhost:8888",
-            token="xoxp-remote_disconnected",
+            token="xoxb-remote_disconnected",
             team_id="T111",
             retry_handlers=[retry_handler],
         )
@@ -33,16 +34,49 @@ def test_remote_disconnected(self):
 
         self.assertEqual(2, retry_handler.call_count)
 
+    def test_ratelimited_no_retry(self):
+        client = WebClient(
+            base_url="http://localhost:8888",
+            token="xoxb-ratelimited",
+            team_id="T111",
+        )
+        try:
+            client.auth_test()
+            self.fail("An exception is expected")
+        except SlackApiError as e:
+            # Just running retries; no assertions for call count so far
+            self.assertEqual(429, e.response.status_code)
+
     def test_ratelimited(self):
         client = WebClient(
             base_url="http://localhost:8888",
-            token="xoxp-ratelimited",
+            token="xoxb-ratelimited_only_once",
             team_id="T111",
         )
         client.retry_handlers.append(RateLimitErrorRetryHandler())
+        # The auto-retry should work here
+        client.auth_test()
+
+    def test_fatal_error_no_retry(self):
+        client = WebClient(
+            base_url="http://localhost:8888",
+            token="xoxb-fatal_error",
+            team_id="T111",
+        )
         try:
             client.auth_test()
             self.fail("An exception is expected")
         except SlackApiError as e:
             # Just running retries; no assertions for call count so far
-            self.assertEqual(429, e.response.status_code)
+            self.assertEqual(200, e.response.status_code)
+            self.assertEqual("fatal_error", e.response["error"])
+
+    def test_fatal_error(self):
+        client = WebClient(
+            base_url="http://localhost:8888",
+            token="xoxb-fatal_error_only_once",
+            team_id="T111",
+        )
+        client.retry_handlers.append(FatalErrorRetryHandler())
+        # The auto-retry should work here
+        client.auth_test()