From 886f28e47e38831c3cb55da2e790d73f81d33181 Mon Sep 17 00:00:00 2001 From: Philip Gichuhi Date: Mon, 2 Dec 2024 13:57:23 +0300 Subject: [PATCH 1/4] fix: Ensure calculated retry delay validated against correct maximum value of 180 secs --- packages/http/httpx/kiota_http/middleware/retry_handler.py | 2 +- .../http/httpx/tests/middleware_tests/test_retry_handler.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/http/httpx/kiota_http/middleware/retry_handler.py b/packages/http/httpx/kiota_http/middleware/retry_handler.py index a0f6919..7aecb34 100644 --- a/packages/http/httpx/kiota_http/middleware/retry_handler.py +++ b/packages/http/httpx/kiota_http/middleware/retry_handler.py @@ -95,7 +95,7 @@ async def send(self, request: httpx.Request, transport: httpx.AsyncBaseTransport # Check if the request needs to be retried based on the response method # and status code should_retry = self.should_retry(request, current_options, response) - if all([should_retry, retry_valid, delay < max_delay]): + if all([should_retry, retry_valid, delay < RetryHandlerOption.MAX_DELAY]): time.sleep(delay) end_time = time.time() max_delay -= (end_time - start_time) diff --git a/packages/http/httpx/tests/middleware_tests/test_retry_handler.py b/packages/http/httpx/tests/middleware_tests/test_retry_handler.py index 72f661f..6b27517 100644 --- a/packages/http/httpx/tests/middleware_tests/test_retry_handler.py +++ b/packages/http/httpx/tests/middleware_tests/test_retry_handler.py @@ -224,9 +224,10 @@ def request_handler(request: httpx.Request): return httpx.Response(200, ) return httpx.Response( TOO_MANY_REQUESTS, - headers={RETRY_AFTER: "20"}, + headers={RETRY_AFTER: "200"}, # value exceeds max delay of 180 secs ) + # Retry-after value takes precedence over the RetryHandlerOption value specified here handler = RetryHandler(RetryHandlerOption(10, 1, True)) request = httpx.Request( 'GET', From 48224f076970ac7d08450332b05724c3a9edb402 Mon Sep 17 00:00:00 2001 From: Philip Gichuhi Date: Mon, 2 Dec 2024 14:09:51 +0300 Subject: [PATCH 2/4] Removes unused max_delay variable & other related variables --- packages/http/httpx/kiota_http/middleware/retry_handler.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/http/httpx/kiota_http/middleware/retry_handler.py b/packages/http/httpx/kiota_http/middleware/retry_handler.py index 7aecb34..24dc748 100644 --- a/packages/http/httpx/kiota_http/middleware/retry_handler.py +++ b/packages/http/httpx/kiota_http/middleware/retry_handler.py @@ -78,12 +78,10 @@ async def send(self, request: httpx.Request, transport: httpx.AsyncBaseTransport _span.set_attribute("com.microsoft.kiota.handler.retry.enable", True) _span.end() retry_valid = current_options.should_retry - max_delay = current_options.max_delay _retry_span = self._create_observability_span( request, f"RetryHandler_send - attempt {retry_count}" ) while retry_valid: - start_time = time.time() response = await super().send(request, transport) _retry_span.set_attribute(HTTP_RESPONSE_STATUS_CODE, response.status_code) # check that max retries has not been hit @@ -97,8 +95,6 @@ async def send(self, request: httpx.Request, transport: httpx.AsyncBaseTransport should_retry = self.should_retry(request, current_options, response) if all([should_retry, retry_valid, delay < RetryHandlerOption.MAX_DELAY]): time.sleep(delay) - end_time = time.time() - max_delay -= (end_time - start_time) # increment the count for retries retry_count += 1 request.headers.update({'retry-attempt': f'{retry_count}'}) From fd87c67d599add9e59f48db76a23dd367376b3e1 Mon Sep 17 00:00:00 2001 From: Philip Gichuhi Date: Mon, 2 Dec 2024 14:18:53 +0300 Subject: [PATCH 3/4] fix: Fixes retry handler exponential back-off to consider the delay specified in the retry handler option --- .../http/httpx/kiota_http/middleware/retry_handler.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/http/httpx/kiota_http/middleware/retry_handler.py b/packages/http/httpx/kiota_http/middleware/retry_handler.py index 24dc748..e5e07ae 100644 --- a/packages/http/httpx/kiota_http/middleware/retry_handler.py +++ b/packages/http/httpx/kiota_http/middleware/retry_handler.py @@ -88,7 +88,7 @@ async def send(self, request: httpx.Request, transport: httpx.AsyncBaseTransport retry_valid = self.check_retry_valid(retry_count, current_options) # Get the delay time between retries - delay = self.get_delay_time(retry_count, response) + delay = self.get_delay_time(retry_count, response, current_options.max_delay) # Check if the request needs to be retried based on the response method # and status code @@ -165,7 +165,7 @@ def check_retry_valid(self, retry_count, options): return True return False - def get_delay_time(self, retry_count, response=None): + def get_delay_time(self, retry_count, response=None, delay=0): """ Get the time in seconds to delay between retry attempts. Respects a retry-after header in the response if provided @@ -174,15 +174,15 @@ def get_delay_time(self, retry_count, response=None): retry_after = self._get_retry_after(response) if retry_after: return retry_after - return self._get_delay_time_exp_backoff(retry_count) + return self._get_delay_time_exp_backoff(retry_count, delay) - def _get_delay_time_exp_backoff(self, retry_count): + def _get_delay_time_exp_backoff(self, retry_count, delay): """ Get time in seconds to delay between retry attempts based on an exponential backoff value. """ exp_backoff_value = self.backoff_factor * +(2**(retry_count - 1)) - backoff_value = exp_backoff_value + (random.randint(0, 1000) / 1000) + backoff_value = exp_backoff_value + (random.randint(0, 1000) / 1000) + delay backoff = min(self.backoff_max, backoff_value) return backoff From c655fa253294bf3b8f65be1b991a93ce0890a46a Mon Sep 17 00:00:00 2001 From: Philip Gichuhi Date: Mon, 2 Dec 2024 15:03:14 +0300 Subject: [PATCH 4/4] fix: Ensures retry count is incremented based on value in retry-attempt header --- .../kiota_http/middleware/retry_handler.py | 22 ++++++++++-------- .../middleware_tests/test_retry_handler.py | 23 +++++++++++++++++++ 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/packages/http/httpx/kiota_http/middleware/retry_handler.py b/packages/http/httpx/kiota_http/middleware/retry_handler.py index e5e07ae..bc58075 100644 --- a/packages/http/httpx/kiota_http/middleware/retry_handler.py +++ b/packages/http/httpx/kiota_http/middleware/retry_handler.py @@ -14,6 +14,8 @@ from .middleware import BaseMiddleware from .options import RetryHandlerOption +RETRY_ATTEMPT = "Retry-Attempt" + class RetryHandler(BaseMiddleware): """ @@ -71,20 +73,21 @@ async def send(self, request: httpx.Request, transport: httpx.AsyncBaseTransport Sends the http request object to the next middleware or retries the request if necessary. """ response = None - retry_count = 0 - _span = self._create_observability_span(request, "RetryHandler_send") current_options = self._get_current_options(request) _span.set_attribute("com.microsoft.kiota.handler.retry.enable", True) _span.end() retry_valid = current_options.should_retry - _retry_span = self._create_observability_span( - request, f"RetryHandler_send - attempt {retry_count}" - ) + while retry_valid: response = await super().send(request, transport) - _retry_span.set_attribute(HTTP_RESPONSE_STATUS_CODE, response.status_code) # check that max retries has not been hit + retry_count = 0 if RETRY_ATTEMPT not in response.request.headers else int( + response.request.headers[RETRY_ATTEMPT] + ) + _retry_span = self._create_observability_span( + request, f"RetryHandler_send - attempt {retry_count}" + ) retry_valid = self.check_retry_valid(retry_count, current_options) # Get the delay time between retries @@ -97,13 +100,14 @@ async def send(self, request: httpx.Request, transport: httpx.AsyncBaseTransport time.sleep(delay) # increment the count for retries retry_count += 1 - request.headers.update({'retry-attempt': f'{retry_count}'}) + request.headers.update({RETRY_ATTEMPT: f'{retry_count}'}) + _retry_span.set_attribute(HTTP_RESPONSE_STATUS_CODE, response.status_code) _retry_span.set_attribute('http.request.resend_count', retry_count) continue + _retry_span.end() break if response is None: response = await super().send(request, transport) - _retry_span.end() return response def _get_current_options(self, request: httpx.Request) -> RetryHandlerOption: @@ -165,7 +169,7 @@ def check_retry_valid(self, retry_count, options): return True return False - def get_delay_time(self, retry_count, response=None, delay=0): + def get_delay_time(self, retry_count, response=None, delay=RetryHandlerOption.DEFAULT_DELAY): """ Get the time in seconds to delay between retry attempts. Respects a retry-after header in the response if provided diff --git a/packages/http/httpx/tests/middleware_tests/test_retry_handler.py b/packages/http/httpx/tests/middleware_tests/test_retry_handler.py index 6b27517..de7dd4f 100644 --- a/packages/http/httpx/tests/middleware_tests/test_retry_handler.py +++ b/packages/http/httpx/tests/middleware_tests/test_retry_handler.py @@ -238,6 +238,29 @@ def request_handler(request: httpx.Request): assert resp.status_code == 429 assert RETRY_ATTEMPT not in resp.request.headers +@pytest.mark.asyncio +async def test_max_retries_respected(): + """Test that a request is not retried more than max_retries configured""" + + def request_handler(request: httpx.Request): + if RETRY_ATTEMPT in request.headers: + return httpx.Response(200, ) + return httpx.Response( + TOO_MANY_REQUESTS, + ) + + # Retry-after value takes precedence over the RetryHandlerOption value specified here + handler = RetryHandler(RetryHandlerOption(10, 3, True)) + request = httpx.Request( + 'GET', + BASE_URL, + headers={RETRY_ATTEMPT: '5'} # value exceeds max retries configured + ) + mock_transport = httpx.MockTransport(request_handler) + resp = await handler.send(request, mock_transport) + assert resp.status_code == 200 + assert RETRY_ATTEMPT in resp.request.headers + assert resp.request.headers[RETRY_ATTEMPT] == '5' @pytest.mark.asyncio async def test_retry_options_apply_per_request():