diff --git a/src/demetriek/cloud.py b/src/demetriek/cloud.py index 2f875dcb..28dfe9a4 100644 --- a/src/demetriek/cloud.py +++ b/src/demetriek/cloud.py @@ -20,6 +20,7 @@ ) from yarl import URL +from .const import TRANSIENT_HTTP_STATUSES from .exceptions import ( LaMetricAuthenticationError, LaMetricConnectionError, @@ -103,10 +104,17 @@ async def _request( body = await response.text() # The cloud did answer, so this is not a connection problem and - # retrying will not help. An expired token must surface as an + # retrying will not help, unless it says it is briefly unable to + # answer at all. An expired token must surface as an # authentication error, so the caller can ask for a new one. if response.status >= HTTPStatus.BAD_REQUEST: reason = error_message(body) or response.reason + if response.status in TRANSIENT_HTTP_STATUSES: + msg = ( + "The LaMetric cloud is temporarily unavailable" + f" ({response.status}): {reason}" + ) + raise LaMetricConnectionError(msg) if response.status in [HTTPStatus.UNAUTHORIZED, HTTPStatus.FORBIDDEN]: msg = f"Authentication to the LaMetric cloud failed: {reason}" raise LaMetricAuthenticationError(msg) diff --git a/src/demetriek/const.py b/src/demetriek/const.py index 92f745b6..51cd3635 100644 --- a/src/demetriek/const.py +++ b/src/demetriek/const.py @@ -1,6 +1,7 @@ """Asynchronous Python client for LaMetric TIME devices.""" from enum import IntEnum, StrEnum +from http import HTTPStatus # The model the device reports is a hardware identifier, not a product name. # LaMetric TIME reports "LM 37X8" on firmware 2.x and "sa8" on 3.x. @@ -10,6 +11,14 @@ "sa8": "TIME", } +# Statuses that say the server is briefly unable to answer, rather than that +# something is wrong with the request. Those are worth another try. +TRANSIENT_HTTP_STATUSES = { + HTTPStatus.BAD_GATEWAY, + HTTPStatus.SERVICE_UNAVAILABLE, + HTTPStatus.GATEWAY_TIMEOUT, +} + class BrightnessMode(StrEnum): """Enum holding the available brightness modes.""" diff --git a/src/demetriek/device.py b/src/demetriek/device.py index 4d48ba42..0b5962a4 100644 --- a/src/demetriek/device.py +++ b/src/demetriek/device.py @@ -22,7 +22,12 @@ ) from yarl import URL -from .const import ScreensaverMode, StreamFillType, StreamRenderMode +from .const import ( + TRANSIENT_HTTP_STATUSES, + ScreensaverMode, + StreamFillType, + StreamRenderMode, +) from .exceptions import ( LaMetricAuthenticationError, LaMetricConnectionError, @@ -76,7 +81,8 @@ def _can_retry(retry_state: RetryCallState) -> bool: return True return isinstance( - exception.__cause__, (aiohttp.ClientConnectorError, socket.gaierror) + exception.__cause__, + (aiohttp.ClientConnectorError, aiohttp.ConnectionTimeoutError, socket.gaierror), ) @@ -150,8 +156,15 @@ async def _request( # The device did answer, so this is not a connection problem and # retrying will not help. Pass on what the device says is wrong. + # Unless it says it is briefly unable to answer at all. if response.status >= HTTPStatus.BAD_REQUEST: reason = error_message(body) or response.reason + if response.status in TRANSIENT_HTTP_STATUSES: + msg = ( + f"The LaMetric device at {self.host} is temporarily" + f" unavailable ({response.status}): {reason}" + ) + raise LaMetricConnectionError(msg) if response.status in [HTTPStatus.UNAUTHORIZED, HTTPStatus.FORBIDDEN]: msg = ( f"Authentication to the LaMetric device at {self.host}" diff --git a/tests/test_cloud.py b/tests/test_cloud.py index 6daec32c..6e841676 100644 --- a/tests/test_cloud.py +++ b/tests/test_cloud.py @@ -127,6 +127,31 @@ async def test_http_error401( assert len(next(iter(responses.requests.values()))) == 1 +@pytest.mark.parametrize("status", [502, 503, 504]) +async def test_temporarily_unavailable_retried( + responses: aioresponses, cloud: LaMetricCloud, status: int +) -> None: + """Test a request is retried when the cloud is briefly unable to answer.""" + responses.get(f"{CLOUD_URL}/", status=status, body="") + responses.get(f"{CLOUD_URL}/", status=200, body='{"status": "ok"}') + + response = await cloud._request("/") + + assert response["status"] == "ok" + + +async def test_temporarily_unavailable_gives_up( + responses: aioresponses, cloud: LaMetricCloud +) -> None: + """Test a cloud that stays unavailable raises a connection error.""" + responses.get(f"{CLOUD_URL}/", status=503, body="", repeat=True) + + with pytest.raises(LaMetricConnectionError, match="temporarily unavailable"): + await cloud._request("/") + + assert len(next(iter(responses.requests.values()))) == 3 + + async def test_http_error_not_retried( responses: aioresponses, cloud: LaMetricCloud ) -> None: diff --git a/tests/test_lametric.py b/tests/test_lametric.py index a7d97bf2..aa0649db 100644 --- a/tests/test_lametric.py +++ b/tests/test_lametric.py @@ -95,6 +95,10 @@ async def test_timeout(responses: aioresponses, device: LaMetricDevice) -> None: assert len(next(iter(responses.requests.values()))) == 3 +CHANGE_METHODS = ["POST", "PUT", "DELETE"] + + +@pytest.mark.parametrize("method", CHANGE_METHODS) @pytest.mark.parametrize( "exception", [ @@ -103,40 +107,87 @@ async def test_timeout(responses: aioresponses, device: LaMetricDevice) -> None: ], ) async def test_change_not_retried_once_sent( - responses: aioresponses, device: LaMetricDevice, exception: Exception + responses: aioresponses, + device: LaMetricDevice, + exception: Exception, + method: str, ) -> None: """Test a change is not repeated when it may have reached the device. Repeating it could, for example, show the same notification twice. """ - responses.post(f"{DEVICE_URL}/", exception=exception, repeat=True) + responses.add(f"{DEVICE_URL}/", method, exception=exception, repeat=True) with pytest.raises(LaMetricConnectionError): - await device._request("/", method="POST") + await device._request("/", method=method) assert len(next(iter(responses.requests.values()))) == 1 +@pytest.mark.parametrize("method", CHANGE_METHODS) @pytest.mark.parametrize( "exception", [ aiohttp.ClientConnectorError(MagicMock(), OSError()), + aiohttp.ConnectionTimeoutError(), socket.gaierror(), ], ) async def test_change_retried_when_not_sent( - responses: aioresponses, device: LaMetricDevice, exception: Exception + responses: aioresponses, + device: LaMetricDevice, + exception: Exception, + method: str, ) -> None: """Test a change is retried when it never reached the device.""" - responses.post(f"{DEVICE_URL}/", exception=exception) - responses.post(f"{DEVICE_URL}/", exception=exception) - responses.post(f"{DEVICE_URL}/", status=200, body='{"status": "ok"}') + responses.add(f"{DEVICE_URL}/", method, exception=exception) + responses.add(f"{DEVICE_URL}/", method, exception=exception) + responses.add(f"{DEVICE_URL}/", method, status=200, body='{"status": "ok"}') + + response = await device._request("/", method=method) + + assert response["status"] == "ok" - response = await device._request("/", method="POST") + +@pytest.mark.parametrize("status", [502, 503, 504]) +async def test_temporarily_unavailable_read_retried( + responses: aioresponses, device: LaMetricDevice, status: int +) -> None: + """Test reading is retried when the device is briefly unable to answer.""" + responses.get(f"{DEVICE_URL}/", status=status, body="") + responses.get(f"{DEVICE_URL}/", status=200, body='{"status": "ok"}') + + response = await device._request("/") assert response["status"] == "ok" +@pytest.mark.parametrize("method", CHANGE_METHODS) +async def test_temporarily_unavailable_change_not_retried( + responses: aioresponses, device: LaMetricDevice, method: str +) -> None: + """Test a change is not repeated, the device may have acted on it.""" + responses.add(f"{DEVICE_URL}/", method, status=503, body="", repeat=True) + + with pytest.raises(LaMetricConnectionError, match="temporarily unavailable"): + await device._request("/", method=method) + + assert len(next(iter(responses.requests.values()))) == 1 + + +async def test_server_error_not_retried( + responses: aioresponses, device: LaMetricDevice +) -> None: + """Test an error that is not transient is raised right away.""" + responses.get(f"{DEVICE_URL}/", status=500, body="", repeat=True) + + with pytest.raises(LaMetricError) as error: + await device._request("/") + + assert not isinstance(error.value, LaMetricConnectionError) + assert len(next(iter(responses.requests.values()))) == 1 + + async def test_http_error404(responses: aioresponses, device: LaMetricDevice) -> None: """Test HTTP 404 response handling.""" responses.get(