Skip to content

fix: avoid querying to refresh types in the ServiceBrowser when they are seen after scheduling#1171

Closed
bdraco wants to merge 4 commits into
masterfrom
refresh_too_soon
Closed

fix: avoid querying to refresh types in the ServiceBrowser when they are seen after scheduling#1171
bdraco wants to merge 4 commits into
masterfrom
refresh_too_soon

Conversation

@bdraco

@bdraco bdraco commented May 4, 2023

Copy link
Copy Markdown
Member

The ServiceBrowser query scheduling was suboptimal because it would reschedule to query for a type when 75% of the ttl was expired, but if the type was seen before the timer expired we would send the query anyways which created more network traffic

fixes #821

…are seen after scheduling

The ServiceBrowser query scheduling was sub-optimial because it would
reschedule to query for a type when 75% of the ttl was expired, but
if the type was seen before the timer expired we would send the query
anyways which created more network traffic

fixes #821
Comment on lines 995 to +998
delay = const._BROWSER_TIME
types_ = {"_hap._tcp.local.", "_http._tcp.local."}
query_scheduler = _services_browser.QueryScheduler(types_, delay, (0, 0))
cache = DNSCache()
query_scheduler = _services_browser.QueryScheduler(cache, types_, delay, (0, 0))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need another query scheduler test that interacts with a populated DNSCache before we can merge this

@bdraco

bdraco commented May 4, 2023

Copy link
Copy Markdown
Member Author

Was trying to fix this before the issue hit the 2 year mark, but out of time to write the test for today

@bdraco

bdraco commented May 4, 2023

Copy link
Copy Markdown
Member Author

The integration test is going to need to set the test PTR record cache to be almost expired as its already way too complex and we don't want to test that there.

We will pickup the coverage in the query scheduler tests

@bdraco

bdraco commented May 4, 2023

Copy link
Copy Markdown
Member Author

The problem is we need two timers.

Once for the regular "next_time" and one for "min_time_to_save_record" so reschedule only affects the second one and we already still do the next_time

@bdraco

bdraco commented May 4, 2023

Copy link
Copy Markdown
Member Author

The downside is that if the network drops for a bit we might not refresh in time but thats probably not a problem since it will eventually recover

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Outgoing query scheduling is suboptimal

1 participant