Merge pull request #923 from ianmcorvidae/close-fixes

Fix some leaks/hangs on close: unstarted StreamInterface streams & TCP reader unblock

Author: ianmcorvidae

meshtastic/stream_interface.py

@@ -149,8 +149,11 @@ def close(self) -> None:
             except RuntimeError:
                 # Thread was never started — happens when close() is invoked
                 # from a failed __init__ before connect() could spawn it.
-                # Nothing to join; safe to ignore.
-                pass
+                # In this case there is no reader thread to close the stream.
+                if self.stream is not None:
+                    with contextlib.suppress(Exception):
+                        self.stream.close()
+                    self.stream = None
 
     def _handleLogByte(self, b):
         """Handle a byte that is part of a log message from the device."""

meshtastic/tcp_interface.py

@@ -74,16 +74,17 @@ def myConnect(self) -> None:
     def close(self) -> None:
         """Close a connection to the device"""
         logger.debug("Closing TCP stream")
-        super().close()
         # Sometimes the socket read might be blocked in the reader thread.
-        # Therefore we force the shutdown by closing the socket here
+        # Therefore force a shutdown first to unblock reader thread reads.
         self._wantExit = True
         if self.socket is not None:
             with contextlib.suppress(Exception):  # Ignore errors in shutdown, because we might have a race with the server
                 self._socket_shutdown()
-            self.socket.close()
+            with contextlib.suppress(Exception):
+                self.socket.close()
 
         self.socket = None
+        super().close()
 
     def _writeBytes(self, b: bytes) -> None:
         """Write an array of bytes to our stream and flush"""

meshtastic/tests/test_stream_interface.py

@@ -33,6 +33,17 @@ def test_StreamInterface_close_safe_when_thread_never_started():
     iface.close()
 
 
+@pytest.mark.unit
+@pytest.mark.usefixtures("reset_mt_config")
+def test_StreamInterface_close_when_thread_never_started_closes_stream():
+    """If no reader thread was started, close() should still close the stream."""
+    iface = StreamInterface(noProto=True, connectNow=False)
+    stream = MagicMock()
+    iface.stream = stream
+    iface.close()
+    stream.close.assert_called_once()
+
+
 @pytest.mark.unit
 @pytest.mark.usefixtures("reset_mt_config")
 def test_StreamInterface_init_cleans_up_when_connect_raises():

meshtastic/tests/test_tcp_interface.py

@@ -1,7 +1,7 @@
 """Meshtastic unit tests for tcp_interface.py"""
 
 import re
-from unittest.mock import patch
+from unittest.mock import MagicMock, patch
 
 import pytest
 
@@ -54,3 +54,25 @@ def test_TCPInterface_without_connecting():
     with patch("socket.socket"):
         iface = TCPInterface(hostname="localhost", noProto=True, connectNow=False)
         assert iface.socket is None
+
+
+@pytest.mark.unit
+def test_TCPInterface_close_shutdowns_socket_before_super_close():
+    """Close should unblock socket reads before waiting on StreamInterface.close()."""
+    iface = TCPInterface(hostname="localhost", noProto=True, connectNow=False)
+    sock = MagicMock()
+    iface.socket = sock
+    call_order = []
+
+    with patch.object(TCPInterface, "_socket_shutdown", autospec=True) as mock_shutdown:
+        with patch(
+            "meshtastic.stream_interface.StreamInterface.close", autospec=True
+        ) as mock_super_close:
+            mock_shutdown.side_effect = lambda _self: call_order.append("shutdown")
+            mock_super_close.side_effect = lambda _self: call_order.append("super_close")
+
+            iface.close()
+
+    assert call_order == ["shutdown", "super_close"]
+    sock.close.assert_called_once()
+    assert iface.socket is None