fix(websocket): drain before closing an overflowed peer - #306
Conversation
CoverageTotal lines: 83.17% → 83.18% (+0.01 pp)
|
Review follow-upA Fable Critic read the change against the code and found two things, both fixed here. The fix did nothing over wss, and cost 500 ms. The test comment stated a ceiling the test does not have. It claimed 250 counted attempts kept the old 5 s bound; the socket is blocking with a 2 s read timeout, so against a silent peer that is minutes. The walk now caps the read at 200 ms and takes 60 attempts, and the comment says what that bounds. The Critic also verified, against the code rather than the description, that the dropped Suite after the rework, openssl on PATH so the TLS group executes: 496 tests, 259 executed, 0 failed, 0 warned. |
A peer that overflows the inbound FIFO cap does so by outrunning the reader, so it is still sending when the 1013 goes out. Closing the socket on top of its unread bytes resets the connection, and the reset discards the close. Confirmed by cutting the client write to just past the cap: with nothing left unread the close arrives on every run. The overflow teardown arms the lingering close #288 built for HTTP/1, and only on an upgraded H1 connection — over H2 the session shares the connection with other streams. 035 also called usleep() and microtime(), which made run-tests retry it and report only the retry, hiding a failure that landed on one run in two.
fuzz_ws_frame links ws_session.c without the connection layer, so the overflow teardown left http_connection_linger_begin unresolved. The harness builds its session with conn == NULL, which is the guard the overflow path checks, so the stub satisfies the linker only.
The lingering close is fed from the plaintext read paths, and the TLS read FSM has no hook into them, so a TLS connection that armed it waited with nothing being read and closed on the same unread bytes at the end. Refusing costs nothing that was not already lost and saves the deadline. The gap itself — https losing a 413, wss losing a 1013 — is recorded in dev/PLAN.md. 035 claimed its counted walk kept the old 5 s ceiling. The socket is blocking with a 2 s read timeout, so 250 attempts was minutes, not seconds. The walk caps the read at 200 ms and takes 60 attempts, and the comment states what that bounds.
d34f50b to
9836d88
Compare
1 participant
Closes #305.
A WebSocket peer that floods past the inbound FIFO cap is closed with 1013 and
never reads the code. It overflowed the cap by outrunning the reader, so it is
still sending, and the bytes already on the wire are unread; a socket closed on
top of unread bytes is reset rather than finished, and the reset discards the
1013 this side has written.
#287named the same shape for HTTP/1, where a 413 was lost this way, and#288answered it with a lingering close. The overflow teardown arms that same drain,
carried by the deadline tick that already walks the connection list, so no timer
is added.
Only an upgraded HTTP/1 connection arms it. Over HTTP/2 the session is one
stream among several on a shared connection; stalling that connection to drain
one stream would punish the rest, and RFC 8441 ends the stream instead.
http_connection_linger_beginloses itsparse_error_handledcondition andbecomes callable across the module. Both existing callers set that flag on the
line above the call, so their behaviour is unchanged; the header states the
contract the condition used to imply.
Evidence
The loop that settled it is the test run outside run-tests, which retries any
test whose FILE section calls
usleep()ormicrotime()and reports only theretry — that retry is why a failure landing on one run in two never reached
suite output. Both calls are gone from
035, so it now reports what it found.client saw close: NULLThe 13-frame row is the cause, not a workaround: the condition is the unread
tail, not the volume.
Through run-tests on the fixed build,
035passes 12 of 12. The full Windowssuite is 496 tests, 245 executed, 0 failed, 0 warned.
Not covered
The permessage-deflate teardown (
pmce_error, 1009) returns -1 down the samepath and should lose its close for the same reason, but no run of it has been
made to fail. Recorded as an open item in
dev/PLAN.mdrather than fixed on theresemblance.