Repository navigation
Sockets: add an optional deadline to ServerSocket::send() - #1826
Merged
Merged
Conversation
A client that connects but never reads fills the kernel send buffer, and the send() loops on all backends spin on EAGAIN forever, pinning the calling thread (on the Qt backend the undrained write also pins the server's event thread, which blocks close_connection()/close() on it). send() now takes a std::chrono::milliseconds deadline (default: std::chrono::milliseconds::max(), i.e. the previous unbounded behavior). On expiry it stops writing and returns the number of bytes actually written, leaving the connection untouched. - POSIX/Win backends: check the deadline at the top of the send loop. - Qt backend: carry the deadline into the queued write call so the event-thread write loop self-terminates at the deadline, and wait for it on the caller side for the deadline plus a start-up margin (the previous flat 5 s safety is kept for the infinite default). - The interface docs live once, on AbstractServerSocket::send(); the default argument re-declaration stays on each override, since C++ default arguments are not inherited and callers use the concrete type. Known Qt backend caveat (pre-existing): QTcpSocket::write() accepts the whole payload into Qt's userspace buffer and reports it as written, so kernel backpressure never reaches the Qt write loop and a stalled peer shows up as "send completed" (data parked in RAM) rather than a partial count. The bounded-caller guarantee still holds: send() always returns within its deadline. Test: per backend, the client stalls reading, and the server sends 256 MB with a 100 ms deadline. The deadline must engage (partial count on POSIX/Win; on Qt only timing is asserted, per the caveat above). Because an OS with unusually large autotuned socket buffers (observed on Windows CI) can buffer the entire payload in the kernel against a non-reading peer, a send that "completes" is retried: those kernel buffers are now full, so a follow-up send cannot complete and must stop at the deadline (up to 4 times, i.e. unless the OS buffers over 1 GiB). The peer's 12 s stall (longer than the 10 s send threshold, which tolerates loaded CI runners - the pre-fix behavior was an infinite hang) is interruptible, so shutting the peer down does not wait for it. close_connection() must finish promptly afterwards.
pa-devbot
force-pushed
the
audit/server-send-timeout
branch
from
November 2, 2026 07:18
387300a to
66a176c
Compare
The parallel test runner can starve threads for long stretches on loaded CI machines (observed: a 5 s close() threshold flaking at 6 s on the macOS runner), where fixed wall-clock assertions then fail tests that would pass on a healthy machine. - timing_unreliable(): a small fixed workload (32 MB copy + 100 ms sleep) that should take well under a second on a healthy core; if it takes more than 2 s the calling thread is being starved and the caller's timing assertions cannot be trusted. - Every socket test skips (instead of failing) when the machine is starved: up front, and on connect/listen-phase failures. - The send/close deadline tests additionally skip at their threshold crossings, and the close check first round-trips a zero-length send through the socket's event thread: on the Qt backend that is the thread close() has to wait on, so a slow close() with a slow control is the machine's fault, not a bug. - The close() threshold is 10 s instead of 5 s, and all connect/listen/data waits share a 30 s budget instead of 5 s: the pre-fix failure mode is an infinite hang, so the thresholds only have to prove boundedness while tolerating a loaded runner. Also restores Sockets_Tests.cpp's CRLF line endings (matching the rest of the Sockets/ directory); an intermediate tool pass had silently rewritten the file to LF, which the "* -text" .gitattributes surfaced as a whole-file diff.
pa-devbot
force-pushed
the
audit/server-send-timeout
branch
from
November 2, 2026 07:34
66a176c to
029f693
Compare
CI starvation is extreme, so the tests that expect to finish quickly now wait 60 s before giving up: - The shared connect/listen/data budget is 60 s instead of 30 s. - The deadline tests' send and close thresholds are 60 s instead of 10 s; the peer's stall (which must outlast the whole retry chain) is 70 s. It stays interruptible, and the stalling listeners now also set "stop" in their destructors, so an early return (skip) does not make the peer's teardown close() wait out the stall. - The one test that deliberately waits a stall out (Qt completion lifetime) keeps the 5 s waits it was written around. Also fixes the close-check's control round trip: it used a 100 ms send timeout, but the Qt caller-side wait is capped at "timeout + 1 s", so the control could never take more than ~1.1 s and could never expose a starved event thread. It now uses the infinite default (canary: ~0 ms on a healthy event thread, the full 5 s internal safety wait if it is starved) and flags >2 s. Finally, the Qt backends' comment on the 5 s infinite-default wait now states what it is: a limitation of this wrapper, not of Qt - if close() destroys the event loop while a send is still queued, Qt discards the pending event and the "done" flag is never set, so an unbounded wait would hang the caller.
…ests close_connection() on the Qt backend queues the close onto the server's event thread and stops waiting after its internal 5 s bound, so on a starved event thread (Mac CI) it returns with the close still pending and the state still not LISTENING; the queued close does run once the event thread gets to it. The tests that assert LISTENING right after close_connection() now poll for it within the shared 60 s budget first, and skip instead of failing if it still has not applied and the machine is starved. Two tests assert that state: the server send-timeout test (which failed on Mac CI) and the cut-connection-during-send test, which had the identical latent race. Test-only change; the backends are untouched.
This was referenced Nov 2, 2026
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.
Summary
Follow-up to the TCP security audit of
Common/Cpp/Sockets/: a client that connects but never reads fills the kernel send buffer, and thesend()loops on every backend spin on EAGAIN forever, pinning the calling thread. On the Qt backend the undrained write also pins the server's event thread, which makesclose_connection()/close()on that server block indefinitely as well.This PR adds an optional deadline to the server socket:
ms::max()— the previous unbounded behavior; the server socket has no app callers yet, so no caller changes are needed.Per backend
close_connection()(which runs on it) can proceed. The caller-side CV wait becomestimeout + 1 s(queueing margin); the previous flat 5 s safety wait is kept for the infinite default, coveringclose()/close_connection()destroying the server before the queued call runs.AbstractServerSocket::send(); the default-argument re-declaration stays on each override (C++ default arguments are not inherited, and callers use the concrete type).Known Qt backend caveat (pre-existing, not changed by this PR)
QTcpSocket::write()accepts the whole payload into Qt's userspace buffer and reports it as written, so kernel backpressure never reaches the Qt write loop. A stalled peer therefore shows up as "send completed" (data parked in RAM), not as a partial count. The bounded-caller guarantee still holds:send()always returns within its deadline.Test
Test_Sockets_ServerSendTimeout(per backend): loopback pair where the client stalls reading; the server sends 256 MB with a 100 ms deadline;close_connection()must finish afterwards.Also in this PR (second commit)
The loopback tests are now tolerant of thread starvation on loaded CI machines — this fixes the recurring macOS flake in the client test from #1825 (
close() did not finish after the timed-out send (6025 ms)), which this branch carries:timing_unreliable(): a 32 MB copy + 100 ms sleep that should take well under a second on a healthy core; more than 2 s means the calling thread is being starved and the test's timing assertions cannot be trusted.close()waits on), so a slow close with a slow control is the machine's fault, not a bug.Verification
pa-buildclean; fullpa-testcommand-line suite: 20,329 passed, 0 failed, 3 skipped (including all 4 deadline tests).Notes for reviewers
send()semantic differences (Win partial-write return vs POSIX/Qt loop-to-completion; the Qt userspace buffering above).