Sonos setup fails with unhandled exceptions on discovery messages (#9…

…0648)

Co-authored-by: J. Nick Koston <nick@koston.org>

homeassistant/components/sonos/__init__.py

@@ -368,15 +368,22 @@ async def async_poll_manual_hosts(
         self, now: datetime.datetime | None = None
     ) -> None:
         """Add and maintain Sonos devices from a manual configuration."""
-        for host in self.hosts:
+
+        # Loop through each configured host and verify that Soco attributes are available for it.
+        for host in self.hosts.copy():
             ip_addr = await self.hass.async_add_executor_job(socket.gethostbyname, host)
             soco = SoCo(ip_addr)
             try:
                 visible_zones = await self.hass.async_add_executor_job(
                     sync_get_visible_zones,
                     soco,
                 )
-            except (OSError, SoCoException, Timeout) as ex:
+            except (
+                OSError,
+                SoCoException,
+                Timeout,
+                asyncio.TimeoutError,
+            ) as ex:
                 if not self.hosts_in_error.get(ip_addr):
                     _LOGGER.warning(
                         "Could not get visible Sonos devices from %s: %s", ip_addr, ex
@@ -386,31 +393,30 @@ async def async_poll_manual_hosts(
                     _LOGGER.debug(
                         "Could not get visible Sonos devices from %s: %s", ip_addr, ex
                     )
+                continue
 
-            else:
-                if self.hosts_in_error.pop(ip_addr, None):
-                    _LOGGER.info("Connection restablished to Sonos device %s", ip_addr)
-                if new_hosts := {
-                    x.ip_address
-                    for x in visible_zones
-                    if x.ip_address not in self.hosts
-                }:
-                    _LOGGER.debug("Adding to manual hosts: %s", new_hosts)
-                    self.hosts.update(new_hosts)
-                async_dispatcher_send(
-                    self.hass,
-                    f"{SONOS_SPEAKER_ACTIVITY}-{soco.uid}",
-                    "manual zone scan",
-                )
-                break
+            if self.hosts_in_error.pop(ip_addr, None):
+                _LOGGER.info("Connection reestablished to Sonos device %s", ip_addr)
+            # Each speaker has the topology for other online speakers, so add them in here if they were not
+            # configured. The metadata is already in Soco for these.
+            if new_hosts := {
+                x.ip_address for x in visible_zones if x.ip_address not in self.hosts
+            }:
+                _LOGGER.debug("Adding to manual hosts: %s", new_hosts)
+                self.hosts.update(new_hosts)
 
-        for host in self.hosts.copy():
-            ip_addr = await self.hass.async_add_executor_job(socket.gethostbyname, host)
             if self.is_device_invisible(ip_addr):
                 _LOGGER.debug("Discarding %s from manual hosts", ip_addr)
                 self.hosts.discard(ip_addr)
-                continue
 
+        # Loop through each configured host that is not in error.  Send a discovery message
+        # if a speaker does not already exist, or ping the speaker if it is unavailable.
+        for host in self.hosts.copy():
+            ip_addr = await self.hass.async_add_executor_job(socket.gethostbyname, host)
+            soco = SoCo(ip_addr)
+            # Skip hosts that are in error to avoid blocking call on soco.uuid in event loop
+            if self.hosts_in_error.get(ip_addr):
+                continue
             known_speaker = next(
                 (
                     speaker
@@ -420,12 +426,28 @@ async def async_poll_manual_hosts(
                 None,
             )
             if not known_speaker:
-                await self._async_handle_discovery_message(
-                    soco.uid, ip_addr, "manual zone scan"
-                )
+                try:
+                    await self._async_handle_discovery_message(
+                        soco.uid,
+                        ip_addr,
+                        "manual zone scan",
+                    )
+                except (
+                    OSError,
+                    SoCoException,
+                    Timeout,
+                    asyncio.TimeoutError,
+                ) as ex:
+                    _LOGGER.warning("Discovery message failed to %s : %s", ip_addr, ex)
             elif not known_speaker.available:
                 try:
                     await self.hass.async_add_executor_job(known_speaker.ping)
+                    # Only send the message if the ping was successful.
+                    async_dispatcher_send(
+                        self.hass,
+                        f"{SONOS_SPEAKER_ACTIVITY}-{soco.uid}",
+                        "manual zone scan",
+                    )
                 except SonosUpdateError:
                     _LOGGER.debug(
                         "Manual poll to %s failed, keeping unavailable", ip_addr

tests/components/sonos/conftest.py

@@ -13,13 +13,33 @@
 from tests.common import MockConfigEntry, load_fixture
 
 
+class SonosMockEventListener:
+    """Mock the event listener."""
+
+    def __init__(self, ip_address: str) -> None:
+        """Initialize the mock event listener."""
+        self.address = [ip_address, "8080"]
+
+
+class SonosMockSubscribe:
+    """Mock the subscription."""
+
+    def __init__(self, ip_address: str, *args, **kwargs) -> None:
+        """Initialize the mock subscriber."""
+        self.event_listener = SonosMockEventListener(ip_address)
+        self.service = Mock()
+
+    async def unsubscribe(self) -> None:
+        """Unsubscribe mock."""
+
+
 class SonosMockService:
     """Mock a Sonos Service used in callbacks."""
 
-    def __init__(self, service_type):
+    def __init__(self, service_type, ip_address="192.168.42.2") -> None:
         """Initialize the instance."""
         self.service_type = service_type
-        self.subscribe = AsyncMock()
+        self.subscribe = AsyncMock(return_value=SonosMockSubscribe(ip_address))
 
 
 class SonosMockEvent:
@@ -84,28 +104,59 @@ def config_entry_fixture():
     return MockConfigEntry(domain=DOMAIN, title="Sonos")
 
 
-@pytest.fixture(name="soco")
-def soco_fixture(
-    music_library, speaker_info, current_track_info_empty, battery_info, alarm_clock
-):
-    """Create a mock soco SoCo fixture."""
-    with patch("homeassistant.components.sonos.SoCo", autospec=True) as mock, patch(
-        "socket.gethostbyname", return_value="192.168.42.2"
-    ):
-        mock_soco = mock.return_value
-        mock_soco.ip_address = "192.168.42.2"
-        mock_soco.uid = "RINCON_test"
+class MockSoCo(MagicMock):
+    """Mock the Soco Object."""
+
+    @property
+    def visible_zones(self):
+        """Return visible zones and allow property to be overridden by device classes."""
+        return {self}
+
+
+class SoCoMockFactory:
+    """Factory for creating SoCo Mocks."""
+
+    def __init__(
+        self,
+        music_library,
+        speaker_info,
+        current_track_info_empty,
+        battery_info,
+        alarm_clock,
+    ) -> None:
+        """Initialize the mock factory."""
+        self.mock_list: dict[str, MockSoCo] = {}
+        self.music_library = music_library
+        self.speaker_info = speaker_info
+        self.current_track_info = current_track_info_empty
+        self.battery_info = battery_info
+        self.alarm_clock = alarm_clock
+
+    def cache_mock(
+        self, mock_soco: MockSoCo, ip_address: str, name: str = "Zone A"
+    ) -> MockSoCo:
+        """Put a user created mock into the cache."""
+        mock_soco.mock_add_spec(SoCo)
+        mock_soco.ip_address = ip_address
+        if ip_address != "192.168.42.2":
+            mock_soco.uid = f"RINCON_test_{ip_address}"
+        else:
+            mock_soco.uid = "RINCON_test"
         mock_soco.play_mode = "NORMAL"
-        mock_soco.music_library = music_library
-        mock_soco.get_current_track_info.return_value = current_track_info_empty
+        mock_soco.music_library = self.music_library
+        mock_soco.get_current_track_info.return_value = self.current_track_info
         mock_soco.music_source_from_uri = SoCo.music_source_from_uri
-        mock_soco.get_speaker_info.return_value = speaker_info
-        mock_soco.avTransport = SonosMockService("AVTransport")
-        mock_soco.renderingControl = SonosMockService("RenderingControl")
-        mock_soco.zoneGroupTopology = SonosMockService("ZoneGroupTopology")
-        mock_soco.contentDirectory = SonosMockService("ContentDirectory")
-        mock_soco.deviceProperties = SonosMockService("DeviceProperties")
-        mock_soco.alarmClock = alarm_clock
+        my_speaker_info = self.speaker_info.copy()
+        my_speaker_info["zone_name"] = name
+        my_speaker_info["uid"] = mock_soco.uid
+        mock_soco.get_speaker_info = Mock(return_value=my_speaker_info)
+
+        mock_soco.avTransport = SonosMockService("AVTransport", ip_address)
+        mock_soco.renderingControl = SonosMockService("RenderingControl", ip_address)
+        mock_soco.zoneGroupTopology = SonosMockService("ZoneGroupTopology", ip_address)
+        mock_soco.contentDirectory = SonosMockService("ContentDirectory", ip_address)
+        mock_soco.deviceProperties = SonosMockService("DeviceProperties", ip_address)
+        mock_soco.alarmClock = self.alarm_clock
         mock_soco.mute = False
         mock_soco.night_mode = True
         mock_soco.dialog_level = True
@@ -123,11 +174,48 @@ def soco_fixture(
         mock_soco.surround_level = 3
         mock_soco.music_surround_level = 4
         mock_soco.soundbar_audio_input_format = "Dolby 5.1"
-        mock_soco.get_battery_info.return_value = battery_info
+        mock_soco.get_battery_info.return_value = self.battery_info
         mock_soco.all_zones = {mock_soco}
-        mock_soco.visible_zones = {mock_soco}
         mock_soco.group.coordinator = mock_soco
-        yield mock_soco
+        self.mock_list[ip_address] = mock_soco
+        return mock_soco
+
+    def get_mock(self, *args) -> SoCo:
+        """Return a mock."""
+        if len(args) > 0:
+            ip_address = args[0]
+        else:
+            ip_address = "192.168.42.2"
+        if ip_address in self.mock_list:
+            return self.mock_list[ip_address]
+        mock_soco = MockSoCo(name=f"Soco Mock {ip_address}")
+        self.cache_mock(mock_soco, ip_address)
+        return mock_soco
+
+
+def patch_gethostbyname(host: str) -> str:
+    """Mock to return host name as ip address for testing."""
+    return host
+
+
+@pytest.fixture(name="soco_factory")
+def soco_factory(
+    music_library, speaker_info, current_track_info_empty, battery_info, alarm_clock
+):
+    """Create factory for instantiating SoCo mocks."""
+    factory = SoCoMockFactory(
+        music_library, speaker_info, current_track_info_empty, battery_info, alarm_clock
+    )
+    with patch("homeassistant.components.sonos.SoCo", new=factory.get_mock), patch(
+        "socket.gethostbyname", side_effect=patch_gethostbyname
+    ), patch("homeassistant.components.sonos.ZGS_SUBSCRIPTION_TIMEOUT", 0):
+        yield factory
+
+
+@pytest.fixture(name="soco")
+def soco_fixture(soco_factory):
+    """Create a default mock soco SoCo fixture."""
+    return soco_factory.get_mock()
 
 
 @pytest.fixture(autouse=True)
@@ -172,7 +260,7 @@ async def do_callback(hass, callback, *args, **kwargs):
 @pytest.fixture(name="config")
 def config_fixture():
     """Create hass config fixture."""
-    return {DOMAIN: {MP_DOMAIN: {CONF_HOSTS: ["192.168.42.1"]}}}
+    return {DOMAIN: {MP_DOMAIN: {CONF_HOSTS: ["192.168.42.2"]}}}
 
 
 @pytest.fixture(name="music_library")