diff --git a/integration_tests/web/test_admin_analytics.py b/integration_tests/web/test_admin_analytics.py index 13fe7c8dc..7b309d959 100644 --- a/integration_tests/web/test_admin_analytics.py +++ b/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) diff --git a/slack_sdk/web/async_internal_utils.py b/slack_sdk/web/async_internal_utils.py index 19541e71d..580c84bc8 100644 --- a/slack_sdk/web/async_internal_utils.py +++ b/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 = { diff --git a/slack_sdk/web/base_client.py b/slack_sdk/web/base_client.py index 0f53295ca..aa04e2404 100644 --- a/slack_sdk/web/base_client.py +++ b/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 diff --git a/tests/slack_sdk/audit_logs/test_client_http_retry.py b/tests/slack_sdk/audit_logs/test_client_http_retry.py index 3c06a4cc8..258d3dd98 100644 --- a/tests/slack_sdk/audit_logs/test_client_http_retry.py +++ b/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/", diff --git a/tests/slack_sdk/fatal_error_retry_handler.py b/tests/slack_sdk/fatal_error_retry_handler.py new file mode 100644 index 000000000..49340f56e --- /dev/null +++ b/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" diff --git a/tests/slack_sdk/web/mock_web_api_server.py b/tests/slack_sdk/web/mock_web_api_server.py index 5cfd3f63c..42d13a11e 100644 --- a/tests/slack_sdk/web/mock_web_api_server.py +++ b/tests/slack_sdk/web/mock_web_api_server.py @@ -28,7 +28,7 @@ class MockHandler(SimpleHTTPRequestHandler): error_html_response_body = '\n\n\n\t\n\tServer Error | Slack\n\t\n\t\n\n\n\t\n\t
\n\t\t
\n\t\t\t

\n\t\t\t\t\n\t\t\t\tServer Error\n\t\t\t

\n\t\t\t
\n\t\t\t\t

It seems like there’s a problem connecting to our servers, and we’re investigating the issue.

\n\t\t\t\t

Please check our Status page for updates.

\n\t\t\t
\n\t\t
\n\t
\n\t\n\n' - 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() diff --git a/tests/slack_sdk/web/mock_web_api_server_http_retry.py b/tests/slack_sdk/web/mock_web_api_server_http_retry.py index 5dbc92bd4..18e405709 100644 --- a/tests/slack_sdk/web/mock_web_api_server_http_retry.py +++ b/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 diff --git a/tests/slack_sdk/web/test_web_client.py b/tests/slack_sdk/web/test_web_client.py index 579ed2135..46810dea6 100644 --- a/tests/slack_sdk/web/test_web_client.py +++ b/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: diff --git a/tests/slack_sdk/web/test_web_client_http_retry.py b/tests/slack_sdk/web/test_web_client_http_retry.py index 912e8f3f6..7036187f3 100644 --- a/tests/slack_sdk/web/test_web_client_http_retry.py +++ b/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() diff --git a/tests/slack_sdk/web/test_web_client_http_retry_ratelimited.py b/tests/slack_sdk/web/test_web_client_http_retry_ratelimited.py deleted file mode 100644 index 7308af88b..000000000 --- a/tests/slack_sdk/web/test_web_client_http_retry_ratelimited.py +++ /dev/null @@ -1,25 +0,0 @@ -import unittest - -from slack_sdk.web import WebClient -from tests.slack_sdk.web.mock_web_api_server import ( - setup_mock_web_api_server, - cleanup_mock_web_api_server, -) -from slack_sdk.http_retry import rate_limit_error_retry_handler - - -class TestWebClient_HttpRetry_RateLimited(unittest.TestCase): - def setUp(self): - setup_mock_web_api_server(self) - - def tearDown(self): - cleanup_mock_web_api_server(self) - - def test_rate_limited(self): - client = WebClient( - base_url="http://localhost:8888", - token="xoxb-rate_limited_only_once", - team_id="T111", - ) - client.retry_handlers.append(rate_limit_error_retry_handler) - client.auth_test() diff --git a/tests/slack_sdk_async/audit_logs/test_async_client_http_retry.py b/tests/slack_sdk_async/audit_logs/test_async_client_http_retry.py index 6c3d7ac46..9e6e59ef0 100644 --- a/tests/slack_sdk_async/audit_logs/test_async_client_http_retry.py +++ b/tests/slack_sdk_async/audit_logs/test_async_client_http_retry.py @@ -34,7 +34,7 @@ async def test_http_retries(self): self.assertEqual(2, retry_handler.call_count) @async_test - async def test_rate_limited(self): + async def test_ratelimited(self): client = AsyncAuditLogsClient( token="xoxp-ratelimited", base_url="http://localhost:8888/", diff --git a/tests/slack_sdk_async/fatal_error_retry_handler.py b/tests/slack_sdk_async/fatal_error_retry_handler.py new file mode 100644 index 000000000..b7b5ac038 --- /dev/null +++ b/tests/slack_sdk_async/fatal_error_retry_handler.py @@ -0,0 +1,30 @@ +from typing import Optional +from aiohttp import ServerDisconnectedError, ClientOSError + +from slack_sdk.http_retry.async_handler import AsyncRetryHandler +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 default_interval_calculator + + +class FatalErrorRetryHandler(AsyncRetryHandler): + 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 + + async def _can_retry_async( + 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" diff --git a/tests/slack_sdk_async/web/test_async_web_client.py b/tests/slack_sdk_async/web/test_async_web_client.py index 53ccb1b76..0e4dc4a20 100644 --- a/tests/slack_sdk_async/web/test_async_web_client.py +++ b/tests/slack_sdk_async/web/test_async_web_client.py @@ -64,7 +64,7 @@ async def test_slack_api_error_is_raised_on_unsuccessful_responses(self): @async_test async def test_slack_api_rate_limiting_exception_returns_retry_after(self): - self.client.token = "xoxb-rate_limited" + self.client.token = "xoxb-ratelimited" try: await self.client.api_test() except err.SlackApiError as slack_api_error: @@ -121,8 +121,13 @@ async def test_token_param_async(self): @async_test async def test_timeout_issue_712_async(self): + client = AsyncWebClient( + token="xoxp-1234", + base_url="http://localhost:8888", + timeout=1, + ) with self.assertRaises(Exception): - await self.client.users_list(token="xoxb-timeout") + await client.users_list(token="xoxb-timeout") @async_test async def test_html_response_body_issue_718_async(self): diff --git a/tests/slack_sdk_async/web/test_async_web_client_http_retry.py b/tests/slack_sdk_async/web/test_async_web_client_http_retry.py index c1ab52ab4..5dd78323c 100644 --- a/tests/slack_sdk_async/web/test_async_web_client_http_retry.py +++ b/tests/slack_sdk_async/web/test_async_web_client_http_retry.py @@ -8,6 +8,7 @@ setup_mock_web_api_server, cleanup_mock_web_api_server, ) +from ..fatal_error_retry_handler import FatalErrorRetryHandler from ..my_retry_handler import MyRetryHandler @@ -27,7 +28,7 @@ async def test_remote_disconnected(self): retry_handler = MyRetryHandler(max_retry_count=2) client = AsyncWebClient( base_url="http://localhost:8888", - token="xoxp-remote_disconnected", + token="xoxb-remote_disconnected", team_id="T111", retry_handlers=[retry_handler], ) @@ -40,12 +41,11 @@ async def test_remote_disconnected(self): self.assertEqual(2, retry_handler.call_count) @async_test - async def test_ratelimited(self): + async def test_ratelimited_no_retry(self): client = AsyncWebClient( base_url="http://localhost:8888", - token="xoxp-ratelimited", + token="xoxb-ratelimited", team_id="T111", - retry_handlers=[AsyncRateLimitErrorRetryHandler()], ) try: await client.auth_test() @@ -53,3 +53,38 @@ async def test_ratelimited(self): except err.SlackApiError as e: # Just running retries; no assertions for call count so far self.assertEqual(429, e.response.status_code) + + @async_test + async def test_ratelimited(self): + client = AsyncWebClient( + base_url="http://localhost:8888", + token="xoxb-ratelimited_only_once", + team_id="T111", + ) + client.retry_handlers.append(AsyncRateLimitErrorRetryHandler()) + # The auto-retry should work here + await client.auth_test() + + @async_test + async def test_fatal_error_no_retry(self): + client = AsyncWebClient( + base_url="http://localhost:8888", + token="xoxb-fatal_error", + team_id="T111", + ) + try: + await client.auth_test() + self.fail("An exception is expected") + except err.SlackApiError as e: + self.assertEqual("fatal_error", e.response["error"]) + + @async_test + async def test_fatal_error(self): + client = AsyncWebClient( + 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 + await client.auth_test() diff --git a/tests/slack_sdk_fixture/web_response_fatal_error_only_once.json b/tests/slack_sdk_fixture/web_response_fatal_error_only_once.json new file mode 100644 index 000000000..47971aa3c --- /dev/null +++ b/tests/slack_sdk_fixture/web_response_fatal_error_only_once.json @@ -0,0 +1,3 @@ +{ + "ok": true +} \ No newline at end of file diff --git a/tests/slack_sdk_fixture/web_response_ratelimited_only_once.json b/tests/slack_sdk_fixture/web_response_ratelimited_only_once.json new file mode 100644 index 000000000..47971aa3c --- /dev/null +++ b/tests/slack_sdk_fixture/web_response_ratelimited_only_once.json @@ -0,0 +1,3 @@ +{ + "ok": true +} \ No newline at end of file diff --git a/tests/web/mock_web_api_server.py b/tests/web/mock_web_api_server.py index 28c358b41..a189a3da2 100644 --- a/tests/web/mock_web_api_server.py +++ b/tests/web/mock_web_api_server.py @@ -99,11 +99,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 diff --git a/tests/web/test_async_web_client.py b/tests/web/test_async_web_client.py index 3f00580da..41cf4ce77 100644 --- a/tests/web/test_async_web_client.py +++ b/tests/web/test_async_web_client.py @@ -64,7 +64,7 @@ async def test_slack_api_error_is_raised_on_unsuccessful_responses(self): @async_test async def test_slack_api_rate_limiting_exception_returns_retry_after(self): - self.client.token = "xoxb-rate_limited" + self.client.token = "xoxb-ratelimited" try: await self.client.api_test() except err.SlackApiError as slack_api_error: diff --git a/tests/web/test_web_client.py b/tests/web/test_web_client.py index 7ed61b765..d13487c04 100644 --- a/tests/web/test_web_client.py +++ b/tests/web/test_web_client.py @@ -127,7 +127,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: