diff --git a/meshtastic/stream_interface.py b/meshtastic/stream_interface.py index 5b244ba0a..584079fb6 100644 --- a/meshtastic/stream_interface.py +++ b/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.""" diff --git a/meshtastic/tcp_interface.py b/meshtastic/tcp_interface.py index e65fc5583..7ba6306a8 100644 --- a/meshtastic/tcp_interface.py +++ b/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""" diff --git a/meshtastic/tests/test_stream_interface.py b/meshtastic/tests/test_stream_interface.py index 0c48cf728..abd26a178 100644 --- a/meshtastic/tests/test_stream_interface.py +++ b/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(): diff --git a/meshtastic/tests/test_tcp_interface.py b/meshtastic/tests/test_tcp_interface.py index 44e79de50..129ad242e 100644 --- a/meshtastic/tests/test_tcp_interface.py +++ b/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