From 07239ac30d2823b77896df45bb026efde197cbea Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Sun, 26 Nov 2023 23:16:28 +0100 Subject: [PATCH 1/4] use recv_timeout instead of keep_alive fixes #189 --- adafruit_minimqtt/adafruit_minimqtt.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/adafruit_minimqtt/adafruit_minimqtt.py b/adafruit_minimqtt/adafruit_minimqtt.py index 7ede9559..66ca7522 100644 --- a/adafruit_minimqtt/adafruit_minimqtt.py +++ b/adafruit_minimqtt/adafruit_minimqtt.py @@ -1068,7 +1068,7 @@ def _sock_exact_recv( to_read = bufsize - recv_len if to_read < 0: raise MMQTTException(f"negative number of bytes to read: {to_read}") - read_timeout = timeout if timeout is not None else self.keep_alive + read_timeout = timeout if timeout is not None else self._recv_timeout mv = mv[recv_len:] while to_read > 0: recv_len = self._sock.recv_into(mv, to_read) @@ -1079,7 +1079,7 @@ def _sock_exact_recv( f"Unable to receive {to_read} bytes within {read_timeout} seconds." ) else: # ESP32SPI Impl. - # This will timeout with socket timeout (not keepalive timeout) + # This will time out with socket timeout (not receive timeout). rc = self._sock.recv(bufsize) if not rc: self.logger.debug("_sock_exact_recv timeout") @@ -1089,7 +1089,7 @@ def _sock_exact_recv( # or raise exception if wait longer than read_timeout to_read = bufsize - len(rc) assert to_read >= 0 - read_timeout = self.keep_alive + read_timeout = self._recv_timeout while to_read > 0: recv = self._sock.recv(to_read) to_read -= len(recv) From 71f9f9d7c9e88278fcc726f742b84840003b79ea Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Sat, 20 Jan 2024 20:28:31 +0100 Subject: [PATCH 2/4] add unit test for recv timeout --- tests/test_recv_timeout.py | 53 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 tests/test_recv_timeout.py diff --git a/tests/test_recv_timeout.py b/tests/test_recv_timeout.py new file mode 100644 index 00000000..0d9d0ee3 --- /dev/null +++ b/tests/test_recv_timeout.py @@ -0,0 +1,53 @@ +# SPDX-FileCopyrightText: 2024 Vladimír Kotal +# +# SPDX-License-Identifier: Unlicense + +"""receive timeout tests""" + +import socket +import time +from unittest import TestCase, main +from unittest.mock import Mock + +import adafruit_minimqtt.adafruit_minimqtt as MQTT + + +class RecvTimeout(TestCase): + """This class contains tests for receive timeout handling.""" + + def test_recv_timeout_vs_keepalive(self) -> None: + """verify that receive timeout as used via ping() is different to keep alive timeout""" + host = "127.0.0.1" + + recv_timeout = 4 + keep_alive = recv_timeout * 2 + mqtt_client = MQTT.MQTT( + broker=host, + socket_pool=socket, + connect_retries=1, + socket_timeout=recv_timeout // 2, + recv_timeout=recv_timeout, + keep_alive=keep_alive, + ) + + # Create a mock socket that will accept anything and return nothing. + socket_mock = Mock() + socket_mock.recv_into = Mock(side_effect=lambda ret_buf, buf_size: 0) + # pylint: disable=protected-access + mqtt_client._sock = socket_mock + + mqtt_client._connected = lambda: True + start = time.monotonic() + with self.assertRaises(MQTT.MMQTTException): + mqtt_client.ping() + + # Verify the mock interactions. + socket_mock.send.assert_called_once() + socket_mock.recv_into.assert_called() + + now = time.monotonic() + assert recv_timeout <= (now - start) < keep_alive + + +if __name__ == "__main__": + main() From 913d2759fb2ddb45792fa355975876ba8a13d2bf Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Sat, 20 Jan 2024 21:31:58 +0100 Subject: [PATCH 3/4] use receive timeout for ping handling as well --- adafruit_minimqtt/adafruit_minimqtt.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/adafruit_minimqtt/adafruit_minimqtt.py b/adafruit_minimqtt/adafruit_minimqtt.py index 66ca7522..2ca47ae8 100644 --- a/adafruit_minimqtt/adafruit_minimqtt.py +++ b/adafruit_minimqtt/adafruit_minimqtt.py @@ -593,7 +593,7 @@ def ping(self) -> list[int]: self._connected() self.logger.debug("Sending PINGREQ") self._sock.send(MQTT_PINGREQ) - ping_timeout = self.keep_alive + ping_timeout = self._recv_timeout stamp = self.get_monotonic_time() self._last_msg_sent_timestamp = stamp rc, rcs = None, [] @@ -602,7 +602,9 @@ def ping(self) -> list[int]: if rc: rcs.append(rc) if self.get_monotonic_time() - stamp > ping_timeout: - raise MMQTTException("PINGRESP not returned from broker.") + raise MMQTTException( + f"PINGRESP not returned from broker within {ping_timeout} seconds." + ) return rcs # pylint: disable=too-many-branches, too-many-statements From 5814fd0bd9792e63f7bc50b2f75801336b138f70 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Sat, 20 Jan 2024 21:53:57 +0100 Subject: [PATCH 4/4] add subtest for timeout exception --- tests/test_recv_timeout.py | 63 ++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/tests/test_recv_timeout.py b/tests/test_recv_timeout.py index 0d9d0ee3..f65dc403 100644 --- a/tests/test_recv_timeout.py +++ b/tests/test_recv_timeout.py @@ -17,36 +17,39 @@ class RecvTimeout(TestCase): def test_recv_timeout_vs_keepalive(self) -> None: """verify that receive timeout as used via ping() is different to keep alive timeout""" - host = "127.0.0.1" - - recv_timeout = 4 - keep_alive = recv_timeout * 2 - mqtt_client = MQTT.MQTT( - broker=host, - socket_pool=socket, - connect_retries=1, - socket_timeout=recv_timeout // 2, - recv_timeout=recv_timeout, - keep_alive=keep_alive, - ) - - # Create a mock socket that will accept anything and return nothing. - socket_mock = Mock() - socket_mock.recv_into = Mock(side_effect=lambda ret_buf, buf_size: 0) - # pylint: disable=protected-access - mqtt_client._sock = socket_mock - - mqtt_client._connected = lambda: True - start = time.monotonic() - with self.assertRaises(MQTT.MMQTTException): - mqtt_client.ping() - - # Verify the mock interactions. - socket_mock.send.assert_called_once() - socket_mock.recv_into.assert_called() - - now = time.monotonic() - assert recv_timeout <= (now - start) < keep_alive + + for side_effect in [lambda ret_buf, buf_size: 0, socket.timeout]: + with self.subTest(): + host = "127.0.0.1" + + recv_timeout = 4 + keep_alive = recv_timeout * 2 + mqtt_client = MQTT.MQTT( + broker=host, + socket_pool=socket, + connect_retries=1, + socket_timeout=recv_timeout // 2, + recv_timeout=recv_timeout, + keep_alive=keep_alive, + ) + + # Create a mock socket that will accept anything and return nothing. + socket_mock = Mock() + socket_mock.recv_into = Mock(side_effect=side_effect) + # pylint: disable=protected-access + mqtt_client._sock = socket_mock + + mqtt_client._connected = lambda: True + start = time.monotonic() + with self.assertRaises(MQTT.MMQTTException): + mqtt_client.ping() + + # Verify the mock interactions. + socket_mock.send.assert_called_once() + socket_mock.recv_into.assert_called() + + now = time.monotonic() + assert recv_timeout <= (now - start) < keep_alive if __name__ == "__main__":