From 8edf4ce0cb1c86d0fff7481afe19d9f6876a25da Mon Sep 17 00:00:00 2001 From: tinegachris Date: Tue, 25 Aug 2026 21:40:42 +0300 Subject: [PATCH] Drop the serial port on a transport error, so the client can reconnect ModbusSerialClient reported itself connected to a port the OS had torn down. send() and recv() let OSError escape with self.socket still set, and since connected is "self.socket is not None" and connect() returns True early on that same test, the client could not be revived, automatically or manually, for the life of the process. This is the same defect #3000 fixed on ModbusTcpClient. The serial client does not inherit from it, so it was not covered, and it has no benign variant: a serial port has no EOF, so the read path is stranded just as thoroughly as the write path. There are also five unguarded surfaces rather than one, and the first one reached, _in_waiting(), is a bare fcntl.ioctl in pyserial that raises a raw OSError rather than a SerialException. send() and recv() now close the port and raise ConnectionException when an operation fails. BlockingIOError and InterruptedError are re-raised untouched, as in #3000, since neither says the transport is dead. No reconnection policy is added, and connect() already calls self.close() in its own exception handler. A device that simply does not answer raises ModbusIOException, which is not an OSError, so a silent slave can never cost the bus its port. --- pymodbus/client/serial.py | 35 ++++++++++++++-------- test/client/test_client_sync.py | 52 +++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-) diff --git a/pymodbus/client/serial.py b/pymodbus/client/serial.py index 3e0238799..b17edca0b 100644 --- a/pymodbus/client/serial.py +++ b/pymodbus/client/serial.py @@ -265,12 +265,18 @@ def send(self, request: bytes, addr: tuple | None = None) -> int: if not self.socket: raise ConnectionException(str(self)) if request: - if waitingbytes := self._in_waiting(): - result = self.socket.read(waitingbytes) - Log.warning("Cleanup recv buffer before send: {}", result, ":hex") - if (size := self.socket.write(request)) is None: # pragma: no cover - size = 0 - return size + try: + if waitingbytes := self._in_waiting(): + result = self.socket.read(waitingbytes) + Log.warning("Cleanup recv buffer before send: {}", result, ":hex") + if (size := self.socket.write(request)) is None: # pragma: no cover + size = 0 + return size + except (BlockingIOError, InterruptedError): + raise + except OSError: + self.close() + raise ConnectionException(str(self)) from None return 0 def _wait_for_data(self) -> int: @@ -296,12 +302,17 @@ def recv(self, size: int | None) -> bytes: """Read data from the underlying descriptor.""" if not self.socket: raise ConnectionException(str(self)) - if size is None: - size = self._wait_for_data() - if size > self._in_waiting(): - self._wait_for_data() - result = self.socket.read(size) - return result + try: + if size is None: + size = self._wait_for_data() + if size > self._in_waiting(): + self._wait_for_data() + return self.socket.read(size) + except (BlockingIOError, InterruptedError): + raise + except OSError: + self.close() + raise ConnectionException(str(self)) from None def is_socket_open(self) -> bool: """Check if socket is open.""" diff --git a/test/client/test_client_sync.py b/test/client/test_client_sync.py index 2f6c7a3a4..da36b8a77 100755 --- a/test/client/test_client_sync.py +++ b/test/client/test_client_sync.py @@ -395,6 +395,32 @@ def test_serial_client_cleanup_buffer_before_send(self, mock_serial): assert not client.send(b"") assert client.send(b"1234") == 4 + def test_serial_client_send_drops_socket_on_os_error(self): + """Test that a port the OS tore down is not left in place as connected.""" + client = ModbusSerialClient("/dev/null") + mock_socket = mock.MagicMock() + mock_socket.in_waiting = 0 + mock_socket.write.side_effect = OSError(5, "Input/output error") + client.socket = mock_socket + with pytest.raises(ConnectionException): + client.send(b"1234") + assert not client.connected + assert client.socket is None + + def test_serial_client_send_keeps_socket_on_transient_error(self): + """Test that a transient write error leaves a healthy port in place.""" + client = ModbusSerialClient("/dev/null") + mock_socket = mock.MagicMock() + mock_socket.in_waiting = 0 + mock_socket.write.side_effect = BlockingIOError( + 11, "Resource temporarily unavailable" + ) + client.socket = mock_socket + with pytest.raises(BlockingIOError): + client.send(b"1234") + assert client.connected + assert client.socket is mock_socket + def test_serial_client_recv(self): """Test the serial client receive method.""" client = ModbusSerialClient("/dev/null") @@ -409,6 +435,32 @@ def test_serial_client_recv(self): assert client.recv(None) == b"" assert client.recv(0) == b"" + def test_serial_client_recv_drops_socket_on_os_error(self): + """Test that a read against a torn-down port drops it rather than escaping.""" + client = ModbusSerialClient("/dev/null") + mock_socket = mock.MagicMock() + mock_socket.in_waiting = 10 + mock_socket.read.side_effect = OSError(5, "Input/output error") + client.socket = mock_socket + with pytest.raises(ConnectionException): + client.recv(4) + assert not client.connected + assert client.socket is None + + def test_serial_client_recv_keeps_socket_on_transient_error(self): + """Test that a transient read error leaves a healthy port in place.""" + client = ModbusSerialClient("/dev/null") + mock_socket = mock.MagicMock() + mock_socket.in_waiting = 10 + mock_socket.read.side_effect = BlockingIOError( + 11, "Resource temporarily unavailable" + ) + client.socket = mock_socket + with pytest.raises(BlockingIOError): + client.recv(4) + assert client.connected + assert client.socket is mock_socket + def test_serial_client_recv_split(self): """Test the serial client receive method.""" client = ModbusSerialClient("/dev/null")