diff --git a/blebox_uniapi/session.py b/blebox_uniapi/session.py index 65a7978..28fc8d9 100644 --- a/blebox_uniapi/session.py +++ b/blebox_uniapi/session.py @@ -6,18 +6,32 @@ from . import error -DEFAULT_TIMEOUT = aiohttp.ClientTimeout(total=None, sock_connect=5, sock_read=5) +CONNECT_TIMEOUT = 3 +CONNECT_ATTEMPTS = 2 +DEFAULT_TIMEOUT = aiohttp.ClientTimeout( + total=None, sock_connect=CONNECT_TIMEOUT, sock_read=5 +) DEFAULT_PORT = 80 logger = logging.getLogger(__name__) +def _client_timeout( + timeout: Union[int, aiohttp.ClientTimeout, None], +) -> aiohttp.ClientTimeout: + if not timeout: + return DEFAULT_TIMEOUT + if isinstance(timeout, aiohttp.ClientTimeout): + return timeout + return aiohttp.ClientTimeout(total=timeout, sock_connect=CONNECT_TIMEOUT) + + class ApiHost: def __init__( self, host: str, port: int, - timeout: int, + timeout: Union[int, aiohttp.ClientTimeout, None], session: Any, loop: Any, logger: logging.Logger = logger, @@ -30,7 +44,7 @@ def __init__( # TODO: handle empty logger? self._logger = logger - self._timeout = timeout if timeout else DEFAULT_TIMEOUT + self._timeout = _client_timeout(timeout) self._session = session @@ -40,7 +54,9 @@ def __init__( auth = aiohttp.BasicAuth(login=self._username, password=self._password) if not self._session: - self._session = aiohttp.ClientSession(loop=loop, timeout=timeout, auth=auth) + self._session = aiohttp.ClientSession( + loop=loop, timeout=self._timeout, auth=auth + ) # TODO: remove? self._loop = loop @@ -52,14 +68,13 @@ async def async_request( data: Union[dict, str, None] = None, allow_ota_check_response: bool = False, ) -> Optional[dict]: - # TODO: check timeout client_timeout = self._timeout url = self.api_path(path) + kwargs: dict[str, Any] = {"timeout": client_timeout} + if data is not None: + kwargs["data"] = data try: - if data is not None: - response = await async_method(url, timeout=client_timeout, data=data) - else: - response = await async_method(url, timeout=client_timeout) + response = await self._async_send(async_method, url, **kwargs) accepted_statuses = (200, 202, 204) if allow_ota_check_response else (200,) if response.status not in accepted_statuses: @@ -76,11 +91,16 @@ async def async_request( return await response.json() - except asyncio.TimeoutError as ex: + except aiohttp.ConnectionTimeoutError as ex: + connect_timeout = client_timeout.sock_connect or client_timeout.connect raise error.TimeoutError( - f"Failed to connect to {self.host}:{self.port} within {client_timeout}s: ({ex})" + f"Failed to connect to {self.host}:{self.port} within " + f"{connect_timeout}s ({CONNECT_ATTEMPTS} attempts)" ) from ex + except asyncio.TimeoutError as ex: + raise error.TimeoutError(f"Request to {url} timed out") from ex + except aiohttp.ClientConnectionError as ex: raise error.ConnectionError( f"Failed to connect to {self.host}:{self.port}: {ex}" @@ -94,6 +114,20 @@ async def async_request( f"Invalid response encoding from {url}: {ex}" ) from ex + async def _async_send(self, async_method: Any, url: str, **kwargs: Any) -> Any: + # Nothing was sent yet, so retrying on a new connection is safe. + for attempt in range(1, CONNECT_ATTEMPTS): + try: + return await async_method(url, **kwargs) + except aiohttp.ConnectionTimeoutError: + self._logger.debug( + "Connection to %s timed out, retrying (attempt %s/%s)", + url, + attempt + 1, + CONNECT_ATTEMPTS, + ) + return await async_method(url, **kwargs) + async def async_api_get(self, path: str) -> Optional[dict]: try: return await self.async_request(path, self._session.get) diff --git a/requirements_tests.txt b/requirements_tests.txt index 800fd21..799efc5 100644 --- a/requirements_tests.txt +++ b/requirements_tests.txt @@ -1,4 +1,4 @@ -aiohttp==3.8.5 +aiohttp==3.10.11 aiosignal==1.2.0 async-timeout==4.0.2 attrs==22.1.0 @@ -29,7 +29,7 @@ requests==2.31.0 tomli==2.0.1 urllib3==1.26.12 yarg==0.1.9 -yarl==1.8.1 +yarl==1.17.2 ruff==0.3.0 pre-commit==3.6.2 jmespath>=1.0.0 diff --git a/setup.py b/setup.py index c90af66..d446ad5 100644 --- a/setup.py +++ b/setup.py @@ -10,7 +10,7 @@ with open("CHANGELOG.md") as changelog_file: changelog = changelog_file.read() -requirements = ["aiohttp>=3", "jmespath>1.0.0"] +requirements = ["aiohttp>=3.10", "jmespath>1.0.0"] setup_requirements = [ "pytest-runner", diff --git a/tests/test_session.py b/tests/test_session.py index da97fd9..c9d45a2 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -6,7 +6,7 @@ from unittest.mock import patch, Mock, AsyncMock -from blebox_uniapi.session import ApiHost as Session +from blebox_uniapi.session import CONNECT_TIMEOUT, ApiHost as Session from blebox_uniapi import error @@ -59,7 +59,10 @@ async def test_session_api_get(logger, client): result = await api_session.async_api_get("/api/foo") - client.get.assert_called_once_with("http://127.0.0.4:88/api/foo", timeout=2) + client.get.assert_called_once_with( + "http://127.0.0.4:88/api/foo", + timeout=aiohttp.ClientTimeout(total=2, sock_connect=CONNECT_TIMEOUT), + ) assert result == 123 @@ -70,7 +73,10 @@ async def test_session_default_client_created(mocked_client, logger): result = await api_session.async_api_get("/api/foo") - mocked_client.get.assert_called_once_with("http://127.0.0.4:88/api/foo", timeout=2) + mocked_client.get.assert_called_once_with( + "http://127.0.0.4:88/api/foo", + timeout=aiohttp.ClientTimeout(total=2, sock_connect=CONNECT_TIMEOUT), + ) assert result == 123 @@ -80,7 +86,7 @@ async def test_session_default_timeout_used(mocked_client, logger): await api_session.async_api_get("/api/foo") expected_timeout = aiohttp.ClientTimeout( - total=None, connect=None, sock_read=5, sock_connect=5 + total=None, connect=None, sock_read=5, sock_connect=CONNECT_TIMEOUT ) mocked_client.get.assert_called_once_with( @@ -231,3 +237,79 @@ async def test_session_api_get_unicode_decode_error_raises_connection_error( api_session = Session("127.0.0.4", "88", 2, client, None, logger) with pytest.raises(error.ConnectionError, match="Invalid response encoding"): await api_session.async_api_get("/api/device/state") + + +async def test_session_retries_after_connection_timeout(logger, client): + client.get = AsyncMock( + side_effect=[aiohttp.ConnectionTimeoutError(), valid_response()] + ) + api_session = Session("127.0.0.4", "88", 2, client, None, logger) + + result = await api_session.async_api_get("/api/foo") + + assert result == 123 + assert client.get.call_count == 2 + + +async def test_session_post_retries_after_connection_timeout(logger, client): + client.post = AsyncMock( + side_effect=[aiohttp.ConnectionTimeoutError(), valid_response()] + ) + api_session = Session("127.0.0.4", "88", 2, client, None, logger) + + result = await api_session.async_api_post("/api/foo", {"a": 1}) + + assert result == 123 + assert client.post.call_count == 2 + client.post.assert_called_with( + "http://127.0.0.4:88/api/foo", + timeout=aiohttp.ClientTimeout(total=2, sock_connect=CONNECT_TIMEOUT), + data={"a": 1}, + ) + + +async def test_session_connection_timeout_after_all_attempts(logger, client): + client.get = AsyncMock(side_effect=aiohttp.ConnectionTimeoutError()) + api_session = Session("127.0.0.4", "88", 2, client, None, logger) + + with pytest.raises( + error.TimeoutError, + match=rf"Failed to connect to 127\.0\.0\.4:88 within {CONNECT_TIMEOUT}s", + ): + await api_session.async_api_get("/api/foo") + + assert client.get.call_count == 2 + + +async def test_session_read_timeout_is_not_retried(logger, client): + client.get = AsyncMock(side_effect=aiohttp.SocketTimeoutError()) + api_session = Session("127.0.0.4", "88", 2, client, None, logger) + + with pytest.raises( + error.TimeoutError, + match=r"Request to http://127\.0\.0\.4:88/api/foo timed out", + ): + await api_session.async_api_get("/api/foo") + + client.get.assert_called_once() + + +async def test_session_client_timeout_passed_through(logger, client): + client.get = AsyncMock(return_value=valid_response()) + timeout = aiohttp.ClientTimeout(total=7, sock_connect=1) + api_session = Session("127.0.0.4", "88", timeout, client, None, logger) + + await api_session.async_api_get("/api/foo") + + client.get.assert_called_once_with("http://127.0.0.4:88/api/foo", timeout=timeout) + + +async def test_session_connection_timeout_message_uses_connect(logger, client): + client.get = AsyncMock(side_effect=aiohttp.ConnectionTimeoutError()) + timeout = aiohttp.ClientTimeout(total=10, connect=4) + api_session = Session("127.0.0.4", "88", timeout, client, None, logger) + + with pytest.raises( + error.TimeoutError, match=r"Failed to connect to 127\.0\.0\.4:88 within 4s" + ): + await api_session.async_api_get("/api/foo")