fix(http1): flush bytes buffered by the write re-check before yielding - #4143
Open
ricochet wants to merge 1 commit into
Open
fix(http1): flush bytes buffered by the write re-check before yielding#4143ricochet wants to merge 1 commit into
ricochet wants to merge 1 commit into
Conversation
`poll_loop`'s main path always calls `poll_flush` after `poll_write`. The "wants_write_again" re-check added in hyperium#3988 calls `poll_write` a second time and returns straight out of the loop when it pends, skipping that flush. That second write can buffer bytes before it pends. When a response body reaches end-of-stream between the two write polls, `end_body()` buffers the end of the message and the write then pends on the *next* message (`poll_msg`). Returning there strands the terminating chunk in the write buffer: the wake-ups the connection is left waiting on are for reads, so nothing flushes it. The peer receives the body but never the terminator and waits until it gives up, at which point the connection reports `IncompleteMessage` from `mid_message_detect_eof`. Observed on a server streaming a chunked body fed from another thread, at roughly one connection in 600k. hyper's own trace shows the divergence: healthy: buf.len=24, buf.len=5, flushed 29 bytes stalled: buf.len=24, flushed 24 bytes, buf.len=5, <nothing> Flush what the re-check buffered before yielding. Guard the flush on there being buffered bytes so the call pattern is otherwise unchanged. Add a test that drives the interleaving deterministically: a body that yields one data frame, then pends, then ends the stream on the very next poll, all within a single `poll_loop` iteration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
poll_loop's main path always callspoll_flushafterpoll_write. The "wants_write_again" re-check added in #3988 callspoll_writea second time and returns straight out of the loop when it pends, skipping that flush so bytes that second write buffered are left stranded.This is a regression in v1.10.0; v1.9.0 and earlier are unaffected.
An HTTP/1 server streaming a chunked body writes the body but never the terminating
0\r\n\r\n. The connection sits mid-message indefinitely, the peer has the data but is still waiting for the end of the message until it gives up. Then the server reportsIncompleteMessagefrommid_message_detect_eof. Observed on a server whose response body is fed from another task/thread. This requies a multi-threaded runtime to reproduce and is rare (~1 response in 600k under sustained load), because it needs the body to reach end-of-stream in the narrow window between hyper's two write polls. I ran into this while doing intensive benchmark testing.When this scenario occurs, it can be observed in the hyper
h1trace, comparing healthy vs. stalled.On the wire, a raw client sees the response end without its terminator:
The fix
Flush what the re-check buffered before yielding.
The flush is guarded on a new
Conn::has_buffered_write()rather than being unconditional. An unconditional flush breaksready_on_poll_stream::body_test: that fixture's mock pends on odd-numbered flush calls (flush_count % 2 != 0), so it is tuned to hyper's exact flush-call count and an extra call flips the parity. Guarding on there actually being buffered bytes leaves the call pattern unchanged when there is nothing to flush.Test
tests/h1_flush_before_yield.rsdrives the interleaving deterministically with a body that yields one data frame, then pends, then ends the stream on the very next poll, all inside a singlepoll_loopiteration. It fails on master (the response stops after5\r\nhello\r\n) and passes with this change.Verification
This PR adds a deterministic regression test, and I also retested against my benchmark (it's the
service_httpbench at https://wasmcloud.github.io/arewefastyet/).