Skip to content

fix(server): treat a client disconnect as a normal end, not an error - #847

Merged
gijzelaerr merged 2 commits into
gijzelaerr:masterfrom
Poseidonas:fix-cotp-disconnect
Sep 2, 2026
Merged

fix(server): treat a client disconnect as a normal end, not an error#847
gijzelaerr merged 2 commits into
gijzelaerr:masterfrom
Poseidonas:fix-cotp-disconnect

Conversation

@Poseidonas

Copy link
Copy Markdown
Contributor

Fixes #846.

Client.disconnect() sends a COTP Disconnect Request, which receive_data() rejected as an unexpected PDU, so an ordinary goodbye from the library's own client was logged as an error:

ERROR  Error handling client ('127.0.0.1', 58143): Expected COTP DT, got 0x80

COTP_DR and COTP_DC were already defined in ServerISOConnection but never used on the receiving side. A DR is now confirmed with a DC and ends the connection through the path _handle_client() already treats as a normal disconnect. Sending the confirmation is best effort — a client that closes right after the request may already be gone, which is what the library's own client does.

accept_connection() logged a peer that leaves before the handshake completes the same way; that is routine, so it is reported at info level now.

Measured, before and after, on the four ways a client can leave — a clean disconnect after a request, a clean disconnect without one, a plain TCP close and an abrupt reset. All four logged an error before; none do now, and the normal disconnect reads:

DEBUG  Received COTP DR from client
INFO   Client ('127.0.0.1', 58156) disconnected

Three tests cover it: the DC framing, that a DR ends the connection and is confirmed, and that it still ends cleanly when the confirmation cannot be sent. Each fails without the change. pytest is 1774 passed, and mypy and ruff report the same counts as master.

One thing I did not touch: snap7/connection.py and snap7/async_client.py have the same Expected COTP DT check on the client side, so a device that sends a DR would be reported as an error there too. I have no hardware to see whether a real CPU does that, so I left it alone rather than guess.

Client.disconnect() sends a COTP Disconnect Request, which receive_data()
rejected as an unexpected PDU, so an ordinary goodbye from the library's
own client was logged as

    ERROR Error handling client ('127.0.0.1', 58143): Expected COTP DT, got 0x80

COTP_DR and COTP_DC were already defined but never used on the receiving
side. A DR is now confirmed with a DC and ends the connection through the
path _handle_client already treats as a normal disconnect. Sending the
confirmation is best effort, since a client that closes right after the
request may already be gone.

A peer that goes away before the ISO handshake completes was logged the
same way; that is routine (port scans, health checks, a cancelled connect)
and is now reported at info level.

Measured against the library's own client and a raw socket: a clean
disconnect after a request, a clean disconnect without one, a plain TCP
close and an abrupt reset all logged an error before, and none do now.

@gijzelaerr gijzelaerr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one remaining noisy path in the same scenario. accept_connection() now logs an early peer departure at info and returns False, but _handle_client() immediately turns that result into logger.warning("Failed to establish ISO connection ..."). Port scans, health checks, and cancelled handshakes therefore still produce a warning even though the new comment says they are routine.

Please change that outer log to the intended normal severity (or return enough information to distinguish routine departure from a malformed handshake), and add a log-level regression test for the complete _handle_client() path. The existing tests call receive_data() directly and do not cover the remaining warning.

@Poseidonas

Copy link
Copy Markdown
Contributor Author

Done — the outer logger.warning is removed rather than lowered.

Every path that makes accept_connection() return False already logs on its own, at the severity it deserves: a routine departure at info, an invalid TPKT version or length at error, a short or wrong-typed COTP CR at error in _parse_cotp_cr(), and a partial frame past the deadline through the generic handler. The outer log was therefore a duplicate in every case, so no extra return value is needed to tell the two apart — the distinction is already in the log.

Added TestHandshakeLogging with four tests that go through the complete _handle_client() path over real sockets, not receive_data() directly: a connect-and-close (port scan), a partial TPKT then close, a malformed TPKT, and a full connect/disconnect with the library's own Client. Each asserts no record at WARNING or above; the malformed one also asserts the ERROR is still there, so the change does not silence a real problem. Three of the four fail against the previous code. The class binds to port 0 and reads the assigned port back, following test_max_clients_is_enforced, so it does not collide when tests run in parallel.

Unrelated, noticed while running the checks: mypy reports snap7/client.py:1404: Redundant cast to "list[dict[str, Any]]" on master as well. The cast after the hasattr(items[0], "Area") branch is no longer needed since the early return already narrows the type; dropping it leaves mypy clean with no other change. Happy to open a separate one-line PR for it if that is useful — it seemed out of place in this one.

@gijzelaerr gijzelaerr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplicate outer warning is gone, malformed handshakes still retain their error log, and the new end-to-end socket tests cover all four requested paths. I also ran the complete server test module locally: 72 passed.

@gijzelaerr
gijzelaerr merged commit b8cec63 into gijzelaerr:master Sep 2, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pure-Python server logs a normal client disconnect as an error

2 participants