Repository: postlund/pyatv
Pull request #2083 (2023-07-02 14:15:42)

Rework zeroconf for unicast scanning

I've tested this on both my large production setups without issues with many airplay, home pods, and apple tv devices.  I think it will solve https://github.com/home-assistant/core/issues/80215

It would be great if someone with an older apple tv could test this.


~~needs https://github.com/python-zeroconf/python-zeroconf/pull/1192~~

~~needs https://github.com/home-assistant/core/pull/95714~~

fixes https://github.com/home-assistant/core/issues/80215

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/postlund/pyatv/pull/2083?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pierre+St%C3%A5hl) Report
Patch coverage: **`100.00`**% and project coverage change: **`-0.25`** :warning:
> Comparison is base [(`7cae1a0`)](https://app.codecov.io/gh/postlund/pyatv/commit/7cae1a0446db329ab24694dc724109961b00658a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pierre+St%C3%A5hl) 88.84% compared to head [(`3b8d2a8`)](https://app.codecov.io/gh/postlund/pyatv/pull/2083?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pierre+St%C3%A5hl) 88.59%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #2083      +/-   ##
==========================================
- Coverage   88.84%   88.59%   -0.25%     
==========================================
  Files         162      162              
  Lines       10863    10862       -1     
==========================================
- Hits         9651     9623      -28     
- Misses       1212     1239      +27     
```


| [Impacted Files](https://app.codecov.io/gh/postlund/pyatv/pull/2083?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pierre+St%C3%A5hl) | Coverage Δ | |
|---|---|---|
| [pyatv/core/scan.py](https://app.codecov.io/gh/postlund/pyatv/pull/2083?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pierre+St%C3%A5hl#diff-cHlhdHYvY29yZS9zY2FuLnB5) | `97.27% <100.00%> (-0.02%)` | :arrow_down: |

... and [5 files with indirect coverage changes](https://app.codecov.io/gh/postlund/pyatv/pull/2083/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pierre+St%C3%A5hl)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/postlund/pyatv/pull/2083?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pierre+St%C3%A5hl).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pierre+St%C3%A5hl).

bdraco: We should try load_from_cache first like we do in aiohomekit

<img width="1144" alt="Screenshot 2023-07-29 at 6 18 00 AM" src="https://github.com/postlund/pyatv/assets/663432/7586f5ad-c389-484d-85f1-84b6066c3a4a">

bdraco: the behavior should be the same exact we don't create hundreds of tasks now if they are just going to return right away because its in the cache
<img width="1151" alt="Screenshot 2023-07-29 at 6 34 45 AM" src="https://github.com/postlund/pyatv/assets/663432/c4a41f98-c5d9-4e7e-8261-58b2fb492a7e">


bdraco: <img width="1145" alt="Screenshot 2023-07-29 at 6 35 40 AM" src="https://github.com/postlund/pyatv/assets/663432/07eb7dc0-ad5f-47df-9f83-a910cff21047">

and if its not in the cache we do create the tasks
bdraco: I'm pretty sure this will solve https://github.com/home-assistant/core/issues/80215 and it doesn't regress any of my production set ups.  It would be nice to get a test from someone else though
bdraco: Since this path is used for HA we could do a release, and bump HA at the top of the month to get a long feedback period
postlund: Just let me know when you are satisfied here and want this merged. I can probably make a release if needed.
bdraco: I think the code is fine. 

I'm internally debating if a test that patches to the code to make sure we do the correct zeroconf calls makes sense or if there is a better way to do it. I think patching is probably the only realistic option since otherwise we end up with an unreliable test that needs specific network conditions 
bdraco: even with checking the cache first, there are still too many requests (this is an existing problem)

https://github.com/python-zeroconf/python-zeroconf/pull/1205
bdraco: fixed that issue. will look at the tests a bit more later
postlund: I suggest patching for tests as well. That's easier and more reliable.
bdraco: Happy with the performance now. Will bump the zeroconf version and add the tests
bdraco: @postlund should be good to go now.


postlund: @bdraco Great! Heading to bed now, but I'll look it over tomorrow and make a new release for you.

Patch:
diff --git a/base_versions.txt b/base_versions.txt
index c689fd80..168d5f1f 100644
--- a/base_versions.txt
+++ b/base_versions.txt
@@ -8,4 +8,4 @@ miniaudio==1.45
 protobuf==4.22.0
 requests==2.23.0
 srptools==0.2.0
-zeroconf==0.38.1
+zeroconf==0.72.0
diff --git a/pyatv/core/scan.py b/pyatv/core/scan.py
index 9075443f..1f760205 100644
--- a/pyatv/core/scan.py
+++ b/pyatv/core/scan.py
@@ -3,7 +3,7 @@
 from abc import ABC, abstractmethod
 import asyncio
 import contextlib
-from ipaddress import IPv4Address, ip_address
+from ipaddress import IPv4Address
 import logging
 import os
 from typing import (
@@ -22,7 +22,7 @@ from typing import (
     cast,
 )
 
-from zeroconf import DNSPointer
+from zeroconf import DNSPointer, IPVersion
 from zeroconf.asyncio import AsyncServiceInfo, AsyncZeroconf
 from zeroconf.const import _CLASS_IN, _TYPE_PTR
 
@@ -322,11 +322,10 @@ def _name_without_type(name: str, type_: str) -> str:
     return name[: -(len(type_) + 1)]
 
 
-def _first_non_link_local_or_non_v6_address(addresses: List[bytes]) -> Optional[str]:
-    """Return the first non ipv6 or non-link local ipv4 address."""
-    for address in addresses:
-        ip_addr = ip_address(address)
-        if not (ip_addr.is_link_local or ip_addr.version == 6):
+def _first_non_link_local_address(addresses: List[IPv4Address]) -> Optional[str]:
+    """Return the first non-link local ipv4 address."""
+    for ip_addr in addresses:
+        if not ip_addr.is_link_local:
             return str(ip_addr)
     return None
 
@@ -391,10 +390,26 @@ class ZeroconfScanner(BaseScanner):
         self, zc_timeout: float
     ) -> Tuple[Dict[str, List[AsyncServiceInfo]], Dict[str, str]]:
         """Lookup services and aggregate them by address."""
+        requests: List[Awaitable[bool]] = []
         infos = self._build_service_info_queries()
-        await asyncio.gather(
-            *[info.async_request(self.zeroconf, zc_timeout) for info in infos]
-        )
+        hosts = self.hosts
+        for info in infos:
+            if info.load_from_cache(self.zeroconf):
+                continue
+            if hosts:
+                for host in hosts:
+                    # Unicast requests
+                    requests.extend(
+                        [
+                            info.async_request(self.zeroconf, zc_timeout, host)
+                            for info in infos
+                        ]
+                    )
+                continue
+            # Multicast requests
+            requests.append(info.async_request(self.zeroconf, zc_timeout))
+        if requests:
+            await asyncio.gather(*requests)
         name_to_model: Dict[str, str] = {}
         services_by_address: Dict[str, List[AsyncServiceInfo]] = {}
         for info in infos:
@@ -405,7 +420,9 @@ class ZeroconfScanner(BaseScanner):
                     with contextlib.suppress(UnicodeDecodeError):
                         name_to_model[name] = model.decode("utf-8")
             else:
-                address = _first_non_link_local_or_non_v6_address(info.addresses)
+                address = _first_non_link_local_address(
+                    info.ip_addresses_by_version(IPVersion.V4Only)
+                )
                 if address:
                     services_by_address.setdefault(address, []).append(info)
         return services_by_address, name_to_model
diff --git a/requirements/requirements.txt b/requirements/requirements.txt
index 0dded67c..e6d60b2c 100644
--- a/requirements/requirements.txt
+++ b/requirements/requirements.txt
@@ -9,4 +9,4 @@ miniaudio==1.59
 protobuf==4.23.4
 requests==2.31.0
 srptools==1.0.1
-zeroconf==0.71.5
+zeroconf==0.72.0


Test patch:
diff --git a/tests/core/test_scan.py b/tests/core/test_scan.py
index a91c1f53..9ea1deed 100644
--- a/tests/core/test_scan.py
+++ b/tests/core/test_scan.py
@@ -167,6 +167,12 @@ COMPANION_LINK_RECORDS = [
     COMPANION_LINK_TXT_RECORD,
 ]
 
+PTR_RECORDS_ONLY = [
+    SLEEP_PROXY_PTR_RECORD,
+    AIRPLAY_PTR_RECORD,
+    RAOP_PTR_RECORD,
+    COMPANION_LINK_PTR_RECORD,
+]
 
 COMPLETE_RECORD_SET_WITH_DEVICE_INFO = [
     DEVICE_A_RECORD,
@@ -342,3 +348,33 @@ async def test_scan_with_zeroconf_missing_companion_link_only():
     assert atv.device_info.model == DeviceModel.AppleTV4KGen2
     await browser.async_cancel()
     await aiozc.async_close()
+
+
+@pytest.mark.asyncio
+async def test_scan_with_zeroconf_multicast_not_found():
+    aiozc, browser = await _create_zc_with_cache(PTR_RECORDS_ONLY)
+    loop = asyncio.get_event_loop()
+    with patch("pyatv.core.scan.AsyncServiceInfo.async_request") as mock_async_request:
+        results = await scan(loop, timeout=0, aiozc=aiozc)
+    assert mock_async_request.mock_calls
+    for call in mock_async_request.mock_calls:
+        # Not called with host argument
+        assert len(call[1]) == 2
+    assert not results
+    await browser.async_cancel()
+    await aiozc.async_close()
+
+
+@pytest.mark.asyncio
+async def test_scan_with_zeroconf_unicast_not_found():
+    aiozc, browser = await _create_zc_with_cache(PTR_RECORDS_ONLY)
+    loop = asyncio.get_event_loop()
+    with patch("pyatv.core.scan.AsyncServiceInfo.async_request") as mock_async_request:
+        results = await scan(loop, timeout=0, aiozc=aiozc, hosts=["127.0.0.1"])
+    assert mock_async_request.mock_calls
+    for call in mock_async_request.mock_calls:
+        # Called with host argument
+        assert call[1][2] == "127.0.0.1"
+    assert not results
+    await browser.async_cancel()
+    await aiozc.async_close()