QuicStream: Consider requiring Flush to guarantee data is sent

[geoffkizer]

Currently, when you do a Write on QuicStream, the data you write is guaranteed to be sent promptly, without requiring a call to Flush. This is in line with how NetworkStream and SslStream behave -- though it's different than how HTTP request/response streams work in HttpClient, which do require Flush.

I think we should consider changing this and require a call to Flush to guarantee data is sent. If you don't call Flush, we may send the data -- e.g. if we are sending something else anyway and can use extra space in the packet for this data. But we won't guarantee that the data is sent unless you call Flush. (Also, ShutdownWrite would force a flush as well.)

This allows for several optimizations and/or simplifications:

• Combining outbound data from more than one stream into a single packet. This is important for HTTP3, where dynamic header compression happens over a control stream, so you want to be able to send on both the header stream and a request stream at the same time. Currently we have no way to achieve this, and it's not obvious how to extend the API to allow this.

But if we require Flush, then you can simply do:

    await stream1.WriteAsync(data1);
    await stream2.WriteAsync(data2);
    await stream1.FlushAsync();
    await stream2.FlushAsync();

If nothing else is happening on the connection, and the two writes fit into a single packet, then on the flush of the first stream, we will be able to write out the first stream's data, notice there is extra space in the packet, and write out the second stream's data as well. (If other packets are being sent concurrently, then the packets may end up getting coalesced in different ways, but since we have the flexibility to choose whether or not to send unflushed data, we should always be able to construct packets in an optimal way.)

• Force a stream to be created on the wire. Open*Stream as defined currently does not actually create the stream on the wire until you do a read or write on the stream. Effectively, the "open" operation is not immediately flushed like other stream operations. But as it stands, there is no way to "flush" the open (i.e. force the stream to be created on the wire) without either reading or writing. We actually hit this issue in stream tests, because you can't create a "connected pair" of QuicStreams without doing either a read or a write on the outbound stream.

If we require Flush, then you can simply do a Flush immediately after Open to force stream creation.

• Gathered writes. We currently have several overloads for doing gathered writes on Stream. These are unnecessary if we don't immediately send the write without a flush; you can just call Write for each individual buffer instead, and Flush after all have been written. This simplifies the user model as well as the implementation.

• Overloads of write with a bool shutdownWrites parameter. These exist so that we coalesce the last chunk of data and the end-of-stream indicator into a single packet. But if we require Flush, then you can simply call Write and then ShutdownWrite and the optimal thing will happen.

cc @stephentoub @scalablecory @nibanks @halter73 @Tratcher @jkotalik @davidfowl

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Description:	Currently, when you do a Write on QuicStream, the data you write is guaranteed to be sent promptly, without requiring a call to Flush. This is in line with how NetworkStream and SslStream behave -- though it's different than how HTTP request/response streams work in HttpClient, which do require Flush. I think we should consider changing this and require a call to Flush to guarantee data is sent. If you don't call Flush, we may send the data -- e.g. if we are sending something else anyway and can use extra space in the packet for this data. But we won't guarantee that the data is sent unless you call Flush. (Also, ShutdownWrite would force a flush as well.) This allows for several optimizations and/or simplifications: Combining outbound data from more than one stream into a single packet. This is important for HTTP3, where dynamic header compression happens over a control stream, so you want to be able to send on both the header stream and a request stream at the same time. Currently we have no way to achieve this, and it's not obvious how to extend the API to allow this. But if we require Flush, then you can simply do: await stream1.WriteAsync(data1); await stream2.WriteAsync(data2); await stream1.FlushAsync(); await stream2.FlushAsync(); If nothing else is happening on the connection, and the two writes fit into a single packet, then on the flush of the first stream, we will be able to write out the first stream's data, notice there is extra space in the packet, and write out the second stream's data as well. (If other packets are being sent concurrently, then the packets may end up getting coalesced in different ways, but since we have the flexibility to choose whether or not to send unflushed data, we should always be able to construct packets in an optimal way.) Force a stream to be created on the wire. Open*Stream as defined currently does not actually create the stream on the wire until you do a read or write on the stream. Effectively, the "open" operation is not immediately flushed like other stream operations. But as it stands, there is no way to "flush" the open (i.e. force the stream to be created on the wire) without either reading or writing. We actually hit this issue in stream tests, because you can't create a "connected pair" of QuicStreams without doing either a read or a write on the outbound stream. If we require Flush, then you can simply do a Flush immediately after Open to force stream creation. Gathered writes. We currently have several overloads for doing gathered writes on Stream. These are unnecessary if we don't immediately send the write without a flush; you can just call Write for each individual buffer instead, and Flush after all have been written. This simplifies the user model as well as the implementation. Overloads of write with a bool shutdownWrites parameter. These exist so that we coalesce the last chunk of data and the end-of-stream indicator into a single packet. But if we require Flush, then you can simply call Write and then ShutdownWrite and the optimal thing will happen. cc @stephentoub @scalablecory @nibanks @halter73 @Tratcher @jkotalik @davidfowl
Author:	geoffkizer
Assignees:	-
Labels:	area-System.Net.Quic
Milestone:	[object Object]

[nibanks]

You might find this recently created MsQuic PR interesting: microsoft/msquic#995. We still need to do some prototyping at the HTTP (in Windows) layer to see how it might improve things (or not).

[scalablecory]

In an ideal world the rule would be "don't send out anything until you can send a full packet, or until Connection.Flush() is called". Essentially, TCP_CORK behavior.

QPACK requires some careful thought still; we can't treat it like HPACK. I'm not sure we want to tightly couple request streams to the QPACK stream (this would remove a major selling point of HTTP/3), but instead will want the QPACK stream to intentionally lag behind.

So, a multi-stream flush would not be useful for QPACK outside of the generic benefit that it would give to all streams. Note, a multi-stream flush isn't even possible with MsQuic.

Speaking of MsQuic, diving into implementation details now...

MsQuic has the very different behavior of "always flush immediately and packets will eventually be optimally sized (and streams combined) if you're bandwidth-limited."

This appears to me to be a bit network-unfriendly from a small packet size perspective, but we should talk to MsQuic team to understand if there is a benefit to their behavior over a flush-required model.

While we can't solve the multi-stream write combining or optimal packet size problem with MsQuic, buffering would at least help the latter if a user's HttpContent makes small writes.

If we decide to implement our own buffering, we should consider using Pipelines with MsQuic's user-controlled buffering mode. This will not solve the stream combining or optimal packet size desire, but it will at least avoid double-buffering.

It would also make Kestrel more performant if we exposed such a Pipe API, as currently their Pipe-native design is already doing this double-buffering.

[nibanks]

There's a tradeoff here between latency/responsiveness and minimizing network packet counts. MsQuic generally makes the decision in favor of improved latency. The problem is a lack of information. The transport only knows that the app queued something to send, it has no idea if anything else may be sent any time soon. microsoft/msquic#995 modifies that a little by giving the app the ability say "there's more incoming very soon". Beyond this, I would push back very strongly in over optimizing things just to reduce packet counts, unless actual performance measurements indicate a real issue. The issue here is not CPU usage. As the CPU becomes more utilized, MsQuic will naturally end up batching more because of how cross-connection work scheduling is designed.

[scalablecory]

It looks like you already solved the issue, just with a slightly different API. Sadly I didn't see your response before submitting mine.

I'm curious how that PR behaves -- does it avoid going to wire until a non-flagged write comes along? Is there a timer to force flushing after some small time? This indeed looks like something we could use. Would a zero-byte write work as a way to implement a Flush() in this case?

[nibanks]

It's all code, so anything is possible! But, currently the PR is written up so that if the delay flag is set, we just queue all the data and state, but don't queue the final connection-wide "flush". We don't have any logic (yet) to explicitly override the user's delay. If something else on the connection results in a flush (i.e. some control data gets queued, or a retransmit) then the data will end up getting sent out then.

We have discussed the possibility of adding two things to explicitly override the flag:

• A cap or threshold of queued data, that when reached we flush anyways.
• A timer, similar to the ACK delay timer (25 ms) that flushes eventually. This one I'm more hesitant to do.

As far as allowing a zero-byte write force a flush, I'd have to think about that. Currently, the only case we allow a zero-byte write is if the FIN flag is set, but it does mean most of the machinery is already in place for zero-byte writes.

[scalablecory]

if the delay flag is set, we just queue all the data and state, but don't queue the final connection-wide "flush" ... If something else on the connection results in a flush (i.e. some control data gets queued, or a retransmit) then the data will end up getting sent out then.

Presumably, in addition to the internal flushes (control/etc.), one stream not setting the bit during a send would result in the entire connection (all other unflushed streams) flushing?

If so, this might end up being an anti-optimization. Consider a high concurrency scenario, where flushing is happening frequently. If a developer is depending on being able to make small writes and know they will be coalesced, and instead we end up generating a bunch of super small STREAM frames, it is probably not what the user intended. In this case, buffering outside of MsQuic probably makes more sense.

[nibanks]

Yes, another stream doing a write without delay will flush everything, by design. Specifically, take HTTP/3 & QPACK design. If you need to send out both a request/response stream data and corresponding QPACK info, this would allow you to queue up both streams worth of data (two separate calls) with the final send causing a flush of everything.

The goal at the transport layer is not to support coalescing lots of small writes. Each write has a non-zero overhead in allocating tracking data structures and handling completion in response to acknowledgements. IMO, this scenario should be handled with buffering at the app layer (if desired) and not the transport.

[geoffkizer]

It would be nice to close on whether QuicStream requires Flush, or is auto-flush. I'm hoping we have enough information and experience to do this now. I'm still leaning toward saying that QuicStream requires Flush. @scalablecory what's your take here?

[nibanks]

We added support for the QUIC_SEND_FLAG_DELAY_SEND flag to allow you to indicate you don't need the data to be sent immediately. You can then later force a flush with a 0 length send with no flag. To be clear though, the delay send flag isn't a guarantee we won't send it sooner than your flush if we opportunistically have something else to send.

[scalablecory]

It would be nice to close on whether QuicStream requires Flush, or is auto-flush. I'm hoping we have enough information and experience to do this now. I'm still leaning toward saying that QuicStream requires Flush. @scalablecory what's your take here?

I agree, we should require Flush.

[nibanks]

I agree, we should require Flush.

So does this mean, every send on a stream will require two API calls? One to send the data and one to flush it? Will there be any way to optimize this to a single call?

[scalablecory]

I agree, we should require Flush.

So does this mean, every send on a stream will require two API calls? One to send the data and one to flush it? Will there be any way to optimize this to a single call?

I'm looking at APIs to make this more efficient here: #43290 (comment)

Is MsQuic particularly impacted by the extra API call?

[geoffkizer]

@nibanks I'm curious about this scenario, please pardon my ignorance of the latest API:

Force a stream to be created on the wire.

Can I create a stream and then force just the stream creation frame to be sent on the wire?

I think I can force the stream to be created on the wire as part of creating the stream, but that's not really what I'm asking here... what I want to figure out is, can we support an API like this:

QuicStream s = connection.OpenStream();
await s.FlushAsync(); // when this completes, the stream has been created on the wire

[nibanks]

Is MsQuic particularly impacted by the extra API call?

Well, it's all relative. If the app is making small sends the cost is definitely going to be relatively higher. Each send call at least results in;

On the caller's thread:

• Allocation of an internal send object
• Synchronization with the QUIC worker thread
• Queue the work to be processed (added to end of current work list)
• Queue the connection to be processed by the worker thread

On the QUIC worker thread:, eventually the worker will get around to processing the connection, and then the connection will get around to processing the send work. Bottom line, nothing is free. Send is one of the cheapest, as far as the API caller, but your still splitting up what could/should be a single call (as far as MsQuic API is concerned).

Can I create a stream and then force just the stream creation frame to be sent on the wire?

MsQuic does allow for you to create a stream and then decide to flush just that creation to be sent on the wire. It's a flag of StreamOpen, so you have to know up front.

[scalablecory]

Triage:

We feel like Flush should be required, to allow optimization re: merging frames into single packets.

There are some questions about how MsQuic functions today. We should chat with @nibanks with our final PR on how we expect to call MsQuic.

There is a perf impact due to queuing work items to MsQuic's thread pool. We should look at getting a new MsQuic API added (not just for this, but to make MsQuic efficient with .NET's Task-based async), and at the very least if there is a large impact we do have the option of disabling MsQuic's send buffer and doing that ourselves.

[geoffkizer]

Note discussion here:

microsoft/msquic#1602 (comment)

Takeaways:

As long as we continue to use msquic send buffering, we probably can't use QUIC_SEND_FLAG_DELAY_SEND.

Longer-term, we should look at doing our own send buffering. Potential wins here:
(1) Support sync send completion
(2) Optimizing packet sends using QUIC_SEND_FLAG_DELAY_SEND

So we still want to require Flush on QuicStream; it's just going to take more work to get value from this.

[karelz]

Triage: We want to require Flush. It will affect API usage patterns, so 7.0.
We need to test for it.

[ManickaP]

Closing, covered by #69675