From f62373269435a3afbee94b7412e93c49d7833fa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Fri, 17 Jul 2026 14:34:18 +0200 Subject: [PATCH] Half-close TCP connections so the last write is not lost close() went straight to shutdown(SHUT_RDWR) + close(). Doing that while either side still has unread data makes the stack send RST rather than FIN, and the device's FromRadio stream means there is essentially always unread data. Winsock discards data that was received but not yet read when an RST arrives, so an admin message written moments earlier is thrown away before the device reads it. Linux delivers the buffered bytes before reporting ECONNRESET, which is why this only surfaced on Windows: every one-shot TCP write there (--set, --seturl, --sendtext) reported success and did nothing. writeConfig() does not wait for an ack, so close() runs microseconds after sendall(). The device cannot compensate; it polls every 5ms and the RST beats its next read. Send FIN first with shutdown(SHUT_WR), which lets the device consume what we wrote, wait briefly for the reader thread to drain and exit, then tear down exactly as before. The full shutdown is kept so a reader still blocked in recv() is unblocked for the join in StreamInterface.close(). The wait is bounded: the device is not obliged to close just because we half-closed. Fixes #957 --- meshtastic/tcp_interface.py | 31 ++++++++++++++++ meshtastic/tests/test_tcp_interface.py | 51 ++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/meshtastic/tcp_interface.py b/meshtastic/tcp_interface.py index 3118f4d26..f1e9a30fe 100644 --- a/meshtastic/tcp_interface.py +++ b/meshtastic/tcp_interface.py @@ -11,6 +11,11 @@ from meshtastic.stream_interface import StreamInterface DEFAULT_TCP_PORT = 4403 + +# How long close() gives the device to consume what we last wrote before forcing +# the connection down. See close() for why this exists. +GRACEFUL_CLOSE_TIMEOUT = 0.25 + logger = logging.getLogger(__name__) @@ -80,6 +85,18 @@ def myConnect(self) -> None: server_address = (self.hostname, self.portNumber) self.socket = socket.create_connection(server_address) + def _wait_for_reader_exit(self, timeout: float) -> None: + """Wait briefly for the reader thread to drain and exit after a half-close. + + Returns as soon as it exits, or after timeout: the device is not obliged + to close just because we did. + """ + rx = getattr(self, "_rxThread", None) + if rx is None or rx is threading.current_thread(): + return + with contextlib.suppress(Exception): + rx.join(timeout) + def close(self) -> None: """Close a connection to the device.""" logger.debug("Closing TCP stream") @@ -87,6 +104,20 @@ def close(self) -> None: # Therefore force a shutdown first to unblock reader thread reads. self._wantExit = True if self.socket is not None: + # Half-close first. shutdown(SHUT_WR) sends FIN, which tells the + # device we are done writing and lets it consume what we last wrote + # before the connection goes away -- typically the admin message a + # one-shot command such as `--set` just sent. + # + # Going straight to shutdown(SHUT_RDWR) + close() while either side + # still has unread data makes the stack send RST instead. Winsock + # then discards data the peer had already received but not yet read, + # so the write is lost; Linux delivers it before reporting + # ECONNRESET, which is why this only bites on Windows. + with contextlib.suppress(Exception): + self.socket.shutdown(socket.SHUT_WR) + self._wait_for_reader_exit(GRACEFUL_CLOSE_TIMEOUT) + with contextlib.suppress( Exception ): # Ignore errors in shutdown, because we might have a race with the server diff --git a/meshtastic/tests/test_tcp_interface.py b/meshtastic/tests/test_tcp_interface.py index 4f0fec9a5..f9e03d565 100644 --- a/meshtastic/tests/test_tcp_interface.py +++ b/meshtastic/tests/test_tcp_interface.py @@ -1,6 +1,7 @@ """Meshtastic unit tests for tcp_interface.py""" import re +import socket from unittest.mock import MagicMock, patch import pytest @@ -78,6 +79,56 @@ def test_TCPInterface_close_shutdowns_socket_before_super_close(): assert iface.socket is None +@pytest.mark.unit +def test_TCPInterface_close_half_closes_before_shutdown(): + """Close should send FIN and let the device drain before forcing the link down. + + Going straight to shutdown(SHUT_RDWR)/close() with data still unread makes the + stack send RST, and Winsock then discards data the device had received but not + yet read, silently losing writes such as `--set`. + """ + 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.object( + TCPInterface, "_wait_for_reader_exit", autospec=True + ) as mock_wait: + with patch( + "meshtastic.stream_interface.StreamInterface.close", autospec=True + ) as mock_super_close: + sock.shutdown.side_effect = lambda how: call_order.append(f"shutdown_{how}") + mock_wait.side_effect = lambda _self, _t: call_order.append("wait") + mock_shutdown.side_effect = lambda _self: call_order.append("full_shutdown") + mock_super_close.side_effect = lambda _self: call_order.append("super_close") + + iface.close() + + assert call_order == [ + f"shutdown_{socket.SHUT_WR}", + "wait", + "full_shutdown", + "super_close", + ] + + +@pytest.mark.unit +def test_TCPInterface_close_survives_half_close_failure(): + """A peer that already vanished must not break close().""" + iface = TCPInterface(hostname="localhost", noProto=True, connectNow=False) + sock = MagicMock() + sock.shutdown.side_effect = OSError("already gone") + iface.socket = sock + + with patch("meshtastic.stream_interface.StreamInterface.close", autospec=True): + iface.close() # must not raise + + sock.close.assert_called_once() + assert iface.socket is None + + @pytest.mark.unit def test_TCPInterface_reconnect(): """Test that _reconnect correctly reconnects"""