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

feat: avoid waking async_request when record updates are not relevant #1153

Merged
merged 36 commits into from Apr 3, 2023

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Apr 3, 2023

Because zc.async_wait() was used, every incoming packet would wake up async_request even if there were not changes.

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (41ea06a) 99.77% compared to head (fc5a986) 99.77%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1153   +/-   ##
=======================================
  Coverage   99.77%   99.77%           
=======================================
  Files          22       22           
  Lines        2646     2654    +8     
  Branches      461      463    +2     
=======================================
+ Hits         2640     2648    +8     
  Misses          4        4           
  Partials        2        2           
Impacted Files Coverage Δ
src/zeroconf/__init__.py 100.00% <100.00%> (ø)
src/zeroconf/_services/info.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/zeroconf/_services/info.py Outdated Show resolved Hide resolved
src/zeroconf/_services/info.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Apr 3, 2023

This should fix all the races with ip address ordering

@bdraco
Copy link
Member Author

bdraco commented Apr 3, 2023

I think we are missing coverage for a few of these races.

We also didn't handle server name changing before correctly

@bdraco
Copy link
Member Author

bdraco commented Apr 3, 2023

Going to need to split this into two PRs

One to fix the races with the server name changing

  • needs test for that
  • needs test for ipv6 address seen last is first
  • needs test for invalid dnsaddress in cache

One to avoid the awake on every packet

@bdraco
Copy link
Member Author

bdraco commented Apr 3, 2023

There is another bug here

self.server needs to start out set to None as well as server_key.

Otherwise we could have addresses in the cache for the ptr name but it actually uses a different name in the dev record and a different address so we end up returning the addresses for the ptr name instead of the srv

Everything that returns self.server might need to be adjusted as well

@bdraco
Copy link
Member Author

bdraco commented Apr 3, 2023

Still need a test for the server changing via a DNSService update

Need to start off with the original name and than change it

@bdraco bdraco marked this pull request as ready for review April 3, 2023 21:09
@bdraco bdraco merged commit a3f970c into master Apr 3, 2023
29 of 30 checks passed
@bdraco bdraco deleted the records branch April 3, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant