Merge pull request #180 from ikalchev/v2.4.2

Release v2.4.2

CHANGELOG.md

@@ -16,6 +16,15 @@ Sections
 ### Developers
 -->
 
+## [2.4.2] - 2019-01-04
+
+### Fixed
+- Fixed an issue where stopping the `AccessoryDriver` can fail with `RuntimeError('dictionary changed size during iteration')`.
+- Fixed an issue where the `HAPServer` can crash when sending events to clients.
+
+### Added
+- Tests for `hap_server`.
+
 ## [2.4.1] - 2018-11-11
 
 ### Fixed

README.md

@@ -1,4 +1,4 @@
-[![PyPI version](https://badge.fury.io/py/HAP-python.svg)](https://badge.fury.io/py/HAP-python) [![Build Status](https://travis-ci.org/ikalchev/HAP-python.svg?branch=master)](https://travis-ci.org/ikalchev/HAP-python) [![codecov](https://codecov.io/gh/ikalchev/HAP-python/branch/master/graph/badge.svg)](https://codecov.io/gh/ikalchev/HAP-python) [![Documentation Status](https://readthedocs.org/projects/hap-python/badge/?version=latest)](http://hap-python.readthedocs.io/en/latest/?badge=latest)
+[![PyPI version](https://badge.fury.io/py/HAP-python.svg)](https://badge.fury.io/py/HAP-python) [![Build Status](https://travis-ci.org/ikalchev/HAP-python.svg?branch=master)](https://travis-ci.org/ikalchev/HAP-python) [![codecov](https://codecov.io/gh/ikalchev/HAP-python/branch/master/graph/badge.svg)](https://codecov.io/gh/ikalchev/HAP-python) [![Documentation Status](https://readthedocs.org/projects/hap-python/badge/?version=latest)](http://hap-python.readthedocs.io/en/latest/?badge=latest) [![Downloads](https://pepy.tech/badge/hap-python)](https://pepy.tech/project/hap-python)
 # HAP-python
 
 HomeKit Accessory Protocol implementation in python 3.

pyhap/const.py

@@ -1,7 +1,7 @@
 """This module contains constants used by other modules."""
 MAJOR_VERSION = 2
 MINOR_VERSION = 4
-PATCH_VERSION = 1
+PATCH_VERSION = 2
 __short_version__ = '{}.{}'.format(MAJOR_VERSION, MINOR_VERSION)
 __version__ = '{}.{}'.format(__short_version__, PATCH_VERSION)
 REQUIRED_PYTHON_VER = (3, 5)

pyhap/hap_server.py

@@ -780,7 +780,7 @@ class HAPServer(socketserver.ThreadingMixIn,
                      b"Content-Length: "
 
     TIMEOUT_ERRNO_CODES = (errno.ECONNRESET, errno.EPIPE, errno.EHOSTUNREACH,
-                           errno.ETIMEDOUT, errno.EHOSTDOWN)
+                           errno.ETIMEDOUT, errno.EHOSTDOWN, errno.EBADF)
 
     @classmethod
     def create_hap_event(cls, bytesdata):
@@ -820,6 +820,8 @@ def _handle_sock_timeout(self, client_addr, exception):
         # NOTE: In python <3.3 socket.timeout is not OSError, hence the above.
         # Also, when it is actually an OSError, it MAY not have an errno equal to
         # ETIMEDOUT.
+        logger.debug("Connection timeout for %s with exception %s", client_addr, exception)
+        logger.debug("Current connections %s", self.connections)
         sock = self.connections.pop(client_addr, None)
         if sock is not None:
             self._close_socket(sock)
@@ -834,26 +836,47 @@ def get_request(self):
         self.connections[client_addr] = client_socket
         return (client_socket, client_addr)
 
-    def finish_request(self, sock, client_addr):
+    def finish_request(self, request, client_address):
+        """Handle the client request.
+
+        HAP connections are not closed. Once the client negotiates a session,
+        the connection is kept open for both incoming and outgoing traffic, including
+        for sending events.
+
+        The client can gracefully close the connection, but in other cases it can just
+        leave, which will result in a timeout. In either case, we need to remove the
+        connection from ``self.connections``, because it could also be used for
+        pushing events to the server.
+        """
         try:
-            self.RequestHandlerClass(sock, client_addr, self, self.accessory_handler)
+            self.RequestHandlerClass(request, client_address,
+                                     self, self.accessory_handler)
         except (OSError, socket.timeout) as e:
-            self._handle_sock_timeout(client_addr, e)
-            logger.debug("Connection timeout")
+            self._handle_sock_timeout(client_address, e)
+            logger.debug('Connection timeout')
+        finally:
+            logger.debug('Cleaning connection to %s', client_address)
+            conn_sock = self.connections.pop(client_address, None)
+            if conn_sock is not None:
+                self._close_socket(conn_sock)
 
     def server_close(self):
         """Close all connections."""
         logger.info('Stopping HAP server')
-        for sock in self.connections.values():
+
+        # When the AccessoryDriver is shutting down, it will stop advertising the
+        # Accessory on the network before stopping the server. At that point, clients
+        # can see the Accessory disappearing and could close the connection. This can
+        # happen while we deal with all connections here so we will get a "changed while
+        # iterating" exception. To avoid that, make a copy and iterate over it instead.
+        for sock in list(self.connections.values()):
             self._close_socket(sock)
         self.connections.clear()
         super().server_close()
 
     def push_event(self, bytesdata, client_addr):
         """Send an event to the current connection with the provided data.
 
-        .. note: Sets a timeout of PUSH_EVENT_TIMEOUT for the duration of socket.sendall.
-
         :param bytesdata: The data to send.
         :type bytesdata: bytes
 
@@ -865,12 +888,14 @@ def push_event(self, bytesdata, client_addr):
         """
         client_socket = self.connections.get(client_addr)
         if client_socket is None:
+            logger.debug('No socket for %s', client_addr)
             return False
         data = self.create_hap_event(bytesdata)
         try:
             client_socket.sendall(data)
             return True
         except (OSError, socket.timeout) as e:
+            logger.debug('exception %s for %s in push_event()', e, client_addr)
             self._handle_sock_timeout(client_addr, e)
             return False
 

tests/test_hap_server.py

@@ -0,0 +1,45 @@
+"""Tests for the HAPServer."""
+from socket import timeout
+from unittest.mock import Mock, MagicMock, patch
+
+import pytest
+
+from pyhap import hap_server
+
+
+@patch('pyhap.hap_server.HAPServer.server_bind', new=MagicMock())
+@patch('pyhap.hap_server.HAPServer.server_activate', new=MagicMock())
+def test_finish_request_pops_socket():
+    """Test that ``finish_request`` always clears the connection after a request."""
+    amock = Mock()
+    client_addr = ('192.168.1.1', 55555)
+    server_addr = ('', 51826)
+
+    # Positive case: The request is handled
+    server = hap_server.HAPServer(server_addr, amock,
+                                  handler_type=lambda *args: MagicMock())
+
+    server.connections[client_addr] = amock
+    server.finish_request(amock, client_addr)
+
+    assert len(server.connections) == 0
+
+    # Negative case: The request fails with a timeout
+    def raises(*args):
+        raise timeout()
+    server = hap_server.HAPServer(server_addr, amock,
+                                  handler_type=raises)
+    server.connections[client_addr] = amock
+    server.finish_request(amock, client_addr)
+
+    assert len(server.connections) == 0
+
+    # Negative case: The request raises some other exception
+    server = hap_server.HAPServer(server_addr, amock,
+                                  handler_type=lambda *args: 1 / 0)
+    server.connections[client_addr] = amock
+
+    with pytest.raises(Exception):
+        server.finish_request(amock, client_addr)
+
+    assert len(server.connections) == 0