From 5a14fdaff36d69b8bcde0c015933ca5b48152fdf Mon Sep 17 00:00:00 2001 From: Matthias Bach Date: Mon, 26 Feb 2024 16:58:04 +0100 Subject: [PATCH] Respect the "Retry-After" times requested by Jira The time Jira sends in the Retry-After header is the minimum time Jira wants us to wait before retrying our request. However, the former implementation used this as a maximum waiting time for the next request. In result, there was a chance that we reached three retries without reaching the time that Jira expected us to wait and our request would fail. This implementation does also affect the other retry cases, as while previously we jittered our backoff between 0 and the target backoff, we now only jitter between 50% and 100% of the target backoff. However, this should still protect us from thundering herds and safes us from introducing a new minimum backoff variable for the retry-after case. This solves one of the issues reported in #1805. --- jira/resilientsession.py | 6 ++++-- tests/test_resilientsession.py | 8 +++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/jira/resilientsession.py b/jira/resilientsession.py index 6b7b11670..330ecbe20 100644 --- a/jira/resilientsession.py +++ b/jira/resilientsession.py @@ -315,7 +315,7 @@ def __recoverable( if response.status_code in recoverable_error_codes: retry_after = response.headers.get("Retry-After") if retry_after: - suggested_delay = int(retry_after) # Do as told + suggested_delay = 2 * int(retry_after) # Do as told elif response.status_code == HTTPStatus.TOO_MANY_REQUESTS: suggested_delay = 10 * 2**counter # Exponential backoff @@ -326,7 +326,9 @@ def __recoverable( is_recoverable = suggested_delay > 0 if is_recoverable: # Apply jitter to prevent thundering herd - delay = min(self.max_retry_delay, suggested_delay) * random.random() + delay = min(self.max_retry_delay, suggested_delay) * random.uniform( + 0.5, 1.0 + ) LOG.warning( f"Got recoverable error from {request_method} {url}, will retry [{counter}/{self.max_retries}] in {delay}s. Err: {msg}" # type: ignore[str-bytes-safe] ) diff --git a/tests/test_resilientsession.py b/tests/test_resilientsession.py index b5e22bb2c..f4ff7e0b9 100644 --- a/tests/test_resilientsession.py +++ b/tests/test_resilientsession.py @@ -106,7 +106,8 @@ def test_status_codes_retries( with_retry_after_header: bool, retry_expected: bool, ): - RETRY_AFTER_HEADER = {"Retry-After": "1"} + RETRY_AFTER_SECONDS = 1 if with_retry_after_header else 0 + RETRY_AFTER_HEADER = {"Retry-After": f"{RETRY_AFTER_SECONDS}"} RATE_LIMIT_HEADERS = { "X-RateLimit-FillRate": "1", "X-RateLimit-Interval-Seconds": "1", @@ -141,6 +142,11 @@ def test_status_codes_retries( assert mocked_request_method.call_count == expected_number_of_requests assert mocked_sleep_method.call_count == expected_number_of_sleep_invocations + for actual_sleep in ( + call_args.args[0] for call_args in mocked_sleep_method.call_args_list + ): + assert actual_sleep >= RETRY_AFTER_SECONDS + errors_parsing_test_data = [ (403, {"x-authentication-denied-reason": "err1"}, "", ["err1"]),