Improve #1084 to run rate limited error retry handler correctly #1094
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
This should exist here but it was missing in this method (this bug affects only RateLimitedErrorRetryHandler)
| @@ -39,6 +39,10 @@ def _can_retry( | |||
| if error is None: | |||
| return False | |||
|
|
|||
| if isinstance(error, URLError): | |||
There was a problem hiding this comment.
As HttpError is a sub-class of URLError, we should have checked if the response exists here.
| @@ -69,7 +73,7 @@ def prepare_for_next_attempt( | |||
| if response is None: | |||
| raise error | |||
|
|
|||
| state.increment_current_attempt() | |||
| state.next_attempt_requested = True | |||
There was a problem hiding this comment.
This should exist here but it was missing in this method (this bug affects only RateLimitedErrorRetryHandler)
| 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) |
There was a problem hiding this comment.
Unrelated to the bug we are fixing here; Just to make this a bit more robust
| @@ -143,6 +143,15 @@ def convert_params(values: dict) -> dict: | |||
| res, | |||
| ) | |||
|
|
|||
| if logger.level <= logging.DEBUG: | |||
There was a problem hiding this comment.
Moved this debug logging from slack_response.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: | |||
There was a problem hiding this comment.
Moved this debug logging from 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: | |||
There was a problem hiding this comment.
Moved this logging from here to base client classes to print this even for the retry patterns
Codecov Report
@@ Coverage Diff @@
## main #1094 +/- ##
==========================================
+ Coverage 86.06% 86.17% +0.11%
==========================================
Files 110 110
Lines 9936 9945 +9
==========================================
+ Hits 8551 8570 +19
+ Misses 1385 1375 -10
Continue to review full report at Codecov.
|
Summary
This pull request fixes a few bugs in #1084 about the built-in rate limited error handlers (sync/async)
Category (place an
xin each of the[ ])/docs-src(Documents, have you run./docs.sh?)/docs-src-v2(Documents, have you run./docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.