Pull requests / #2929
#2929 serve: the drain's deadline bounds the whole drain again, not just each chunk header
open · @ctr-137 · 0 commentaires · Sur GitHub
Description
`_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)
Sur le site
Liens install, modèles, releases.