Pull requests / #2916
#2916 serve: the handler's connection has a deadline, so a peer that has stopped cannot own the server
open · @ctr-137 · 0 comentários · No GitHub
Setup & installServer & APIAMD / HIP
Descrição
A client that has stopped — a silently dead one (no RST, no FIN), or one still connected that never reads its
streamed answer — can own a handler thread for good and, worse, the **engine's single turn**. This gives the
handler's connection a deadline, and everything else that was needed was already written.
### What happens now
The SSE loop writes with `self.wfile.write()` while the generator is suspended inside the `ExitStack` that holds the
fifo (`Service.request`), so every later request queues behind a stream nobody is reading:
```python
with turn: # holds the fifo while the generator is suspended here
...
for c in chunks:
self.wfile.write(b"data: " + json.dumps(c, ...) + b"\n\n") # blocks here, turn still held
```
The disconnect watcher (#430 #431) cannot see this. It polls the socket for *readability* and treats EOF as a
hang-up, which catches a client that closed; a client that is connected and simply not reading is not readable, and
the watcher never closes or shuts down the socket, so it cannot release a write that is already blocked.
### The change
```python
PEER_STALL_S = _timeout_env("STRATA_PEER_STALL_S", 120.0)
...
class Handler(BaseHTTPRequestHandler):
timeout = PEER_STALL_S # socketserver applies it to the connection in setup()
```
`socketserver.StreamRequestHandler.setup()` does `if self.timeout is not None: self.connection.settimeout(...)`, so
this is the deadline for **every socket operation the handler makes** — the request line, the body, and the writes.
That covers three sites of one class at once, including one the audit did not have: a client that connects and sends
nothing at all, not even a request line, parked a thread just as well.
Nothing else had to be written, because the recovery already exists in two places:
- the SSE loop's `except OSError` notes the outcome, calls `cancel.set()` and `chunks.close()` — and closing the
generator unwinds `with turn:`, which releases the turn;
- `BaseHTTPRequestHandler.handle_one_request` catches `TimeoutError` around the whole `do_<method>()` call —
*"a read or a write timed out. Discard this connection"* — as a backstop for anything that escapes the above.
It fires only when nothing has moved in either direction for that long, so a client that is sending or reading,
however slowly, is never cut off. `0` switches it off (wait for ever, as before).
### How this was checked
**Measured against the real server**, as shipped versus with a 2 s deadline:
| | as shipped | with the deadline |
|---|---|---|
| a client that connects and sends nothing | still connected after 6 s; the thread is parked | connection ended after **2.0 s** |
| a client stops reading a stream; a second client asks a short question | **no answer within 30 s** — the turn is held | answered after **1.0 s** |
**`serve/test_connection_deadline.py`** (new, 4 tests) makes both of those regressions permanent: a silent client's
connection is ended at the deadline; a stream nobody reads no longer holds the turn; a reader *slower than the
writer* still receives the whole stream; and `0` switches it off. All four need no GPU and no engine.
With the `timeout` attribute removed and everything else left in place, **two of the four fail** — the silent client
and the held turn — and the other two pass. That is the pair of defects exactly.
**No regressions:** `python -m unittest discover -s serve` → 852 tests, one error, in `test_winjob.py`, which fails
the same way on unmodified `main` (a `tasklist` invocation returning `None` on this machine).
### What I left alone
Nothing about the body read. `serve/test_request_hardening.py`'s case (#2814, pull #2910) gives the body its own
deadline and answers 408, because a body that stops arriving deserves an answer rather than a discarded connection.
With this in place that one is belt-and-braces: if you would rather have a single knob, `BODY_STALL_S` can fold into
`PEER_STALL_S`.
There is no issue for this; it came out of a static audit of the tree rather than a report. Happy to file one if you
would rather triage it there first.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
No site
Links install, modelos, releases.