Repository navigation
Regression in unique/shared handling for service browser in 0.24.5? #236
Description
Home Assistant uses zeroconf to detect HomeKit (_hap._tcp.local) devices. Since it was updated to use 0.24.5 we've seen a couple of cases where discovery wasn't behaving correctly. With the help of an affected user we ran browser.py. This showed that 0.24.5 without any Home Assistant code triggered the error.
Eventually we discovered that they have devices on their network which have srv records that don't set the record as unique, and since #217 was merged this triggers an assertion in get_service_info:
Exception in thread zeroconf-ServiceBrowser__hap._tcp.local.:
Traceback (most recent call last):
File "/usr/lib/python3.7/threading.py", line 917, in _bootstrap_inner
self.run()
File "/home/pi/homekit-debugging/venv/lib/python3.7/site-packages/zeroconf/__init__.py", line 1504, in run
handler(self.zc)
File "/home/pi/homekit-debugging/venv/lib/python3.7/site-packages/zeroconf/__init__.py", line 1444, in <lambda>
zeroconf=zeroconf, service_type=self.type, name=name, state_change=state_change
File "/home/pi/homekit-debugging/venv/lib/python3.7/site-packages/zeroconf/__init__.py", line 1322, in fire
h(**kwargs)
File "browser.py", line 20, in on_service_state_change
info = zeroconf.get_service_info(service_type, name)
File "/home/pi/homekit-debugging/venv/lib/python3.7/site-packages/zeroconf/__init__.py", line 2191, in get_service_info
if info.request(self, timeout):
File "/home/pi/homekit-debugging/venv/lib/python3.7/site-packages/zeroconf/__init__.py", line 1762, in request
out.add_answer_at_time(zc.cache.get_by_details(self.name, _TYPE_SRV, _CLASS_IN), now)
File "/home/pi/homekit-debugging/venv/lib/python3.7/site-packages/zeroconf/__init__.py", line 907, in add_answer_at_time
assert record.unique
AssertionError
Unplugging these devices allowed things to function normally.
Reverting to 0.24.4 (before #217) also allowed things to function normally.
If i understood correctly the check in #217 was for when publishing a zeroconf record? Is it possible to relax/disalbe the check when browsing? As obviously we can't do anything about vendors implementations of things.
CCing @neejoh as would not have been able to trace this issue without their devices and @mattsaxon in case you can shed any light on this?
Activity
mattsaxon commentedon Apr 1, 2020
I agree that this assertion should only be when publishing information, not browsing.
That is definitely an error as it is acceptable (as per mDNS spec) for other devices to publish non unique records.
As a trivial fix, simply remove these assertions statements.
jstasiak commentedon Apr 1, 2020
I can remove those temporarily assertions and release a new version if this is ok with you @mattsaxon. BTW thank you @Jc2k and @neejoh for the report and analysis.
Jc2k commentedon Apr 1, 2020
Thanks for the quick response everyone! This approach would be great for us if it works for you all! 👍
mattsaxon commentedon Apr 1, 2020
@jstasiak good for me. We can add tests back a bit more selectively following this.
Remove uniqueness assertions
Remove uniqueness assertions
jstasiak commentedon Apr 3, 2020
I removed the assertions, just released version 0.25.0 with this change. Note that there's a backwards incompatible change included since it's been merged earlier: #226
Jc2k commentedon Apr 3, 2020
Thanks @jstasiak ! I've pushed this into a PR for HA, thanks to your quick action it'll hopefully make it into the next release.
jstasiak commentedon Apr 3, 2020
No problem. FYI there's another regression that's yet to be resolved: #235