Keep an accepted connection's peer instead of re-reading it at cleanup - #14
Open
FezVrasta wants to merge 1 commit into
Open
Keep an accepted connection's peer instead of re-reading it at cleanup#14FezVrasta wants to merge 1 commit into
FezVrasta wants to merge 1 commit into
Conversation
close_connection() and handle_writable_socket() both asked the socket itself who the peer was, at points that only happen once the peer is already gone. getpeername() raises there, and neither handler survived it. close_connection() removed the socket from inputs/outputs, then called getpeername() and got [Errno 107]. Everything after it was skipped: the send_queuereg entry was never deleted, the loggerreg entry for the datalogger was never cleaned up, and the socket was never closed - the fd only went away when refcounting happened to collect it. handle_writable_socket() wrapped the same call in a bare except that merely printed and fell through, leaving client_address unbound so the next line raised NameError into the generic handler. On a datalogger that reconnects every minute that is two "exception in server thread" lines per disconnect, which was ~140k log lines a day on the reporter's add-on and buried every real error. Record the peer in self.peers when the connection is accepted, while it is still reachable, and read it from there. close_connection() now closes the socket in a finally block so a failure anywhere above it can no longer strand the fd, and tolerates being called twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tusdu4cS5cacEms4ruvoTt
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.
Independent of #13 — both touch
handle_new_connection, but in different hunks, so they merge cleanly in either order.The bug
close_connection()andhandle_writable_socket()both ask the socket who its peer is, at points that only occur once the peer is already gone.getpeername()raises there, and neither handler survives it.close_connection()removes the socket frominputs/outputs, then callsgetpeername()and gets[Errno 107] Transport endpoint is not connected. Everything after that line is skipped:del send_queuereg[qname]never runsloggerregentry for the datalogger is never cleaned ups.close()never runs — the fd is only released when refcounting happens to collect the sockethandle_writable_socket()wraps the same call in a bareexceptthat prints and falls through withpass, leavingclient_addressunbound. The very next line buildsqnamefrom it and raisesNameError, caught by the generic handler.Observed on a live add-on, one full disconnect cycle:
The datalogger there reconnects about once a minute, so this produced roughly 140k log lines a day and buried every real error.
The fix
Record the peer in
self.peerswhen the connection is accepted — while it is still reachable — and read it from there afterwards.close_connection()now closes the socket in afinallyblock, so a failure anywhere above can no longer strand the fd, and it tolerates being called twice (the select loop can offer an already-reaped socket).No protocol or MQTT behaviour changes.
Tests
tests/test_grottserver_connection_cleanup.py, 9 cases over real loopback sockets: the peer is recorded on accept, cleanup after a disconnect drops the send queue and closes the fd, deregistration is complete, a double close is harmless, the writable handler reports no exception for a socket it no longer knows, and it still delivers queued responses.Two of them assert structurally (AST) that neither handler calls
getpeername()at all. Whether that call actually raises is the kernel's timing to decide — on macOS loopback it can still succeed right after the client closes — so racing a disconnect would make the regression guard flaky. Asserting the call is gone is deterministic on every platform.ruff check --select E9,F63,F7,F82 .passes. The 36 pre-existing failures intest_release_validation.py/test_release_metadata.py/test_addon_runtime_config.pyreproduce unchanged onmaster— they gate on a clean worktree and release environment.