quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.85k stars 394 forks source link

Allow a SendStream to buffer until more than a full packet is available #1492

Open flub opened 1 year ago

flub commented 1 year ago

Problem

Finishing Streams

It happens often enough that a SendStream::finish() call happens after the last data of the stream is already sent in a STREAM frame, this then results in yet another, empty, STREAM frame with the FIN bit set. This in turn can cross wires with the application logic which may already know the data is complete and may have dropped the stream or called RecvStream::stop() explicitly as it no longer needs this RecvStream. When this occurs the SendStream::finish() call will fail if the STOP_SENDING frame reaches it in time. An example of this is also described in #1487.

Small writes

It might be that an application does several small writes and sending these out as individual frames (modulo any packet pacing etc) is sub-optimal. TCP addresses this with the TCP_CORK socket option which gives application control on when to pause and restart sending things on the wire.

Proposal

Both these could be addressed with a flag on the SendStream to only send data when there is more than one packet's worth of data buffered. Once set, data sent would only be transmitted when:

Enabling this flag gives sufficient control to the application to avoid small writes as well as place the FIN bit on the last STREAM packet which will have data. The other write APIs would not need to be changed as this is stream-level behaviour.

djc commented 1 year ago

Thanks for the suggestion! The proposed API seems a bit tricky because this is all happening in the context of a larger connection. Presumably if some other stream needs to send a packet and there's some space left over in that packet, you wouldn't mind the stream packing its bits in with that packet?

Alternatively, could we get away with a flag that prevents the stream from triggering any packet sends, leaving it to the application to trigger sends immediately? This grants more control to the application at the cost of making its life a little harder (but might not be a big deal if your application has a clear idea of when it wants to send).

Another issue that I see is that "a packet size" worth of data isn't very deterministic, because it might depend on what else gets send in that packet how much space there is. So presumably you'd definitely like your data to piggyback on packets that are sent anyway, but you want to ensure some minimal outstanding buffer before a packet is triggered.

flub commented 1 year ago

So this idea came from @Ralith on matrix, I tried to put it down in an issue but may have some pieces wrong. Please correct me if so.

It is indeed a good point that packets already send on the connection anyway are used to attach frames to. I don't think the idea of not fully sending the stream has to conflict with this. If you can attach a STREAM frame to the packet while still leaving unsent data in the stream then that can be done. But you wouldn't send a packet just because there is data in this stream, nor would you send the last bit of data of the stream because you might still need to set the FIN bit to that frame.

Ralith commented 1 year ago

@djc raises some good points that weren't immediately obvious to me. In particular, not sending data on a packet that's outgoing anyway and has free space seems pessimal, but sending data while reserving at least one byte, even if there's room for it, to avoid finishing an application-layer message feels cheesy. Maybe we shouldn't try to solve that, since peers can avoid the problem robustly by reading to end anyway? Reducing unintended fragmentation with small application writes is probably independently useful.

Ralith commented 1 year ago

Thought: if your application protocol is not intended to have any data following a certain message, you might want to detect cases where that isn't upheld (i.e. where the peer is badly behaved), which requires reading the stream until finished.

Ralith commented 1 year ago

Another related but slightly different case: when opening a new connection and sending 0-RTT data, Quinn will typically send a padded 1200-byte UDP datagram bearing the Initial packet, followed by some number of datagrams bearing the 0-RTT data. This is inefficient; 0-RTT data could have been replaced the padding.

Deferring opening the connection will probably require a different API than deferring use of a single stream... but maybe stream granularity isn't a complete solution anyway? For example, you might want to buffer up small writes to multiple streams before sending a packet...

flub commented 3 months ago

I think the small writes part of this issue is probably already sufficiently addressed by e.g. tokio's BufWriter to wraps the SendStream.