Skip to content

Commit

Permalink
Respect the "Retry-After" times requested by Jira
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
matthias-bach-by committed Feb 26, 2024
1 parent ff6985b commit 5a14fda
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 3 deletions.
6 changes: 4 additions & 2 deletions jira/resilientsession.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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]
)
Expand Down
8 changes: 7 additions & 1 deletion tests/test_resilientsession.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"]),
Expand Down

0 comments on commit 5a14fda

Please sign in to comment.