Support zero-byte reads on HTTP response streams

[MihaZupan]

Fixes #61475

This PR does the plumbing to get zero-byte reads working but doesn't change the buffer management of underlying connections.
On HTTP/1.1: The 4k receive buffer remains
On HTTP/2: We don't issue zero-byte reads to the underlying connection at all, but that is unrelated to whether we support zero-byte reads on the response streams

The minimal change to get WebSockets working on HTTP/1.1 is just 25029a7

[ghost]

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

Issue Details

---

Fixes #61475

This PR does the plumbing to get zero-byte reads working but doesn't change the buffer management of underlying connections.
On HTTP/1.1: The 4k receive buffer remains
On HTTP/2: We don't issue zero-byte reads to the underlying connection at all, but that is unrelated to whether we support zero-byte reads on the response streams

The minimal change to get WebSockets working on HTTP/1.1 is just 25029a7

Author:	MihaZupan
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	.NET 7.0

[CarnaViire]

HTTP/3 part LGTM. I notice MockQuicStreamConformanceTests.ZeroByteRead_BlocksUntilDataAvailableOrNops test started failing -- is that after changes to make your added HTTP/3 test work? We don't really support Mock Quic now and will most likely remove it in the near future, so don't spend too much time investigating that...

[geoffkizer]

Why is this necessary? Won't the Fill call immediately below take care of this?

[MihaZupan]

Fill will pass the entire connection buffer to the transport stream.
Checking for 0 here here means that we will attempt a zero byte read first if the user requested it and no data was available in the buffer.

This allows us to take advantage of the zero byte read on SslStream.

[geoffkizer]

Yeah, that makes sense. I assume we do this for the non-chunked cases too?

We should add a comment explaining this.

I do wonder a bit about cases where we have received part of a chunk already (or even just part of a chunk header). In these cases it seems reasonable to assume that the rest of the chunk will arrive promptly, and thus a zero byte read is probably counterproductive.

In other words, zero-byte read is mostly useful when you have some sort of streaming scenario. In that scenario the server is almost certainly not going to actually pause sending in the middle of a chunk; it's going to do it between chunks. So it's really the case where we have fully consumed the previous chunk and haven't received any of the subsequent chunk where zero-byte read is useful.

In SslStream we only do the zero-byte read on the first read of a new TLS frame (I think).

All that said, perhaps this doesn't really matter in practice. Thoughts?

[MihaZupan]

I assume we do this for the non-chunked cases too?

Yes, we now do this for all HTTP 1 response streams.
On HTTP/2 and HTTP/3, the response streams also block on zero-byte reads, but those reads are separate from the reads to the underlying transport.

I agree that the most obvious benefits are to idle-ish connections (streaming), but even regular content transfers will often be slow enough for the receive loop to outperform and be forced to wait.

AspNetCore separates this into two parts:

• The streams (pipes) exposed to the user will block on zero-byte reads
• Whether the underlying transport uses zero-byte reads is controlled by the WaitForDataBeforeAllocatingBuffer flag which always defaults to true. Their SocketTransport will issue a zero-byte read before every actual read.
  • We should consider proactively doing the same for HTTP/2 and 3 (it would help with Improvements to memory usage of HttpClient connections and requests #61223).

The overhead of honoring the user's request to issue a zero-byte read should be low, especially if we already checked that no data has been buffered.

[geoffkizer]

even regular content transfers will often be slow enough for the receive loop to outperform and be forced to wait.

Yes, that's a good point, and I agree it's reasonable to do a zero-byte read in these cases.

To step back for a second, here's how I think about this generally:

The response streams should always support zero-byte reads -- meaning, a zero-byte read will block until data is actually available and the subsequent read is guaranteed to not block. This enables a user to reduce their overall buffer memory usage: they only need to allocate a buffer when data is actually available.

That said, supporting zero-byte read does not require that the response stream does a zero-byte read against the underlying stream. This is most obvious in the HTTP2 case because of multiplexing. But it also applies to a case like the chunked response stream: we (usually) have an internal read buffer, and so we could just do regular (non-zero-byte) reads into that read buffer and then use that to provide the zero-byte-read semantics to the user.

What that approach doesn't address is, being smart about our own buffer management. Currently, we never release the internal read buffer between read calls, even when it's empty. And currently we never do a zero-byte read against the underlying stream. But if the user issues a zero-byte-read against us, then it seems reasonable to assume that they want to trade off some small CPU overhead for reduced memory usage -- that's the whole point of doing a zero-byte read. And so when that happens, we should avoid holding/allocating a buffer during the user's zero-byte read when possible, and instead do a zero-byte read against the underlying stream, and only allocate a buffer when that zero-byte read returns. (That's the change we made to SslStream in 6.0.)

What's weird about this PR is that we haven't changed the internal buffer management at all -- we still never release the internal read buffer. So even though we do a zero-byte read against the underlying stream, we aren't actually getting any advantages from this in terms of reduced memory usage.

Put differently: there's no point in doing a zero-byte read unless we are going to defer allocation of the read buffer until that zero-byte read completes.

Thoughts?

[geoffkizer]

Re your comments on buffer improvements to Raw/Chunked/HTTP2/etc, that all seems good to me.

[MihaZupan]

It's so that the user's zero-byte read translates into a zero-byte read to SslStream (deferring its 32k buffer allocation) while avoiding changes to the lifetimes of HttpConnection owned buffers. I.e. it's a minimal change to get most of the benefit, matching other HTTP/1.x streams.
It's akin to this change (HttpConnection line 1732) - not needed for the zero-byte read to block, but allows us to make use of SslStream's zero-byte read improvements.

I agree the comment is misleading now without further changes, I'll update that.

[geoffkizer]

Okay, makes sense. Let's change the comment and get this in.

[geoffkizer]

Just to ensure we are on the same page:

It seems like there are really three pieces of work here to fully support zero-byte reads:

(1) Support user zero-byte read so they can be smart about managing their buffers
(2) Use the user's zero-byte read to improve our own internal buffer management
(3) Perform zero-byte read against the underlying stream so that the underlying stream can be smart about their buffer management

This PR contains (1) and (3) for all response streams (except (3) for HTTP2, due to multiplexing).

We will look at (2) in a future PR. This may include changes to buffer management beyond just zero-byte-read cases.

We will also look at (3) for HTTP2 in a future PR.

[MihaZupan]

Yes, that matches my understanding exactly

[geoffkizer]

Generally LGTM, a couple comments above.

[MihaZupan]

@CarnaViire the test hang was my doing - I didn't update the conformance test to indicate the stream now supports zero-byte reads.