Retry a stalled Twinkly request (#179353)

homeassistant/components/twinkly/__init__.py

@@ -10,7 +10,7 @@ from homeassistant.core import HomeAssistant
 from homeassistant.helpers import device_registry as dr, entity_registry as er
 from homeassistant.helpers.aiohttp_client import async_get_clientsession
 
-from .const import DOMAIN
+from .const import DEVICE_TIMEOUT, DOMAIN
 from .coordinator import TwinklyConfigEntry, TwinklyCoordinator
 
 PLATFORMS = [Platform.LIGHT, Platform.SELECT]
@@ -25,7 +25,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: TwinklyConfigEntry) -> b
     # we will be able to properly share the connection.
     host = entry.data[CONF_HOST]
 
-    client = Twinkly(host, async_get_clientsession(hass))
+    client = Twinkly(host, async_get_clientsession(hass), timeout=DEVICE_TIMEOUT)
 
     coordinator = TwinklyCoordinator(hass, entry, client)
 
@@ -47,7 +47,11 @@ async def async_unload_entry(hass: HomeAssistant, entry: TwinklyConfigEntry) ->
 async def async_migrate_entry(hass: HomeAssistant, entry: TwinklyConfigEntry) -> bool:
     """Migrate old entry."""
     if entry.minor_version == 1:
-        client = Twinkly(entry.data[CONF_HOST], async_get_clientsession(hass))
+        client = Twinkly(
+            entry.data[CONF_HOST],
+            async_get_clientsession(hass),
+            timeout=DEVICE_TIMEOUT,
+        )
         try:
             device_info = await client.get_details()
         except (TimeoutError, ClientError) as exception:

homeassistant/components/twinkly/config_flow.py

@@ -12,7 +12,7 @@ from homeassistant.const import CONF_HOST, CONF_ID, CONF_MODEL, CONF_NAME
 from homeassistant.helpers.aiohttp_client import async_get_clientsession
 from homeassistant.helpers.service_info.dhcp import DhcpServiceInfo
 
-from .const import DEV_ID, DEV_MODEL, DEV_NAME, DOMAIN
+from .const import DEV_ID, DEV_MODEL, DEV_NAME, DEVICE_TIMEOUT, DOMAIN
 
 _LOGGER = logging.getLogger(__name__)
 
@@ -40,7 +40,7 @@ class TwinklyConfigFlow(ConfigFlow, domain=DOMAIN):
         if host is not None:
             try:
                 device_info = await Twinkly(
-                    host, async_get_clientsession(self.hass)
+                    host, async_get_clientsession(self.hass), timeout=DEVICE_TIMEOUT
                 ).get_details()
             except TimeoutError, ClientError:
                 errors[CONF_HOST] = "cannot_connect"
@@ -64,7 +64,9 @@ class TwinklyConfigFlow(ConfigFlow, domain=DOMAIN):
         self._async_abort_entries_match({CONF_HOST: discovery_info.ip})
         try:
             device_info = await Twinkly(
-                discovery_info.ip, async_get_clientsession(self.hass)
+                discovery_info.ip,
+                async_get_clientsession(self.hass),
+                timeout=DEVICE_TIMEOUT,
             ).get_details()
         except TimeoutError, ClientError:
             return self.async_abort(reason="cannot_connect")

homeassistant/components/twinkly/const.py

@@ -17,3 +17,7 @@ DEV_PROFILE_RGBW = "RGBW"
 
 # Minimum version required to support effects
 MIN_EFFECT_VERSION = "2.7.1"
+
+# Matches the library default, set explicitly so the integration does not
+# inherit it. A device waking its radio can take several seconds to answer.
+DEVICE_TIMEOUT = 10

homeassistant/components/twinkly/coordinator.py

@@ -1,5 +1,6 @@
 """Coordinator for Twinkly."""
 
+from collections.abc import Awaitable, Callable
 from dataclasses import dataclass
 from datetime import timedelta
 import logging
@@ -58,8 +59,8 @@ class TwinklyCoordinator(DataUpdateCoordinator[TwinklyData]):
     async def _async_setup(self) -> None:
         """Set up the Twinkly data."""
         try:
-            software_version = await self.client.get_firmware_version()
-            self.device_name = (await self.client.get_details())[DEV_NAME]
+            software_version = await self._request(self.client.get_firmware_version)
+            self.device_name = (await self._request(self.client.get_details))[DEV_NAME]
         except (TimeoutError, ClientError) as exception:
             raise UpdateFailed from exception
         self.software_version = software_version["version"]
@@ -67,24 +68,37 @@ class TwinklyCoordinator(DataUpdateCoordinator[TwinklyData]):
             MIN_EFFECT_VERSION
         )
 
+    async def _request[_T](self, request: Callable[[], Awaitable[_T]]) -> _T:
+        """Make a request, retrying it once if it times out.
+
+        A device that stalls one request keeps answering others: it replies on
+        a new connection within milliseconds while the first is still hanging.
+        aiohttp closes a timed-out connection instead of returning it to the
+        pool, so the retry gets a fresh one.
+        """
+        try:
+            return await request()
+        except TimeoutError:
+            return await request()
+
     @override
     async def _async_update_data(self) -> TwinklyData:
         """Fetch data from Twinkly."""
         movies: list[dict[str, Any]] = []
         current_movie: dict[str, Any] = {}
         try:
-            device_info = await self.client.get_details()
-            brightness = await self.client.get_brightness()
-            is_on = await self.client.is_on()
-            mode_data = await self.client.get_mode()
+            device_info = await self._request(self.client.get_details)
+            brightness = await self._request(self.client.get_brightness)
+            is_on = await self._request(self.client.is_on)
+            mode_data = await self._request(self.client.get_mode)
             current_mode = mode_data.get("mode")
             if self.supports_effects:
-                movies = (await self.client.get_saved_movies())["movies"]
+                movies = (await self._request(self.client.get_saved_movies))["movies"]
         except (TimeoutError, ClientError) as exception:
             raise UpdateFailed from exception
         if self.supports_effects:
             try:
-                current_movie = await self.client.get_current_movie()
+                current_movie = await self._request(self.client.get_current_movie)
             except (TwinklyError, TimeoutError, ClientError) as exception:
                 _LOGGER.debug("Error fetching current movie: %s", exception)
         brightness = (

tests/components/twinkly/test_init.py

@@ -15,7 +15,7 @@ from homeassistant.helpers import device_registry as dr, entity_registry as er
 from . import setup_integration
 from .const import TEST_MAC, TEST_MODEL
 
-from tests.common import MockConfigEntry
+from tests.common import MockConfigEntry, async_load_json_object_fixture
 
 
 @pytest.mark.usefixtures("mock_twinkly_client")
@@ -84,3 +84,33 @@ async def test_mac_migration(
         (DOMAIN, config_entry.unique_id), config_entry.entry_id
     ).identifiers == {(DOMAIN, TEST_MAC)}
     assert config_entry.unique_id == TEST_MAC
+
+
+@pytest.mark.usefixtures("mock_twinkly_client")
+async def test_request_retried_once_on_timeout(
+    hass: HomeAssistant,
+    mock_config_entry: MockConfigEntry,
+    mock_twinkly_client: AsyncMock,
+) -> None:
+    """A request that times out once is retried, so setup still succeeds."""
+    details = await async_load_json_object_fixture(hass, "get_details.json", DOMAIN)
+    # Only the first call times out; without the retry setup would fail here.
+    mock_twinkly_client.get_details.side_effect = [TimeoutError, details, details]
+
+    await setup_integration(hass, mock_config_entry)
+
+    assert mock_config_entry.state is ConfigEntryState.LOADED
+
+
+async def test_request_gives_up_after_the_retry(
+    hass: HomeAssistant,
+    mock_config_entry: MockConfigEntry,
+    mock_twinkly_client: AsyncMock,
+) -> None:
+    """A request that keeps timing out still fails, after exactly one retry."""
+    mock_twinkly_client.get_details.side_effect = TimeoutError
+
+    await setup_integration(hass, mock_config_entry)
+
+    assert mock_config_entry.state is ConfigEntryState.SETUP_RETRY
+    assert mock_twinkly_client.get_details.call_count == 2