Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Oct 9, 2024

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
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 enabled auto-merge (squash) October 9, 2024 21:24
@camshaft camshaft merged commit 0fbb99d into main Oct 9, 2024
130 checks passed
@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