Repository navigation
Sockets: add a per-connection id to server socket sends - #1910
Conversation
send() can now be given the connection_id() the caller captured when it started writing. The id increments on every accept, under the socket's lock, and send() compares it against the current id at transport selection and on every loop iteration (queue time and run time in the Qt backend). A write that outlived its connection - the peer disconnected and a new client was accepted in the meantime - is refused instead of being delivered into the new client's socket. Motivated by the in-app HTTP server: its per-connection task may be inside a long handler call when the client disconnects, and a fast reconnect can be accepted before the task notices; without the id, its final response could be written into the new client's stream. Default argument (0 = no check) keeps existing callers unchanged. Test: Sockets - A send for a replaced connection is refused (runs on the Qt, POSIX, and WinSocket backends).
pa-devbot
left a comment
There was a problem hiding this comment.
AI review: Reviewed all three backends and the new test.
Correctness: POSIX/WinSocket check the id under m_lock at send() entry and re-check on every send-loop iteration (so a long write stops mid-loop if the connection is replaced); Qt also carries the id in the queued SendData and re-checks at run time. m_connection_id is incremented under the lock on every accept, and the default connection = 0 keeps existing callers' behavior unchanged. Test_Sockets_StaleSend covers exactly the motivating race (stale id refused, current id delivered) and runs for every backend.
Verified locally (aarch64): full Sockets suite passes on this branch (30/30, both POSIX and Qt); the stacked HttpServer PR (#1879, rebased onto this branch) passes its full suite (20,588 passed, 0 failed, 5 skipped).
One small doc nit below; otherwise this looks clean and ready.
|
|
||
| // A token identifying the connection the socket is currently | ||
| // serving. It changes every time a new client is accepted, and it | ||
| // is 0 when no connection is active. Capture it when a write |
There was a problem hiding this comment.
AI review: Minor: none of the backends reset m_connection_id when a connection closes, so after the first connection this returns the last id while idle, not 0 (0 only holds before the very first accept). Harmless in practice — a stale id is still refused, and when no connection is active the state check in send() refuses first — but a caller can't use connection_id() == 0 as an "idle" test. Suggest either rewording the comment ("0 before the first connection; otherwise the id of the most recently accepted connection") or actually resetting to 0 on close if the idle⇒0 guarantee is wanted.
There was a problem hiding this comment.
Fixed in c5c9e7f — with a twist: the accessor now reports 0 whenever the socket is not CONNECTED, while the stored counter stays monotonic (resetting the counter itself on close would let a queued write from the old connection match the next connection's id and slip through). send() compares against the counter, so its behavior is unchanged; Test_Sockets_StaleSend now also pins the idle⇒0 state (before the first connect and between connections). Sockets suite 30/30; the stacked #1879 (re-synced onto this commit) passes its full suite (20,588 passed, 0 failed, 5 skipped).
The documented contract ("0 when no connection is active") now holds in
all three backends: the accessor reports 0 whenever the socket is not
CONNECTED. The stored counter stays monotonic - resetting it on close
would let a queued write from the previous connection match the id of
the next one - and send() compares against the counter, so the
stale-send refusal is unaffected. Test_Sockets_StaleSend now also
checks the idle state before and after the first connection.
Suggested by review: the note explaining why the counter is never reset was duplicated in all three backends; it now lives once in the abstract class next to the connection_id() contract. Comment-only change.
72de561 Sockets: add a per-connection id to server socket sends (PokemonAutomation#1910) 8d67694 Home - Detect Selection Hand (PokemonAutomation#1903) e5b3172 Support different game languages (PokemonAutomation#1904) (the FRLG Box Sorter) 14cf718 Recover the boss from the summary when header read is not a boss (PokemonAutomation#1894) No conflicts; none of them touches the AutoStory. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFpKyhKsEg9bJNoPWQzjJJ
Adds
connection_id()toAbstractServerSocket: a token identifying the connection the socket is currently serving. It increments (under the socket's lock) on every accept, andsend()now takes an optionalconnectionargument — a write that outlived its connection (the peer disconnected and a new client was accepted in the meantime) is refused instead of being delivered into the new client's socket.connection_id()returns the current token;send(..., connection)re-checks the token at transport selection and on every send-loop iteration (queue time and run time in the Qt backend).Split out of #1879 (HttpServer) at the maintainer's request: the in-app HTTP server is the motivating use case — its per-connection task may be inside a long handler call when the client disconnects, and a fast reconnect can be accepted before the task notices; without the id, its final response could be written into the new client's stream. #1879 is stacked on this PR.
Tests: new
Test_Sockets_StaleSend(runs for every backend): a send for a replaced connection is refused, a send for the current connection is delivered. Full Sockets suite passes.