Skip to content

fix(http1): flush buffered data on shutdown even when pipeline-flushing - #1

Open
fotinakis wants to merge 1 commit into
masterfrom
fix/http1-flush-buffered-before-shutdown
Open

fix(http1): flush buffered data on shutdown even when pipeline-flushing#1
fotinakis wants to merge 1 commit into
masterfrom
fix/http1-flush-buffered-before-shutdown

Conversation

@fotinakis

Copy link
Copy Markdown
Owner

Summary

pipeline_flush(true) can cause an HTTP/1 server to close a connection with a committed response still buffered, truncating (or entirely dropping) it. This is the same failure mode as hyperium#4022, but reached through a code path that the hyperium#4018 fix doesn't cover.

Background

hyperium#4018 fixed hyperium#4022 by making Buffered::poll_shutdown flush any buffered response bytes before closing:

pub(crate) fn poll_shutdown(&mut self, cx: &mut Context<'_>) -> Poll<io::Result<()>> {
    ready!(self.poll_flush(cx))?;
    Pin::new(&mut self.io).poll_shutdown(cx)
}

The catch is that poll_flush has a short-circuit for the pipeline-flush optimization:

pub(crate) fn poll_flush(&mut self, cx: &mut Context<'_>) -> Poll<io::Result<()>> {
    if self.flush_pipeline && !self.read_buf.is_empty() {
        Poll::Ready(Ok(()))          // <-- returns without flushing
    } else if self.write_buf.remaining() == 0 {
    ...

That short-circuit is correct for a normal flush (hold off so responses to already-queued requests get batched into one write), but poll_shutdown inherits it. So when a server is built with pipeline_flush(true) and there are still unparsed request bytes in the read buffer, the flush inside poll_shutdown becomes a no-op and the connection is torn down with the response still in write_buf.

The client sees a status line and Content-Length promising a full response, a clean connection close, and a short body — no error anywhere.

How to hit it

  • Server built with pipeline_flush(true).
  • A client pipelines another request into the connection while a response is being written, so read_buf is non-empty when the connection goes to close. Pipelining after a Connection: close request is enough — that's a "SHOULD NOT", not a "MUST NOT" (RFC 9112 §9.6).

Fix

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 that instead. poll_flush's behavior is unchanged; only shutdown is affected.

fn poll_flush_buffered(&mut self, cx: &mut Context<'_>) -> Poll<io::Result<()>> {
    // ... the existing drain-write_buf-then-flush logic ...
}

pub(crate) fn poll_shutdown(&mut self, cx: &mut Context<'_>) -> Poll<io::Result<()>> {
    ready!(self.poll_flush_buffered(cx))?;
    Pin::new(&mut self.io).poll_shutdown(cx)
}

Test

Adds a Buffered unit test that puts the connection in exactly the short-circuit state — pipeline flushing on, a pipelined request in the read buffer, a committed response in the write buffer — and asserts shutdown flushes the response before closing. It fails on master ("data left to write") and passes with the fix.

Notes

Found while building a small deterministic simulation harness around the HTTP/1 server (the same kind of setup that surfaced hyperium#4022): randomizing socket buffer sizes, request pipelining, and hyper's own config knobs, with an invariant that every 200 response must deliver exactly Content-Length bytes. With pipeline_flush in the mix this reproduced across a large fraction of seeds; the fix clears all of them.

`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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Slower client can cause h1 server to disconnect without flushing Full body

1 participant