Use a single socket for InterfaceChoice.Default #331

bdraco · 2021-03-21T19:20:42Z

Fixes #330

When using multiple sockets with multi-cast, the outgoing
socket's responses could be read back on the incoming socket,
which leads to duplicate processing and could fill up the
incoming buffer before it could be processed.

This behavior manifested with error similar to
OSError: [Errno 105] No buffer space available

By using a single socket with InterfaceChoice.Default
we avoid this case.

bdraco · 2021-03-21T19:29:22Z

@jjlawren Can you give this a shot with https://www.home-assistant.io/integrations/zeroconf/#default_interface set to True?

bdraco · 2021-03-21T19:36:00Z

Verified .All still works

[pid 19654] recvfrom(8, "\0\0\204\0\0\0\0\2\0\0\0\33\4_hap\4_tcp\5local\0\0\f\0"..., 8966, 0, {sa_family=AF_INET, sin_port=htons(5353), sin_addr=inet_addr("169.254.236.67")}, [16]) = 1460
[pid 19654] select(18, [8 9 10 12 13 14 16 17], [], [], {5, 0}) = 1 (in [8], left {4, 999997})
[pid 19654] recvfrom(8, "\0\0\204\0\0\0\0\2\0\0\0\33\4_hap\4_tcp\5local\0\0\f\0"..., 8966, 0, {sa_family=AF_INET, sin_port=htons(5353), sin_addr=inet_addr("127.0.0.1")}, [16]) = 1460
[pid 19654] select(18, [8 9 10 12 13 14 16 17], [], [], {5, 0}) = 1 (in [8], left {4, 999998})
[pid 19654] recvfrom(8, "\0\0\204\0\0\0\0\2\0\0\0\33\4_hap\4_tcp\5local\0\0\f\0"..., 8966, 0, {sa_family=AF_INET, sin_port=htons(5353), sin_addr=inet_addr("192.168.107.5")}, [16]) = 1460
[pid 19654] select(18, [8 9 10 12 13 14 16 17], [], [], {5, 0}) = 1 (in [8], left {4, 999998})
[pid 19654] recvfrom(8, "\0\0\204\0\0\0\0\2\0\0\0\33\4_hap\4_tcp\5local\0\0\f\0"..., 8966, 0, {sa_family=AF_INET, sin_port=htons(5353), sin_addr=inet_addr("172.17.0.1")}, [16]) = 1460
[pid 19654] select(18, [8 9 10 12 13 14 16 17], [], [], {5, 0}) = 1 (in [8], left {4, 999998})

12 = UDP *:mdns
13 = is the self pipe
14 = UDP 169.254.236.67:mdns
15 = UDP 127.0.0.1:mdns
16 = UDP 192.168.107.5:mdns
17 = UDP 172.17.0.1:mdns

bdraco · 2021-03-21T19:39:17Z

[pid 20649] select(14, [12 13], [], [], {5, 0}) = 1 (in [12], left {4, 999997})
[pid 20649] recvfrom(12, "\0\0\0\0\0\0\0\3\0\0\0\1\4_hap\4_tcp\5local\0\0\f\0"..., 8966, 0, {sa_family=AF_INET, sin_port=htons(5353), sin_addr=inet_addr("192.168.107.116")}, [16]) = 158

and .Default

12 = UDP *:mdns
13 = is the self pipe

jjlawren · 2021-03-24T17:46:08Z

Ran some basic tests against this branch. Everything seems to be working as I'd expect it to with far less traffic.

bdraco · 2021-03-24T18:01:56Z

I've got it running on 4 systems and all the buffer errors no longer happen :)

SebasT87 · 2021-03-24T18:45:05Z

I would like to give this a try with home assistant. Could you give me some directions on how to get this running within hass?
Thanks!

jstasiak

I swear one of these days I need to make the logic that choses, creates and configures sockets into a testable unit, sometimes I can't wrap my head around this logic anymore.

Can you update the commit message with some details and motivation behind this? I'd like those to be readily available through the commit history rather than needed to be found on GitHub.

Looks good otherwise. :)

zeroconf/__init__.py

bdraco · 2021-03-24T21:01:27Z

Retesting..

diff --git a/zeroconf/__init__.py b/zeroconf/__init__.py
index 9eeaef5..e21c3e8 100644
--- a/zeroconf/__init__.py
+++ b/zeroconf/__init__.py
@@ -2500,7 +2500,6 @@ class Zeroconf(QuietLogger):
         # hook for threads
         self._GLOBAL_DONE = False
         self.unicast = unicast
-        self.interfaces = interfaces
 
         if apple_p2p and not platform.system() == 'Darwin':
             raise RuntimeError('Option `apple_p2p` is not supported on non-Apple platforms.')
@@ -2509,6 +2508,7 @@ class Zeroconf(QuietLogger):
             interfaces, unicast, ip_version, apple_p2p=apple_p2p
         )
         log.debug('Listen socket %s, respond sockets %s', self._listen_socket, self._respond_sockets)
+        self.multi_socket = unicast or interfaces is not InterfaceChoice.Default
 
         self.listeners = []  # type: List[RecordUpdateListener]
         self.browsers = {}  # type: Dict[ServiceListener, ServiceBrowser]
@@ -2528,7 +2528,7 @@ class Zeroconf(QuietLogger):
         self.listener = Listener(self)
         if not unicast:
             self.engine.add_reader(self.listener, cast(socket.socket, self._listen_socket))
-        if unicast or interfaces is not InterfaceChoice.Default:
+        if self.multi_socket:
             for s in self._respond_sockets:
                 self.engine.add_reader(self.listener, s)
         self.reaper = Reaper(self)
@@ -2994,7 +2994,7 @@ class Zeroconf(QuietLogger):
             if not self.unicast:
                 self.engine.del_reader(cast(socket.socket, self._listen_socket))
                 cast(socket.socket, self._listen_socket).close()
-            if self.unicast or self.interfaces is not InterfaceChoice.Default:
+            if self.multi_socket:
                 for s in self._respond_sockets:
                     self.engine.del_reader(s)
             self.engine.join()

bdraco · 2021-03-24T21:05:19Z

Sadly thats not explicit enough since it needs to pickup the unicast cast.

coveralls · 2021-03-24T21:06:11Z

Coverage decreased (-0.002%) to 93.736% when pulling 1ede182 on bdraco:interface_choice_default into 5e268fa on jstasiak:master.

When using multiple sockets with multi-cast, the outgoing socket's responses could be read back on the incoming socket, which leads to duplicate processing and could fill up the incoming buffer before it could be processed. This behavior manifested with error similar to `OSError: [Errno 105] No buffer space available` By using a single socket with InterfaceChoice.Default we avoid this case.

bdraco · 2021-03-24T21:11:55Z

Retested python3 3881 root 6u IPv4 330088 0t0 UDP *:mdns 👍

Working as expected

jstasiak · 2021-03-24T21:13:05Z

Nice!

bdraco · 2021-03-24T21:22:39Z

Thanks, would you mind pushing a release so we can merge home-assistant/core#48302 ?

jstasiak · 2021-03-24T21:26:03Z

Oh yeah, sure. There's an issue with the typing that I haven't spotted before merging, would you mind having a look?

zeroconf/__init__.py:2335: error: List item 0 has incompatible type "Optional[socket]"; expected "socket"

(from https://travis-ci.org/github/jstasiak/python-zeroconf/jobs/764291256)

bdraco · 2021-03-24T21:31:33Z

Looking.. I think it just needs to be casted

bdraco · 2021-03-24T21:36:14Z

#333

Also I can open a PR to switch to GH actions if you like since travis is shutting down .org

jstasiak · 2021-03-24T21:55:58Z

Oh, I'll take care of that in a moment, I have a template for that already, but thank you for the offer.

jstasiak · 2021-03-24T23:16:55Z

@bdraco Released in 0.29.0!

bdraco · 2021-03-25T00:32:35Z

@bdraco Released in 0.29.0!

Thank you 🙏🏻

Fixes a regression caused by refactoring error in python-zeroconf#331

bdraco force-pushed the interface_choice_default branch from 98722b5 to 6f50602 Compare March 21, 2021 19:46

bdraco marked this pull request as ready for review March 24, 2021 18:00

jstasiak approved these changes Mar 24, 2021

View reviewed changes

zeroconf/__init__.py Outdated Show resolved Hide resolved

bdraco force-pushed the interface_choice_default branch from 6f50602 to 921d644 Compare March 24, 2021 21:00

bdraco force-pushed the interface_choice_default branch from 921d644 to 280be4a Compare March 24, 2021 21:06

bdraco force-pushed the interface_choice_default branch from 280be4a to 1ede182 Compare March 24, 2021 21:06

jstasiak merged commit 6beefbb into python-zeroconf:master Mar 24, 2021

bdraco mentioned this pull request Mar 24, 2021

Listen on the default interface by default for zeroconf home-assistant/core#48302

Merged

21 tasks

bdraco added a commit to bdraco/python-zeroconf that referenced this pull request Apr 26, 2021

Skip socket creation if add_multicast_member fails (windows)

57e603c

Fixes a regression caused by refactoring error in python-zeroconf#331

bdraco mentioned this pull request Apr 26, 2021

Skip socket creation if add_multicast_member fails (windows) #341

Merged

bdraco pushed a commit to bdraco/python-zeroconf that referenced this pull request Apr 26, 2021

Skip socket creation if add_multicast_member fails (windows)

fd1c063

Fixes a regression caused by refactoring error in python-zeroconf#331

Use a single socket for InterfaceChoice.Default #331

Use a single socket for InterfaceChoice.Default #331

Uh oh!

Conversation

bdraco commented Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Mar 21, 2021

Uh oh!

bdraco commented Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjlawren commented Mar 24, 2021

Uh oh!

bdraco commented Mar 24, 2021

Uh oh!

SebasT87 commented Mar 24, 2021

Uh oh!

jstasiak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdraco commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Mar 24, 2021

Uh oh!

coveralls commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Mar 24, 2021

Uh oh!

jstasiak commented Mar 24, 2021

Uh oh!

bdraco commented Mar 24, 2021

Uh oh!

jstasiak commented Mar 24, 2021

Uh oh!

bdraco commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jstasiak commented Mar 24, 2021

Uh oh!

jstasiak commented Mar 24, 2021

Uh oh!

bdraco commented Mar 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bdraco commented Mar 21, 2021 •

edited

Loading

bdraco commented Mar 21, 2021 •

edited

Loading

bdraco commented Mar 21, 2021 •

edited

Loading

bdraco commented Mar 24, 2021 •

edited

Loading

coveralls commented Mar 24, 2021 •

edited

Loading

bdraco commented Mar 24, 2021 •

edited

Loading

bdraco commented Mar 24, 2021 •

edited

Loading