Add Stream ReadAtLeast and ReadExactly

[eerhardt]

Adds methods to Stream to read at least a minimum amount of bytes, or a full buffer, of data from the stream.
ReadAtLeast allows for the caller to specify whether an exception should be thrown or not on the end of the stream.

Make use of the new methods where appropriate in net7.0.

Fix #16598

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Adds methods to Stream to read at least a minimum amount of bytes, or a full buffer, of data from the stream.
ReadAtLeast allows for the caller to specify whether an exception should be thrown or not on the end of the stream.

Make use of the new methods where appropriate in net7.0.

Fix #16598

Author:	eerhardt
Assignees:	-
Labels:	area-System.IO, new-api-needs-documentation
Milestone:	-

[stephentoub]

There is a risk here. Previously if the async read pends, it would force the ReadMessageAsync state machine to be allocated. Now if the async read pends, it will force the ReadMessageAsync state machine to be allocated and the ReadAtLeastAsync state machine to be allocated.

We should consider using the pooled async state machine builders on the ReadAtLeastAsync async methods.

[eerhardt]

Is this as simple as adding

[AsyncMethodBuilder(typeof(PoolingAsyncValueTaskMethodBuilder))]

to the Stream.ReadAtLeastAsyncCore method? Or is it more involved than that?

[stephentoub]

Implementation-wise, that's it, easy peasy. The important thing is validating performance. The concern with such pooling is that it can have a negative impact on the larger system, especially if it ends up creating more gen2 to gen0 references (by referencing gen0 objects from pooled objects that are likely to eventually be gen2). But I worry that without doing this, the methods will end up not being used because folks will see the allocation and choose to avoid the helpers as a result.

[eerhardt]

Let's chat tomorrow about how I can validate the performance here. I'd like to get your thoughts on what to try / look for.

[carlossanlop]

Thanks for this change! Left some comments and questions.

[eerhardt]

Thanks for the review so far. I now believe this is fully ready for review. PTAL.

[stephentoub]

My personal preference would be to throw for empty buffers

More specifically, I think we should throw for minimumBytes <= 0.

There are two main situations where someone might be passing 0 to a read method:

• Want the "wait for data to be available" behavior
• Doing math to compute how much to read and not special-casing 0

Read is available for the former. For the latter, a direct call to ReadAtLeast with a specified lower bound of 0 doesn't seem likely, given that this is expected to be used in scenarios like protocol parsing where you're expecting some number of bytes necessary to make forward progress, and for ReadExactly, it'll typically be used in situations where you're trying to read everything in the payload into some buffer you have... doing math is much more common with Read, where you might need to loop, and we wouldn't expect much looping around ReadExactly.

[adamsitnik]

Based on responses provided by @stephentoub it LGTM. Thanks!

[carlossanlop]

Left some documentation suggestions.

[GSPP]

How would ReadExactly be used to read a length-prefixed binary value from some protocol? A zero-length binary data is no special case and should just work.

My understanding is that this call would currently throw. I'm not sure why there is a need to create a special case where, naturally, no special case is necessary.

@stephentoub mentioned that this can happen if "buffer math" results in a zero-length read. I don't see this as a bad thing. It can be an outcome of natural code patterns.

[stephentoub]

I'm not sure why there is a need to create a special case

Because otherwise it could hang until there's data available. If you want that behavior, Read already exists and provides everything needed. In your example scenario, it's almost certainly not the behavior you want.

An alternative is special-casing the new methods to handle 0 by always returning immediately rather than throwing.

[GSPP]

Oh, now I see. If we have a zero-length read, this would cause many streams to wait until more data is incoming! That's a bug, and I'm afraid I will now have to check some of my code for this particular mistake 😁. Thanks for explaining.

But that's actually an argument to add that special case check. That seems to fit the use-case perfectly.

Without that special case, many callers will need to special case this themselves. I have done quite a bit of protocol work, and zero-length data is not uncommon.

[stephentoub]

An alternative is special-casing the new methods to handle 0 by always returning immediately

@eerhardt, what would you think of this?

[eerhardt]

I don't really like having different behavior between Read(buffer, 0, 0) and ReadAtLeast(buffer, 0). My first proposal was to make these behave roughly equivalent - block until data is available. Passing in an empty buffer into both ReadAtLeast and ReadExactly became a problem though, since 0 was returned from the Read operation, and that looked like an end-of-stream.

At a high-level, I can think of the following possibilities:

1. Check up front and throw an exception. (current PR behavior)
2. Return immediately, don't do a "read", even though a "read" method was called.
3. Always call Read, with 2 options on when an empty buffer is passed:
   a. Throw an exception
   b. Special case an empty buffer, and allow for 0 to be returned from Read. Return successfully for ReadExactly and 0 for ReadAtLeast.

My current thinking is that 3(b) seems to be the most consistent with Stream.Read, and would be reasonable behavior.

[stephentoub]

My current thinking is that 3(b) seems to be the most consistent with Stream.Read, and would be reasonable behavior.

I don't think that's desirable. It means a scenario like:

int numBytesInPayload = ReadPayloadSize(stream);
byte[] payload = new byte[numBytesInPayload];
stream.ReadExactly(payload);

could block indefinitely even when the payload has been fully and successfully read.

I do think 0 is special for ReadExactly and ReadAtLeast and needs to be treated differently from what Read does. We could choose to throw (1) or return immediately (2), but calling Read with 0 (3) is problematic.

If someone were using their own read loop for this, they would be special-casing 0, e.g.

int numBytesInPayload = ReadPayloadSize(stream);
byte[] payload = new byte[numBytesInPayload];
int totalRead = 0;
while (totalRead < numBytesInPayload)
{
    int n = stream.Read(payload.AsSpan(totalRead));
    if (n == 0) throw ...;
    totalRead += n;
}

such that stream.Read would never be called if numBytesInPayload were 0. We want them to be able to switch to do that instead with ReadExactly, but with any variant of (3), they'd have to special-case 0 themselves:

if (payload != 0)
{
    stream.ReadExactly(payload);
}

Now, maybe we'd want them to do that anyway in this particular example, in order to prefer using Array.Empty<byte>() in that case. But that's an optimization, and this API is meant to be the easy button.

Is there a use case where someone would be using ReadExactly or ReadAtLeast, passing in 0, and wanting the "wait until data is available" behavior that stream.Read might provide?

[eerhardt]

Are there two different scenarios here?

1. ReadExactly and ReadAtLeast with an empty buffer (which implies minimumBytes must be 0).
2. ReadAtLeast with a non-empty buffer, but passing in minimumBytes = 0.

In case the non-empty buffer case, would you expect no Read to happen?

[stephentoub]

Are there two different scenarios here?

That's part of my question... what is the scenario for calling ReadAtLeast with a non-zero-length buffer and minimumBytes == 0?
Why wouldn't you just use Read? I'm not clear on in what situation you'd say "please read data but if you feel like not giving any back that's cool".

[eerhardt]

Maybe the behavior split makes sense between the new methods then?

• ReadExactly with an empty buffer (or count == 0 for the array overload) - no-ops
• ReadAtLeast with minimumBytes == 0 - throws

I just wouldn't expect ReadAtLeast to no-op. It feels closer to Read, and isn't the easy button. If we don't have a scenario for ReadAtLeast(buffer, 0), maybe it makes the most sense to throw.

[stephentoub]

Actually, I just answered my own question ("I'm not clear on in what situation you'd say "please read data but if you feel like not giving any back that's cool"). Consider the same scenario I outlined earlier, but where the protocol parser is maintaining its own buffer. It wants to read the payload but also any additional data that might be available, in order to avoid unnecessary additional calls for when it wants more data later. That's what ReadAtLeast is for, and you wouldn't want it to throw for 0 (0 is valid) nor block waiting for more (because the 0 was satisfied). Allowing 0 to just nop means ReadAtLeast works for non-0-length in this manner, giving back as much as it can, while still enabling 0 to work without throwing or blocking.

Stream.Read special-cases 0 (on some streams). ReadExactly and ReadAtLeast serve different purposes than does Read, and since they're implemented in terms of it (effectively as an implementation detail), they then also need to special-case 0. I think it makes sense for them to special-case it to mean "I've read what you asked and now I'm done". It would be nice if ReadAtLeast could give more back if more was available, but to do so it would need to call Read with 0, which could block.

[stephentoub]

I just wouldn't expect ReadAtLeast to no-op

Why? It's in the name "at least"... when it has at least 0, it's done.

[eerhardt]

OK, I can make this change.

It would be nice if ReadAtLeast could give more back if more was available, but to do so it would need to call Read with 0, which could block.

This might be a case for making ReadAtLeast virtual - so a derived stream could give back data if it was already available, without blocking. But I don't see it as a "need", and we can add it in the future if necessary.