fix: flush data buffered by send_data before the next write and before finish - #360
Open
vcarus wants to merge 1 commit into
Open
fix: flush data buffered by send_data before the next write and before finish#360vcarus wants to merge 1 commit into
vcarus wants to merge 1 commit into
Conversation
…e finish h3's `stream::write` is `send_data` followed by `poll_ready`. When that future is dropped between the two, for example by `tokio::time::timeout` or `select!`, the transport keeps the rest of the frame in its buffer. The next `send_data` on that stream then finds the stream not ready; h3-quinn treats that as a misuse of the trait and answers with `ConnectionErrorIncoming::InternalError`, which closes the whole QUIC connection with H3_INTERNAL_ERROR. On the first request stream of a connection a plain `finish()` is enough to get there, because it writes a grease frame first. Independently, h3-quinn's `poll_finish` sent FIN without flushing that buffer, so the peer saw a truncated frame. `stream::write` now polls `poll_ready` before `send_data` (a no-op when nothing is buffered) and keeps the trailing `poll_ready`, so a write still completes within the call. h3-quinn's `poll_finish` flushes before `finish`, matching s2n-quic-h3's implementation of the same trait. Both paths get a regression test that fails without the corresponding change.
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.
What happens
stream::writein h3 issend_datafollowed bypoll_ready. That future is not cancel-safe: when it is dropped between the two (atokio::time::timeoutaroundRequestStream::send_data, aselect!branch losing, an application deadline), the transport keeps the rest of the frame in itswritingbuffer.From there two things go wrong:
send_dataon that stream findswriting.is_some(). h3-quinn treats that as a misuse of the trait (// This can only happen if the traits are misused by h3 itself) and returnsConnectionErrorIncoming::InternalError, whichhandle_quic_stream_errorturns into closing the whole QUIC connection withH3_INTERNAL_ERROR. Every other request multiplexed on that connection is gone. On the first request stream of a connection a plainfinish()is enough to hit this, becausefinishwrites a grease frame first; on later streams any furthersend_dataorsend_trailersdoes it.poll_finishcallsquinn::SendStream::finish()without flushingwriting, so FIN goes out while the tail of the last DATA frame is still buffered. This one does not even need a cancellation:send_data+poll_finishwith nopoll_readyin between already loses the data.Reproduction for (1), as in the new test: send a response body larger than the peer's stream receive window, poll the
send_datafuture once, drop it, then callfinish(). Before this change the server getsConnectionError(Remote(InternalError("internal error in the http stack")))and the client seesApplicationClose(H3_INTERNAL_ERROR).The change
h3/src/stream.rs:writepollspoll_readybeforesend_dataas well as after. The leading poll is a no-op when nothing is buffered and otherwise flushes the leftover of a cancelled write, sosend_datais never called on a stream that is not ready.h3-quinn/src/lib.rs:poll_finishflushes viapoll_readybeforefinish(). This matches what s2n-quic-h3's implementation of the same trait already does (ready!(self.poll_ready(cx))?; self.stream.finish()); the twosend_dataguards are word-for-word identical, h3-quinn just lacked the flushing half.Two regression tests, one per change; each fails without its change and passes with it (
finish_after_cancelled_send_data_keeps_connection_openintests/request.rs,h3_quinn_finish_flushes_buffered_dataintests/connection.rs).cargo testpasses for the three feature sets CI runs.Relation to #78 / #81
#81 swapped the order to
poll_readythensend_dataand dropped the trailingpoll_ready; that stalled transfers because h3-quinn only writes onpoll_ready, so data sat in the buffer until the next call. This PR keeps the trailingpoll_ready, so a write still completes within the call, and only adds the leading one. It does not change the trait contract or any public API.Found while running a MASQUE (CONNECT-UDP) proxy on h3, where a client that stops reading one tunnel used to take down every other tunnel on the connection.