postlund / pyatv

A client library for Apple TV and AirPlay devices
https://pyatv.dev
MIT License
891 stars 100 forks source link

Fallback to sending unicast PTR queries when multicast is broken or packets are being dropped #2122

Closed bdraco closed 1 year ago

bdraco commented 1 year ago

fixes https://github.com/home-assistant/core/issues/95768

I came up with a pretty good way to test this. I moved my sonos roam to the edge of wifi range to the point were it had significant packet loss and removed the filters in HA. I'm now able to add the roam when the wifi has significant packet loss. This is effectively the same problem as why Passive Observation Of Failures (POOF) is unviable for setups where the responders are on Wi-Fi

By design we only send one unicast packet to do the fallback query to minimize the network and processing impact. If the network is flakey Home Assistant is going to retry setup again anyways and the next attempt is likely to be successful.

This is a bit complex because the unicast scanner takes a list of hosts so we have to send to handle that case even though in practice we only ever send to one. We may need this behavior in the future so I didn't remove it and make it a single host only.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 99.13% and project coverage change: -0.15% :warning:

Comparison is base (b64bdf9) 88.66% compared to head (75b6a7e) 88.51%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2122 +/- ## ========================================== - Coverage 88.66% 88.51% -0.15% ========================================== Files 163 163 Lines 11001 11070 +69 ========================================== + Hits 9754 9799 +45 - Misses 1247 1271 +24 ``` | [Files Changed](https://app.codecov.io/gh/postlund/pyatv/pull/2122?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/2122?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) | `98.00% <99.12%> (+0.16%)` | :arrow_up: | | [pyatv/\_\_init\_\_.py](https://app.codecov.io/gh/postlund/pyatv/pull/2122?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pierre+St%C3%A5hl#diff-cHlhdHYvX19pbml0X18ucHk=) | `92.68% <100.00%> (ΓΈ)` | | ... and [4 files with indirect coverage changes](https://app.codecov.io/gh/postlund/pyatv/pull/2122/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)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bdraco commented 1 year ago

profile

bdraco commented 1 year ago

performance fix https://github.com/python-zeroconf/python-zeroconf/pull/1211 https://github.com/python-zeroconf/python-zeroconf/pull/1212

bdraco commented 1 year ago

hass bump https://github.com/home-assistant/core/pull/97745

bdraco commented 1 year ago

with the performance changes in zc zc_changes

bdraco commented 1 year ago

performance trade off to solve this seems ok now

bdraco commented 1 year ago

@postlund I ended up going down the rabbit hole quite a bit on this one. This should fix many of the users who have flakey multicast or devices that drop udp packets due to wifi interference/range issues.

I was very careful to minimize the impact on the network from this change. I found a few places were zeroconf has some performance issues that I fixed as a result of getting it wrong the first few designs πŸ˜‰

postlund commented 1 year ago

@bdraco Yeah, I noticed πŸ˜‰ it's great to see improvements in this area though, seems to pop up issues every now and then regarding this. So πŸ‘ I will try to look over this tonight. Do you want to wait for confirmation in https://github.com/home-assistant/core/issues/95768 before I make a release?

bdraco commented 1 year ago

@bdraco Yeah, I noticed πŸ˜‰ it's great to see improvements in this area though, seems to pop up issues every now and then regarding this. So πŸ‘ I will try to look over this tonight. Do you want to wait for confirmation in https://github.com/home-assistant/core/issues/95768 before I make a release?

I ran the Sonos roam at the edge of Wi-Fi range in a reload loop overnight and it stays stable now. It also improved the situation one of my (Wi-Fi connected) HomePod that needs to try twice every so often.

If we don't solve 95768 with this, we likely need more information before I do another run at it.

Since it will at least solve the Sonos roam case it's good to release.

Cheers

bdraco commented 1 year ago

Everything has been rock solid now on all my production setups with the HomePods and Sonos devices moved to the edge of Wi-Fi range

It would be great to get a new release

bdraco commented 1 year ago

Also noticed a nice traffic reduction since it's not having to retry anymore. So even with the unicast ptr probe the net result is less traffic

postlund commented 1 year ago

Awesome! Will push for it!

bdraco commented 1 year ago

thanks