fix: retry once when device connection times out

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)

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

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",

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")