Use a single socket for InterfaceChoice.Default

[bdraco]

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]

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

[bdraco]

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]

[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]

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

[bdraco]

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

[SebasT87]

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. :)

[bdraco]

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]

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

[coveralls]

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

[bdraco]

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

Working as expected

[jstasiak]

Nice!

[bdraco]

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

[jstasiak]

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]

Looking.. I think it just needs to be casted

[bdraco]

#333

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

[jstasiak]

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

[jstasiak]

@bdraco Released in 0.29.0!

[bdraco]

@bdraco Released in 0.29.0!

Thank you 🙏🏻