Pull requests / #2929

#2929 serve: the drain's deadline bounds the whole drain again, not just each chunk header

open · @ctr-137 · 0 comentários · No GitHub

Server & APIModels & quants

Descrição

`_drain_body`'s docstring says what the drain promises:

> *What has not arrived DRAIN_SECONDS later is left unread: the limit is time, so that a conversation of many
> megabytes, at any speed the client has, still gets its answer.*

The timeout was armed **once per chunk**, at the header, and the chunk was then read with

```python
piece = self.rfile.read(min(left, 1 << 20))
```

which waits for that whole buffer inside one call, looping over socket recvs internally.  Each recv is bounded; the
call is not.  A client that keeps a byte arriving inside the window — comfortably inside it — holds the drain
**for ever**: one MiB announced at one byte every 0.25 s is about three days, and the announced size is bounded only
by `body_limit()` (256 MiB by default).

The route only has to answer *before* reading the body for the drain to run at all, which `GET /nope` (a 404) does.

### The change

The two things that make the bound real are the pair its sibling already uses, in `_drain_body`'s own
Content-Length loop:

- re-arm the deadline before **every** read (`armed()`), so it is checked per socket read and not per chunk;
- `read1`, so one iteration **is** one socket read — the sibling's comment says it: *"one socket read: read() would
  wait for it all"*.

I wrote the first of those two alone first, and it was not enough: the re-armed deadline still sat in front of a
`read()` that would not come back for days.  The test added here is what showed it, and it is what would catch a
regression of either half.

**Scope:** only the drain passes a deadline, and it reads with `keep=False`, so every added branch is inert on the
real body path — a chunked body from `_body()` and the control body both pass no deadline.  The connection is
HTTP/1.0 and ends with the answer either way, so ending the drain early can only ever drop a body that was going to
be dropped.

### How this was checked

Two cases added to `serve/test_chunked_body.py`.

The trickle case: a 404 route that reads no body, 1 MiB announced, one byte every 0.25 s, `DRAIN_SECONDS` 1.0.
**Without the fix it fails** — the client times out at 10.2 s because the drain never gives up — and with it the
connection is let go in 1.6 s.

The control: a complete 2 MiB chunked body is still read in full, and gets the answer that body deserves (a 400 about
the *context*, not about the framing).  That is there because the re-arming must be inert when nothing rushed the
body.

No regressions: `python -m unittest discover -s serve` → 850 tests, 2 failures and 1 error, and **all three fail on
unmodified `main` as well**:

- `test_winjob` — a `tasklist` invocation returns `None` in this environment;
- `test_keepawake` — waits on keep-awake callbacks with `wait_for`, fails alone on `main`;
- `test_responses` — counts threads named `(_loop)` globally, fails alone on `main`.

Both of those last two are load- and order-dependent, which is why the same pair of files failed *differently* in
each arm across runs; each fails on `main` on its own.

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.