Skip to content

fix(s2n-quic): make AsyncWrite::poll_flush a no-op#2347

Merged
camshaft merged 1 commit into
mainfrom
camshaft/flush-change
Oct 9, 2024
Merged

fix(s2n-quic): make AsyncWrite::poll_flush a no-op#2347
camshaft merged 1 commit into
mainfrom
camshaft/flush-change

Conversation

@camshaft

@camshaft camshaft commented Oct 9, 2024

Copy link
Copy Markdown
Contributor

Description of changes:

The current implementation of tokio::io::AsyncWrite::poll_flush performs a full flush of buffers, which means it waits for the peer to acknowledge all of the outstanding in the stream. However, this isn't the behavior most trait usage expects. Instead, it expects poll_flush to flush any temporary buffers to the socket. You can see this expectation in the impl AsyncWrite for TcpStream:

https://github.com/tokio-rs/tokio/blob/679d7657dc267a4d6472597c91cdda8792cf9e97/tokio/src/net/tcp/stream.rs#L1358-L1359

    #[inline]
    fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<io::Result<()>> {
        // tcp flush is a no-op
        Poll::Ready(Ok(()))
    }

An example of our current behavior going wrong is the tokio framed utilities:

https://github.com/tokio-rs/tokio/blob/679d7657dc267a4d6472597c91cdda8792cf9e97/tokio-util/src/codec/framed_impl.rs#L263

    fn poll_ready(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        if self.state.borrow().buffer.len() >= self.state.borrow().backpressure_boundary {
            self.as_mut().poll_flush(cx)
        } else {
            Poll::Ready(Ok(()))
        }
    }

In this case, random calls to poll_write will result in a full flush of the stream, rather than pipelining the buffer. When comparing s2n-quic streams to something like TCP, we will be much worse.

For the solution, I have made all of our poll_flush trait impls as no-ops to match TCP behavior. We preserve the associated poll_flush that keeps the original behavior, in case applications are wanting to do that.

Call-outs:

This is technically a behavior change. It's possible that an application is depending on this behavior to track when the stream was completely ACKed by the peer. But in reality, it's more likely that the application is expecting our trait impls to match what TCP does, which makes no guarantees. So I think the behavior change is preferred.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft
camshaft marked this pull request as ready for review October 9, 2024 17:51
@WesleyRosenblum

Copy link
Copy Markdown
Contributor

I guess you can't really document a trait impl, but do you think it would be worth updating the doc comments on the stream API to mention this behavior in the trait impls?

@camshaft
camshaft force-pushed the camshaft/flush-change branch from 8873e37 to db11425 Compare October 9, 2024 21:21
@camshaft
camshaft enabled auto-merge (squash) October 9, 2024 21:24
@camshaft
camshaft merged commit 0fbb99d into main Oct 9, 2024
@camshaft
camshaft deleted the camshaft/flush-change branch October 9, 2024 21:50
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.

2 participants