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 comments · View on GitHub

Setup & installServer & APIAMD / HIP

Description

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)

Related on this site

Quick links to related pages.