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 write to check poll_ready before sending data #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

seanmonstar
Copy link
Member

Closes #78

@seanmonstar seanmonstar requested a review from stammw February 10, 2022 01:06
@camshaft
Copy link
Contributor

camshaft commented Feb 12, 2022

The current Quinn implementation doesn't actually send the data on the stream; it just buffers it until a poll_ready is called again. So that would need to change as well.

h3/h3-quinn/src/lib.rs

Lines 421 to 425 in 4ccd1a2

fn send_data<D: Into<WriteBuf<B>>>(&mut self, data: D) -> Result<(), Self::Error> {
if self.writing.is_some() {
return Err(Self::Error::NotReady);
}
self.writing = Some(data.into());

h3/h3-quinn/src/lib.rs

Lines 382 to 385 in 4ccd1a2

fn poll_ready(&mut self, cx: &mut task::Context<'_>) -> Poll<Result<(), Self::Error>> {
if let Some(ref mut data) = self.writing {
while data.has_remaining() {
match ready!(Pin::new(&mut self.stream).poll_write(cx, data.chunk())) {

Copy link
Contributor

@stammw stammw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@stammw
Copy link
Contributor

stammw commented Feb 18, 2022

Oops, approved this a bit too quickly... I looks like this swap makes all the transfers fail. I vaguely remember having this problem. @camshaft suggestion might help.

Also, do you think the transfer should be initiated at the first send_data() call ?

@camshaft
Copy link
Contributor

Like mentioned in the issue, it would be most obvious to behave like the Sink trait:

poll_ready

This method must be called and return Poll::Ready(Ok(())) prior to each call to start_send.

This method returns Poll::Ready once the underlying sink is ready to receive data.

start_send

Begin the process of sending a value to the sink. Each call to this function must be preceded by a successful call to poll_ready which returned Poll::Ready(Ok(())).

As the name suggests, this method only begins the process of sending the item. If the sink employs buffering, the item isn’t fully processed until the buffer is fully flushed. Since sinks are designed to work with asynchronous I/O, the process of actually writing out the data to an underlying object takes place asynchronously. You must use poll_flush or poll_close in order to guarantee completion of a send.

@stammw
Copy link
Contributor

stammw commented Feb 18, 2022

Right, maybe I should have read the issue beforehand 😅 . Thanks!

While we're at it, we might want to ditch AsyncWrite by doing what Ralith suggested in this issue:

quinn could also be modified to expose raw poll_* style methods alongside important futures, if a better approach is required in the mean time. That's assuming it's not practical to modify h3 to push the pinning/lifetime issues downstream.

Or maybe implement Sinkdirectly in quinn and use it.

@camshaft
Copy link
Contributor

Yeah AsyncWrite isn't the most efficient way of transferring data, especially when the data is already a Bytes. With s2n-quic we've exported poll functions for all of the stream operations. It leads to a really clean implementation of the h3 traits.

@stammw
Copy link
Contributor

stammw commented Feb 18, 2022

Nice ! I didn't know about this project.

@camshaft
Copy link
Contributor

we just launched yesterday 😄

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.

write method calls send_data before poll_ready
3 participants