h1 servers can shutdown connections with pending buffered data on filled sockets#4018
Conversation
deven96
changed the title
deven96
changed the title
deven96
changed the title
3de4ad6 to
e786002
Compare
|
After some discussion, decided to not follow the approach of returning pending from within the poll loop as it could cause us to read less frequently ... given that the bug is explicitly that we shouldn't call shutdown whenever buffered hasn't completed then it probably makes sense to ensure that |
e786002 to
0757da0
Compare
|
|
|
Sorry for the delay, just took a look. So, I think ultimately, the In the meantime, this patch does fix it, so I'm inclined to go with it. It looks like CI is angry about an unused method, now. |
cc08d5a to
3c039ea
Compare
Thanks for taking a look @seanmonstar . Yes it was the fragility of alternative changes that caused the shutdown instead ... otherwise the best option would be to the poll_loop itself. Willing to look into it in a separate PR if need be |
|
Hm, is the CI failure relevant? Kinda looks that way, but I'm distracted at the moment: Test output``` Running tests/ready_on_poll_stream.rs (target/debug/deps/ready_on_poll_stream-d4f86d9d0defa2fd)running 1 test failures: ---- body_test stdout ---- thread 'body_test' (3627) panicked at tests/h1_server/fixture.rs:105:17: as core::future::future::Future>::poll failures: test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.73s |
764686d to
faa15de
Compare
faa15de to
f41c288
Compare
Yes |
seanmonstar
left a comment
seanmonstar
left a comment
There was a problem hiding this comment.
Right on, thanks for the fix!
`Buffered::poll_shutdown` reuses `poll_flush` to drain any committed response bytes before closing the connection (added in hyperium#4018). But `poll_flush` short-circuits to `Ready(Ok(()))` whenever the pipeline-flush optimization is enabled and there are still unparsed request bytes in the read buffer: if self.flush_pipeline && !self.read_buf.is_empty() { Poll::Ready(Ok(())) } So when a server is built with `pipeline_flush(true)` and a client pipelines another request into the read buffer, the flush inside `poll_shutdown` becomes a no-op and the connection is torn down with response bytes still sitting in the write buffer. The client gets a truncated (or entirely missing) response even though the status line and Content-Length claimed a complete one. This is the same failure mode as hyperium#4022, reached through the pipeline-flush path that hyperium#4018 didn't cover. A client pipelining after a `Connection: close` request is enough to hit this ("SHOULD NOT", not "MUST NOT" -- RFC 9112 section 9.6). Split the actual buffer-draining out of `poll_flush` into `poll_flush_buffered`, which ignores the `flush_pipeline` short-circuit, and have `poll_shutdown` call it. `poll_flush`'s behavior is unchanged. Adds a unit test that puts a `Buffered` in the exact short-circuit state (pipeline flushing on, a pipelined request in the read buffer, a committed response in the write buffer) and asserts shutdown flushes it first.
…ected EOF (#4797) * fix(deps): pin hyper to flush-before-shutdown fix for large-GET unexpected EOF hyper <= 1.10.1 can call poll_shutdown() on an HTTP/1 socket while response bytes are still buffered (a prior poll_flush() returned Poll::Pending and the result was discarded), so a backpressured/slow-reading peer receives a graceful FIN before the full Content-Length body is flushed. Standard S3 clients (minio-go/warp) report this as sporadic `unexpected EOF` on large-object GET under load (rustfs/backlog#1232). This is the transport-layer bug from Cloudflare's "hyper-bug" writeup — distinct from the EC-reconstruct-desync EOF already fixed via lockstep decode; here the app layer delivers the full body and truncation is purely hyper->socket. Fixed upstream in hyperium/hyper#4018 (commit 72046cc7, "fix(http1): flush buffered data before shutdown"), which is not in any crates.io release yet as of hyper 1.10.1. Pin hyper via [patch.crates-io] to git rev ccc1e850 (a descendant of the fix that still declares version 1.10.1). Use [patch.crates-io], not a git+rev on the workspace `hyper` dependency: the server connection is driven by the transitive hyper-util (conn::auto / GracefulShutdown), and a git source would not unify with the crates.io hyper hyper-util resolves, leaving two hyper copies with the server path still on the buggy one. The patch rewrites the crates.io source globally, so the lockfile holds a single git-sourced hyper. Add rustfs/tests/hyper_h1_shutdown_flush_regression.rs, a deterministic guard (mirrors hyper's own h1_shutdown_while_buffered) that fails if the pin is dropped or hyper is downgraded below the fix. Its load-bearing assertion is a synchronously-set flag, so a slow runner can only under-detect, never false-red. Closes rustfs/backlog#1232. * build(deny): allow the hyperium/hyper git source for the flush-before-shutdown pin cargo-deny's [sources] check denies unknown git sources. The hyper [patch.crates-io] pin added for rustfs/backlog#1232 uses a github.com/hyperium git source, so add it to allow-git with an owner/review note and a removal condition (drop once a released hyper > 1.10.1 carries commit 72046cc7).
3 participants
Target fix for #4022
The fix I am proposing is to simply propagate pending flushes within the poll loop to give the buffer a chance to get fully flushed and the included test just simply asserts that
shutdownwas not called with pending flushesI did notice a previously yanked change on here that modified the poll loop to achieve something similar but this should lead to exiting the poll loop and waiting to get polled again rather than exhausting the CPU