Allow un-examining in PipeReader.AdvanceTo(...)

[BrennanConroy]

Fixes #107213

Changes PipeReader.AdvanceTo(...) to allow un-examining as long as the examine index is >= consumed. This allows code that might look at parts of the Pipe, and then pass the Pipe on, to work better.

A specific example is when examining some of the Pipe (pipeReader.AdvanceTo(buffer.Start, buffer.End)) and then creating a Stream via PipeReader.AsStream() and doing a zero-byte read on the Stream.

Code that would have thrown in this case will now work. The few examples we found of the original "examined position cannot be less than the previously examined position" exception have all been in cases where this fix would have made changing our code to avoid the exception unnecessary.

Analyzing other PipeReader implementations

ASP.NET Core (Kestrel)

Kestrel has 1 PipeReader impementation: HttpRequestPipeReader

This is the type application developers can interact with. Internally it calls into different implementations depending on what version of Http the request is using. Most implementations look fine, the main interesting effect this change will have is on the tracking of how many bytes have been looked at for the case of flow control
https://github.com/dotnet/aspnetcore/blob/226b1c67495ad29a5d08be5a72d4f4e5a465220d/src/Servers/Kestrel/Core/src/Internal/Http/MessageBody.cs#L304
This can now return a negative value, which is then passed on to Http2 flow control. The flow control code looks like it can handle negative values and even calls it out as a scenario
https://github.com/dotnet/aspnetcore/blob/226b1c67495ad29a5d08be5a72d4f4e5a465220d/src/Servers/Kestrel/Core/src/Internal/Http2/FlowControl/FlowControl.cs#L36-L39

Nerdbank.Streams

The Nerdbank.Streams repo has a couple PipeReader implementations
https://github.com/dotnet/Nerdbank.Streams/blob/886a3b356f355ab84c915e1325bba20129c6f599/src/Nerdbank.Streams/StreamPipeReader.cs#L78
and
https://github.com/dotnet/Nerdbank.Streams/blob/886a3b356f355ab84c915e1325bba20129c6f599/src/Nerdbank.Streams/NestedPipeReader.cs#L36
both of which look like they'll continue working as expected with this change.

Pipelines.Sockets.Unofficial

MemoryMappedPipeReader looks like it will continue working as expected with this change.

---

Original change

Core change

Changes PipeReader.AdvanceTo(SequencePosition consumed) to choose between the internal examined index and consumed depending on which one is further advanced. This allows code that might look at parts of the Pipe, and then pass the Pipe on, to work better.

A specific example is when examining some of the Pipe (pipeReader.AdvanceTo(buffer.Start, buffer.End)) and then creating a Stream via PipeReader.AsStream() and doing a zero-byte read on the Stream.

This would technically be a behavior breaking change, but I believe it's mostly additive, so shouldn't break anyone unless they were relying on seeing an InvalidOperationException in this specific case.

Analyzing other PipeReader implementations

ASP.NET Core (Kestrel)

In order to fix the original issue we have to at a minimum change DuplexPipeStream to call AdvanceTo(consumed), right now it explicitly calls AdvanceTo(consumed, consumed).

There is also HttpRequestPipeReader which calls MessageBody.AdvanceTo(consumed) and that ends up calling MessageBody.AdvanceTo(consumed, consumed) which has multiple implementations, many of which call AdvanceTo(consumed, examined) on an underlying PipeReader. The HttpRequestPipeReader is used in user app code, so isn't as urgent to update if we go this route.

Nerdbank.Streams

The Nerdbank.Streams repo has a couple PipeReader implementations that would diverge in behavior
https://github.com/dotnet/Nerdbank.Streams/blob/886a3b356f355ab84c915e1325bba20129c6f599/src/Nerdbank.Streams/StreamPipeReader.cs#L78
and
https://github.com/dotnet/Nerdbank.Streams/blob/886a3b356f355ab84c915e1325bba20129c6f599/src/Nerdbank.Streams/NestedPipeReader.cs#L36
The NestedPipeReader one is the more immediately interesting one as it wraps a PipeReader and would end up calling AdvanceTo(consumed, consumed) which ends up missing this change.

Pipelines.Sockets.Unofficial

MemoryMappedPipeReader behavior would diverge.

Alternative design

We could attempt to do a fix that's targeted specifically to PipeReaderStream. One silly idea is to always set examined to 1 less than the end of the read buffer (unless consuming the entire buffer) which would make it so we never run into using an examined index less than the internal examined index, and would let any future read calls to complete immediately since there is at least 1 "new" byte to read.

[BrennanConroy]

I think it would technically be ok to access _lastExaminedIndex outside of the lock as it should only be modified by read calls or the Pipe completing, both of which should be synchronized (in user code) with the advance from the current read.

[halter73]

I agree that this lock isn't necessary considering it's only comparing values supplied by the read loop.

I think we need to be able to advance examinedSegment and examinedIndex to subsequent BufferSegments if the goal is to not throw for unexamining. If we want to go that route, I think it makes sense to store the entire _lastExaminedSequencePosition rather than just the _lastExaminedIndex and use that if it's greater than consumed which could also probably be done outside a lock.

[BrennanConroy]

@halter73 @davidfowl thoughts on this approach?

[halter73]

I agree that this lock isn't necessary considering it's only comparing values supplied by the read loop.

I think we need to be able to advance examinedSegment and examinedIndex to subsequent BufferSegments if the goal is to not throw for unexamining. If we want to go that route, I think it makes sense to store the entire _lastExaminedSequencePosition rather than just the _lastExaminedIndex and use that if it's greater than consumed which could also probably be done outside a lock.

[halter73]

LGTM!

[BrennanConroy]

@AArnott and @mgravell any thoughts here before we merge?

[AArnott]

Thanks for the mention. No concerns here.

[mgravell]

LGTM 👍