Drop the socket when a send fails, so the client can reconnect

[tinegachris]

Problem

After a peer resets the connection, ModbusTcpClient keeps a dead socket and reports itself
connected, so it can never be revived — not automatically, and not by a caller reconnecting manually.

send() calls self.socket.send(request) without handling OSError, so the exception escapes and
self.socket is left set. Since connected is self.socket is not None and connect() returns
True early on that same test, the per-request connect() in execute() becomes a no-op and every
later request fails identically for the life of the process.

I understand the sync client does no automatic reconnection (#2320)
and I am not asking for that. The problem is that manual reconnection is also impossible: a
caller cannot re-establish the connection without first calling close(), and connected gives
them no way to discover that this is needed.

This only affects an abortive close. A clean close surfaces in recv as EOF, _handle_abrupt_socket_close()
raises ConnectionException and closes the socket itself, and the client recovers. That asymmetry
means it hits write-heavy clients hardest, since their next operation is a send.

Reproduction

Standalone script — peer closes abortively with SO_LINGER 0

"""Reproduction: ModbusTcpClient keeps a reset socket and reports itself connected."""
import socket
import struct
import threading
import time

from pymodbus.client import ModbusTcpClient

srv = socket.socket()
srv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
srv.bind(("127.0.0.1", 0))
port = srv.getsockname()[1]
srv.listen(1)

conns = []


def accept():
    """Accept connections and park a reader on each."""
    while True:
        try:
            c, _ = srv.accept()
            conns.append(c)
            threading.Thread(target=lambda s=c: s.recv(1024), daemon=True).start()
        except OSError:
            return


threading.Thread(target=accept, daemon=True).start()

client = ModbusTcpClient(host="127.0.0.1", port=port, timeout=1, retries=1)
client.connect()
print("connected:", client.connected)
time.sleep(0.3)

# abortive close -> RST
for c in conns:
    c.setsockopt(socket.SOL_SOCKET, socket.SO_LINGER, struct.pack("ii", 1, 0))
    c.close()
srv.close()
time.sleep(0.3)

ADU = b"\x00\x01\x00\x00\x00\x06\x01\x03\x00\x00\x00\x02"

for attempt in range(1, 5):
    try:
        client.send(ADU)
        print(f"  client.send {attempt}: ok")
    except Exception as e:
        print(f"  client.send {attempt}: {type(e).__name__}: {e}")
    print(
        f"     client.connected={client.connected}  connect()={client.connect()}  "
        f"socket={'set' if client.socket else 'None'}"
    )
    time.sleep(0.2)

client.close()

On dev (5617e05c) the raw OSError escapes client.send() and the client stays "connected" to a
dead socket forever:

connected: True
  client.send 1: ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host
     client.connected=True  connect()=True  socket=set
  client.send 2: ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host
     client.connected=True  connect()=True  socket=set
  client.send 3: ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host
     client.connected=True  connect()=True  socket=set
  client.send 4: ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host
     client.connected=True  connect()=True  socket=set

With this change the socket is dropped and connect() genuinely re-dials (it fails here only
because the test server is gone, which is the correct answer):

connected: True
  client.send 1: ConnectionException: Modbus Error: [Connection] ModbusTcpClient 127.0.0.1:21720
     client.connected=False  connect()=False  socket=None
  client.send 2: ConnectionException: Modbus Error: [Connection] ModbusTcpClient 127.0.0.1:21720
     client.connected=False  connect()=False  socket=None

(Transcript above is Windows/Python 3.12; the same behaviour was originally observed on Linux, where
the errors read [Errno 104] Connection reset by peer and [Errno 32] Broken pipe.)

Seen in production on a controller polling nine devices: after the peer process died and returned,
the read-dominated devices recovered on their own while the one write-heavy device logged 100 broken
pipes, no ConnectionException, and stayed unusable until the application was restarted.

Change

send() closes the socket and raises ConnectionException when the write fails with OSError.

This adds no reconnection policy — no retry, no delay. It only makes the object's state match
reality after a failed write, so the manual path works. connect() already calls self.close() in
its own OSError handler, so this is the existing pattern in the same class.

Existing behaviour of send is unchanged for a normal send, an absent socket, and an empty request.

ModbusTlsClient subclasses ModbusTcpClient and does not override send, so this covers the TLS
client too.

BlockingIOError and InterruptedError are deliberately re-raised rather than treated as a dead
connection. recv() sets the socket non-blocking and never restores it, so from the first read
onward a momentarily full send buffer can raise BlockingIOError — an OSError, but not a
connection failure — and closing on it would drop a healthy connection under load. InterruptedError
(EINTR) is transient for the same reason.

Three things for your call

1. I convert OSError to ConnectionException for consistency with what send/recv already raise
   when the socket is absent, and with the contract execute() declares (:raises ConnectionException:).
   Happy to close and re-raise the original instead if you prefer not to change the exception type.
2. I catch OSError minus the two transient cases. Catching ConnectionError only would be narrower
   and would exclude them automatically, at the cost of missing EBADF/ENOTCONN. Tell me which you
   prefer.
3. recv() has the same shape and no OSError handling. I left it alone to keep this focused — glad
   to extend the patch if you want it covered.

Tests

Two tests added to TestSyncClientTcp:

• test_tcp_client_send_drops_socket_on_os_error — fails on dev today with BrokenPipeError
  escaping send, passes with this change.
• test_tcp_client_send_keeps_socket_on_transient_error — pins the BlockingIOError carve-out so a
  transient write error still leaves a healthy socket in place.

Ran check_ci.sh locally: codespell, ruff (check and format) and pylint (10.00/10) clean, and zuban
clean over pymodbus and test. Full suite 1934 passed, 17 skipped, with no new failures and
coverage unchanged from dev. My local run was on Windows, where zuban additionally reports
pre-existing errors under examples/contrib — the [tool.mypy] exclude = '/contrib/' regex does not
match backslash paths — and test/global/test_logging.py errors in teardown because the log file is
still open. Both are unrelated to this change and do not reproduce on your Linux CI.

[janiversen]

LGTM, thanks.

That explains an old bug we never got pinpointed (solved differently).