Repository: adafruit/Adafruit_CircuitPython_MiniMQTT (language: python)

Issue:
recv_timeout should be used as a receive timeout in _sock_exact_recv()
`socket_timeout` is period which the underlying implementation will break out from a "select" loop, "recv_timeout" should be the maximum period for which to wait for data from the server. `_sock_exact_recv()` however uses `keep_alive` as a receive timeout:
https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/blob/1c25441a34f4e8d841bdf554ca772e773247eec5/adafruit_minimqtt/adafruit_minimqtt.py#L1118-L1127

By default `socket_timeout` is 1 second, `recv_timeout` is 10 seconds and `keep_alive` is 60 seconds.
As a result, when a server disconnects the client and the client pings it, it will take 60 seconds for the client to discover  that the connection is unresponsive. Instead, `recv_timeout` should be used. The way I understand it is that keep alive is server side feature and should not trump receive timeout on the client. 

~The fix would simplify the various `No data ...` `MMQTTException`s used throughout the code.~



Discussion / hints:

Fix patch:
diff --git a/adafruit_minimqtt/adafruit_minimqtt.py b/adafruit_minimqtt/adafruit_minimqtt.py
index 7ede955..2ca47ae 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
@@ -1068,7 +1070,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 +1081,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 +1091,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)


Test patch:
diff --git a/tests/test_recv_timeout.py b/tests/test_recv_timeout.py
new file mode 100644
index 0000000..f65dc40
--- /dev/null
+++ b/tests/test_recv_timeout.py
@@ -0,0 +1,56 @@
+# 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"""
+
+        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__":
+    main()