From 62001465a97e259b27ae356d2cadd83a9c09e6f1 Mon Sep 17 00:00:00 2001 From: Philipp Sontag Date: Mon, 27 Sep 2021 11:19:18 +0200 Subject: [PATCH] Retry asyncio.Timeouterrors during API requests aiohttp is no longer using aiohttp specific timeout errors (the only exception being the ServerTimeoutError which is used in two cases https://github.com/aio-libs/aiohttp/blob/v3.7.4/tests/test_client_functional.py). In all other scenarios aiohttp instead expects an asyncio.TimeoutError. A timeout during an API request should be considered temporary and therefore only be escalated when all retries are exhausted https://github.com/nolar/kopf/issues/840 Signed-off-by: Philipp Sontag --- kopf/_cogs/clients/api.py | 2 +- tests/apis/test_api_requests.py | 12 ++++++++++++ tests/apis/test_error_retries.py | 21 +++++++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/kopf/_cogs/clients/api.py b/kopf/_cogs/clients/api.py index fc4c11f4..87bd2380 100644 --- a/kopf/_cogs/clients/api.py +++ b/kopf/_cogs/clients/api.py @@ -84,7 +84,7 @@ async def request( ) await errors.check_response(response) # but do not parse it! - except (aiohttp.ClientConnectionError, errors.APIServerError) as e: + except (aiohttp.ClientConnectionError, errors.APIServerError, asyncio.TimeoutError) as e: if backoff is None: # i.e. the last or the only attempt. logger.error(f"Request attempt {idx} failed; escalating: {what} -> {e!r}") raise diff --git a/tests/apis/test_api_requests.py b/tests/apis/test_api_requests.py index ad2106ec..ea2aa9de 100644 --- a/tests/apis/test_api_requests.py +++ b/tests/apis/test_api_requests.py @@ -149,6 +149,9 @@ async def serve_slowly(): with timer, pytest.raises(asyncio.TimeoutError): timeout = aiohttp.ClientTimeout(total=0.1) + # aiohttp raises an asyncio.TimeoutError which is automatically retried. + # To reduce the test duration we disable retries for this test. + settings.networking.error_backoffs = None await fn('/url', timeout=timeout, settings=settings, logger=logger) assert 0.1 < timer.seconds < 0.2 @@ -172,6 +175,9 @@ async def serve_slowly(): with timer, pytest.raises(asyncio.TimeoutError): settings.networking.request_timeout = 0.1 + # aiohttp raises an asyncio.TimeoutError which is automatically retried. + # To reduce the test duration we disable retries for this test. + settings.networking.error_backoffs = None await fn('/url', settings=settings, logger=logger) assert 0.1 < timer.seconds < 0.2 @@ -190,6 +196,9 @@ async def serve_slowly(): with timer, pytest.raises(asyncio.TimeoutError): timeout = aiohttp.ClientTimeout(total=0.1) + # aiohttp raises an asyncio.TimeoutError which is automatically retried. + # To reduce the test duration we disable retries for this test. + settings.networking.error_backoffs = None async for _ in stream('/url', timeout=timeout, settings=settings, logger=logger): pass @@ -209,6 +218,9 @@ async def serve_slowly(): with timer, pytest.raises(asyncio.TimeoutError): settings.networking.request_timeout = 0.1 + # aiohttp raises an asyncio.TimeoutError which is automatically retried. + # To reduce the test duration we disable retries for this test. + settings.networking.error_backoffs = None async for _ in stream('/url', settings=settings, logger=logger): pass diff --git a/tests/apis/test_error_retries.py b/tests/apis/test_error_retries.py index e6866888..73e81cc4 100644 --- a/tests/apis/test_error_retries.py +++ b/tests/apis/test_error_retries.py @@ -1,3 +1,5 @@ +import asyncio + import aiohttp.web import pytest @@ -97,6 +99,25 @@ async def test_connection_errors_escalate_with_retries( ]) +async def test_timeout_errors_escalate_with_retries( + caplog, assert_logs, settings, logger, resp_mocker, aresponses, hostname, request_fn): + caplog.set_level(0) + + request_fn.side_effect = asyncio.TimeoutError() + + settings.networking.error_backoffs = [0, 0, 0] + with pytest.raises(asyncio.TimeoutError): + await request('get', '/url', settings=settings, logger=logger) + + assert request_fn.call_count == 4 + assert_logs([ + "attempt #1/4 failed; will retry", + "attempt #2/4 failed; will retry", + "attempt #3/4 failed; will retry", + "attempt #4/4 failed; escalating", + ]) + + async def test_retried_until_succeeded( caplog, assert_logs, settings, logger, resp_mocker, aresponses, hostname): caplog.set_level(0)