Drop the serial port on a transport error, so the client can reconnect - #3008
Merged
janiversen merged 1 commit intoAug 25, 2026
Conversation
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 pymodbus-dev#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 pymodbus-dev#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ModbusSerialClientreports itself connected to a port the operating system has torn down, and can never be revived automatically, and not by a caller reconnecting manually.This is the same defect #3000 fixed on
ModbusTcpClient.ModbusSerialClientdoes not inherit from it, so it was not covered, and it has the same three pieces:connectedisself.socket is not None,connect()returnsTrueearly on that same test, and the I/O path letsOSErrorescape without clearingself.socket. The per-requestconnect()inexecute()therefore becomes a no-op, and every later request fails identically for the life of the process.Unguarded surfaces:
sendself._in_waiting()OSErrorsendself.socket.read(waitingbytes)SerialExceptionsendself.socket.write(request)SerialExceptionrecvself._wait_for_data()→_in_waiting()OSErrorrecvself.socket.read(size)SerialException_in_waiting()is reached first on both paths — everysend()calls it before writing and everyrecv()reaches it via_wait_for_data(). In pyserial, it is a barefcntl.ioctl(self.fd, TIOCINQ, ...)with no error handling at all, so it raises a rawOSErrorrather than aSerialException. Wrapping only thewritecall, the shape of #3000, would not catch it.pyserial's
read()detects this condition and raisesSerialExceptionwith the message "device reports readiness to read but returned no data (device disconnected or multiple access on port?)", but pymodbus discards that signal.Reproduction
Verified against
dev(2aa31032). Each of the five surfaces above was driven with what the realtransport raises on a dead port, and in every case the exception escaped
send/recvraw and theport was left in place:
End to end through the public API it is worse than "every request fails": a raw
OSErrorescapesread_holding_registers(), which callers written against the documentedConnectionException/ModbusIOExceptioncontract will not catch.On Linux a pty pair reproduces it with no adapter needed
Output on pymodbus 3.13.0 / pyserial 3.5, Linux, Python 3.11 — a raw
OSErroron bothoperations, forever:
Change
send()andrecv()close the port and raiseConnectionExceptionwhen an operation fails withOSError.No reconnection policy is added — no retry, no delay. This only makes the object's state match
reality after a failed operation, so the manual path works.
connect()already callsself.close()in its own exception handler, so this is the existing pattern in the same class.BlockingIOErrorandInterruptedErrorare re-raised rather than treated as a dead port, for thesame reason #3000 excluded them.
Existing behaviour is unchanged for a normal send (returns the byte count), an absent socket (raises
ConnectionException), and an empty request (returns0).A silent slave does not cost the bus its port
One
ModbusSerialClientserves every device on the bus, so the obvious concern — and the one raised in #2269 — is whether one unresponsive slave now drops the port for all of them. It does not, and the discriminator is exact: a device that does not answer raisesModbusIOException, which is not anOSErrorand never reaches the new handler. pyserial'sread()also breaks out of its own loop on timeout and returns short rather than raising. A timeout is a statement about one device; anOSErroris a statement about the port.On narrowing the exception
Narrowing to
except ConnectionErroris not an option here, unlike on the TCP client.serial.SerialExceptionsubclassesOSErrorbut notConnectionError, and the surface that firesfirst,
_in_waiting(), raises a bareOSError. It would catch nothing. Flagging it before it isproposed.
Tests
Four tests added to
TestSyncClientSerial— both branches of both methods, so the new code is fullycovered:
test_serial_client_send_drops_socket_on_os_errortest_serial_client_send_keeps_socket_on_transient_errortest_serial_client_recv_drops_socket_on_os_errortest_serial_client_recv_keeps_socket_on_transient_error