Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework zeroconf for unicast scanning #2083

Merged
merged 14 commits into from
Aug 3, 2023
Merged

Conversation

bdraco
Copy link
Contributor

@bdraco bdraco commented Jul 2, 2023

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 home-assistant/core#80215

It would be great if someone with an older apple tv could test this.

needs python-zeroconf/python-zeroconf#1192

needs home-assistant/core#95714

fixes home-assistant/core#80215

@codecov
Copy link

codecov bot commented Jul 2, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.12% ⚠️

Comparison is base (dfcc82b) 88.61% compared to head (d5ea8d8) 88.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2083      +/-   ##
==========================================
- Coverage   88.61%   88.50%   -0.12%     
==========================================
  Files         163      163              
  Lines       10991    11001      +10     
==========================================
- Hits         9740     9736       -4     
- Misses       1251     1265      +14     
Files Changed Coverage Δ
pyatv/core/scan.py 97.83% <100.00%> (+0.55%) ⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pyatv/core/scan.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Contributor Author

bdraco commented Jul 29, 2023

We should try load_from_cache first like we do in aiohomekit

Screenshot 2023-07-29 at 6 18 00 AM

@bdraco
Copy link
Contributor Author

bdraco commented Jul 29, 2023

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
Screenshot 2023-07-29 at 6 34 45 AM

@bdraco
Copy link
Contributor Author

bdraco commented Jul 29, 2023

Screenshot 2023-07-29 at 6 35 40 AM

and if its not in the cache we do create the tasks

@bdraco
Copy link
Contributor Author

bdraco commented Jul 29, 2023

I'm pretty sure this will solve home-assistant/core#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 bdraco marked this pull request as ready for review July 29, 2023 13:38
@bdraco
Copy link
Contributor Author

bdraco commented Jul 29, 2023

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
Copy link
Owner

postlund commented Aug 1, 2023

Just let me know when you are satisfied here and want this merged. I can probably make a release if needed.

@bdraco
Copy link
Contributor Author

bdraco commented Aug 1, 2023

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
Copy link
Contributor Author

bdraco commented Aug 1, 2023

even with checking the cache first, there are still too many requests (this is an existing problem)

python-zeroconf/python-zeroconf#1205

@bdraco
Copy link
Contributor Author

bdraco commented Aug 2, 2023

fixed that issue. will look at the tests a bit more later

@postlund
Copy link
Owner

postlund commented Aug 2, 2023

I suggest patching for tests as well. That's easier and more reliable.

@postlund postlund added the scanning Related to scanning, e.g. zeroconf or unicast scanning label Aug 2, 2023
@bdraco
Copy link
Contributor Author

bdraco commented Aug 2, 2023

Happy with the performance now. Will bump the zeroconf version and add the tests

@bdraco
Copy link
Contributor Author

bdraco commented Aug 2, 2023

@postlund should be good to go now.

@postlund
Copy link
Owner

postlund commented Aug 2, 2023

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

@bdraco
Copy link
Contributor Author

bdraco commented Aug 2, 2023

perfect. thanks!

@postlund postlund merged commit 8022f68 into postlund:master Aug 3, 2023
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scanning Related to scanning, e.g. zeroconf or unicast scanning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apple TV stuck on Retrying Setup
2 participants