write method calls send_data before poll_ready

[WesleyRosenblum]

On stream.rs#L28-L29, stream.send_data is called prior to stream.poll_ready.

stream.poll_ready should be called first, similar to as described in https://docs.rs/futures/latest/futures/sink/trait.Sink.html#tymethod.poll_ready.

Thanks for considering this issue, and let me know if you have any questions!

[seanmonstar]

Huh, good point. I don't recall why the order was switched there... Granted, technically we aren't using Sink specifically, but the pattern is definitely similar.

[camshaft]

Another option would be to rename the methods to something like set_data and poll_send_data, which it seems like is how it's being used currently.

But if we're considering an API change, I'm not sure why we couldn't just change it to:

fn poll_send_data(&mut self, buf: &mut dyn Buf) -> Poll<Result<()>>;

It would remove the need for the stream implementation to convert/store the buffer and make things a bit simpler, IMO.

[seanmonstar]

That change would require that the bytes always be copied out, no? I guess unless it was already a Bytes. Whereas currently, by passing ownership of the impl Buf, a QUIC implementation could copy, but it could also choose to store it, kinda like h2 does.

[camshaft]

Not necessarily. If the caller used copy_to_bytes with chunk lengths the Buf impl would specialize on it:

let len = buf.chunk().len();
let bytes = buf.copy_to_bytes(len);

(see, for example, impl Buf for Bytes)

[seanmonstar]

Yea, that's what I meant by if it was already a Bytes. If it was a custom impl Buf, it would end up being copied into a Bytes... I think there's merit in discussing if the API around sending data is the right one. But for this specific issue, it also looks like that's a simple bug: the calls should be reversed.