Skip to content

Reduce chance of accidental synchronization of ServiceInfo requests#955

Merged
bdraco merged 2 commits into
python-zeroconf:masterfrom
bdraco:reduce_accidental_sync
Aug 13, 2021
Merged

Reduce chance of accidental synchronization of ServiceInfo requests#955
bdraco merged 2 commits into
python-zeroconf:masterfrom
bdraco:reduce_accidental_sync

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented Aug 13, 2021

  • ServiceInfo requests are frequently triggered by multicast responses
    to ServiceBrowser requests. When multiple instances of zeroconf are
    present on the same network, they can all end up sending ServiceInfo
    queries at the same time. We now use a random delay as described in
    rfc6762 sec 5.2 after the first request. Ideally we would add the
    delay before the first query as well, however that may break existing
    workflows so it was not done at this time.

Fixes #897

@bdraco
Reduce chance of accidental synchronization of ServiceInfo requests
b5037f4 
- ServiceInfo requests are frequently triggered by multicast responses
  to ServiceBrowser requests.  When multiple instances of zeroconf are
  present on the same network, they can all end up sending ServiceInfo
  queries at the same time.  We now use a random delay as described in
  rfc6762 sec 5.2 after the first request.  Ideally we would add the
  delay before the first query as well, however that may break existing
  workflows so it was not done at this time.
@bdraco bdraco force-pushed the reduce_accidental_sync branch from b47fef3 to b5037f4 Compare August 13, 2021 13:31
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 13, 2021 •
edited
Loading

Codecov Report

Merging #955 (40bb735) into master (5fb3e20) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #955   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2433      2436    +3     
  Branches       401       401           
=========================================
+ Hits          2433      2436    +3
Impacted Files Coverage Δ
zeroconf/_services/info.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fb3e20...40bb735. Read the comment docs.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Aug 13, 2021

Looks like we already have decent coverage for this

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Aug 13, 2021

It would still be good to have a test to make sure we issue 4 queries with the default timeout (3000ms)

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Aug 13, 2021

Tested manually with


class AsyncRunner:
    def __init__(self, ip_version: IPVersion) -> None:
        self.ip_version = ip_version
        self.aiozc: Optional[AsyncZeroconf] = None

    async def test_mcast(self) -> None:
        self.aiozc = AsyncZeroconf(ip_version=self.ip_version)
        zc = self.aiozc.zeroconf
        for i in range(5000):
            info = await self.aiozc.async_get_service_info(
                "_home-assistant._tcp.local.", "806 Alexander._home-assistant._tcp.local."
            )
            import pprint

            pprint.pprint(info)
            assert info is not None
            zc.cache.cache.clear()
            zc.question_history._history.clear()
            info = await self.aiozc.async_get_service_info(
                "_home-assistant._tcp.local.", "Defend-2._home-assistant._tcp.local."
            )
            import pprint

            pprint.pprint(info)
            assert info is not None
            zc.cache.cache.clear()
            zc.question_history._history.clear()


        await self.aiozc.async_close()

Everything was still usable while the above query flood was going on.

@bdraco bdraco merged commit c772936 into python-zeroconf:master Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Try to avoid accidental synchronization of ServiceInfo requests from ServiceBrowser callbacks

2 participants

@bdraco @codecov-commenter