ReadOnlySequence.Slice which accepts a SequencePosition throws ArgumentOutOfRangeException unexpectedly.

[ahsonkhan]

The behavior of Slice(offset, SequencePosition) should match GetPosition(offset) and Slice(offset).
I should be able to slice a ReadOnlySequence past the current sequence if the next one is empty (for example: [97] -> []).

[Fact]
public static void ReadOnlySequenceTest()
{
    string jsonString = "a";

    ReadOnlyMemory<byte> dataMemory = Encoding.UTF8.GetBytes(jsonString);

    var firstSegment = new BufferSegment<byte>(dataMemory.Slice(0, 1));
    ReadOnlyMemory<byte> secondMem = dataMemory.Slice(0, 0);
    BufferSegment<byte> secondSegment = firstSegment.Append(secondMem);

    // A two segment sequence where the second one is empty: [a]-> []
    var sequence = new ReadOnlySequence<byte>(firstSegment, 0, secondSegment, secondMem.Length);

    SequencePosition expectedPosition = sequence.GetPosition(1);
    Console.WriteLine(expectedPosition.GetInteger());   // 0
    Console.WriteLine(expectedPosition.GetObject());    // System.Text.Json.Tests.BufferSegment`1[System.Byte]
    Assert.True(((ReadOnlySequenceSegment<byte>)expectedPosition.GetObject()).Memory.IsEmpty);

    SequencePosition actualPosition = sequence.Slice(1).Start;
    Console.WriteLine(actualPosition.GetInteger()); // 0
    Console.WriteLine(actualPosition.GetObject());  // System.Text.Json.Tests.BufferSegment`1[System.Byte]
    Assert.True(((ReadOnlySequenceSegment<byte>)actualPosition.GetObject()).Memory.IsEmpty);
    Assert.Equal(expectedPosition, actualPosition); // True

    SequencePosition actualPosition2 = sequence.GetPosition(1, sequence.Start);
    Console.WriteLine(actualPosition2.GetInteger());   // 0
    Console.WriteLine(actualPosition2.GetObject());    // System.Text.Json.Tests.BufferSegment`1[System.Byte]
    Assert.True(((ReadOnlySequenceSegment<byte>)actualPosition2.GetObject()).Memory.IsEmpty);

    // System.ArgumentOutOfRangeException : Specified argument was out of the range of valid values.

    /*
        System.ArgumentOutOfRangeException : Specified argument was out of the range of valid values.
        Parameter name: start
        Stack Trace:
        E:\GitHub\Fork\corefx\src\System.Memory\src\System\ThrowHelper.cs(31,0): at System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument)
        E:\GitHub\Fork\corefx\src\System.Memory\src\System\Buffers\ReadOnlySequence.cs(281,0): at System.Buffers.ReadOnlySequence`1.Slice(Int64 start, SequencePosition end)
        E:\GitHub\Fork\corefx\src\System.Memory\src\System\Buffers\ReadOnlySequence.cs(388,0): at System.Buffers.ReadOnlySequence`1.Slice(Int32 start, SequencePosition end)
        E:\GitHub\Fork\corefx\src\System.Text.Json\tests\Utf8JsonReaderTests.TryGet.cs(357,0): at System.Text.Json.Tests.Utf8JsonReaderTests.ReadOnlySequenceTest()
    */
    sequence = sequence.Slice(1, sequence.Start);
}

We need to use the Slice overload that takes a SequencePosition for performance to avoid having to scan the segments from the beginning every time. Using GetPosition with long is too slow:
https://github.com/dotnet/corefxlab/blob/63ef1a5db4fdbfcf9822b8cd2cbc3921b3d3cb9e/src/System.Text.JsonLab/System/Text/Json/JsonUtf8Reader.cs#L112

cc @pakrym, @davidfowl

[devsko]

@ahsonkhan is this still a problem? Utf8JsonReader seems to work…
https://github.com/dotnet/corefx/blob/master/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs#L118

[ahsonkhan]

There's still an issue with the ROSequence .Slice API. GetPosition works as expected which is why Utf8JsonReader can use it.

[devsko]

I'm asking because I think I just fixed dotnet/corefx#35254 (PR coming soon) and thought it might be related.

[devsko]

But I don't understand this issue. I would say the sample throws the exception as expected.

[ahsonkhan]

But I don't understand this issue. I would say the sample throws the exception as expected.

// A two segment sequence where the second one is empty: [a]-> []
SequencePosition pos = sequence.GetPosition(1, sequence.Start); // success
sequence = sequence.Slice(1, sequence.Start); // throws

Why would you consider the exception when calling slice, expected?

[devsko]

As I understand the API this means a slice from position 1 back to position 0. The second parameter is the end of the resulting slice. The second param of .GetPosition() has a different meaning. What slice would you expect it to return?

[ahsonkhan]

As I understand the API this means a slice from position 1 back to position 0. The second parameter is the end of the resulting slice.

Actually, you are totally right. I confused the parameters of GetPosition and Slice (end vs origin). Passing in sequence.Start as the end position should continue to throw.

public ReadOnlySequence<T> Slice(int start, SequencePosition end) { throw null; }
public ReadOnlySequence<T> Slice(long start, SequencePosition end) { throw null; }
public SequencePosition GetPosition(long offset, SequencePosition origin)

For now, it looks like this issue is non-actionable. Let me page back the scenario where I was seeing an issue and double check the expected behavior. I'll either update the issue or close it based on my findings. Thanks for bringing up the clarification and correcting me :)

[devsko]

Sounds good. Other PR is ready.

[MarcoRossignoli]

For now, it looks like this issue is non-actionable. Let me page back the scenario where I was seeing an issue and double check the expected behavior. I'll either update the issue or close it based on my findings. Thanks for bringing up the clarification and correcting me :)

@ahsonkhan should we close this or move future?

[ahsonkhan]

I haven't gotten a chance to look again. Moving to future for now. Thanks for the ping :)

[sowelipililimute]

i think running into this trying to use system.io.pipelines, i'm not sure what this means i need to do to workaround this

edit:

i should've provided more context:

var result = await reader.ReadAtLeastAsync(2);
var buffer = result.Buffer;
var position = buffer.GetPosition(sizeof(UInt16));
var packetLengthBuffer = buffer.Slice(0, position);
reader.AdvanceTo(packetLengthBuffer.End);
if (result.IsCompleted)
    return null;

var packetLength = ReadUInt16(packetLengthBuffer);        
if (packetLength < 3)
    return null;

Console.WriteLine($"{dbg} length {packetLength} contents: {Convert.ToHexString(packetLengthBuffer.First.Span)}");

result = await reader.ReadAtLeastAsync(1);
buffer = result.Buffer;
position = buffer.GetPosition(sizeof(byte));
var packetTypeBuffer = buffer.Slice(0, position);
reader.AdvanceTo(packetTypeBuffer.End);
if (result.IsCompleted)
    return null;

var packetType = ReadUInt8(packetTypeBuffer);

Console.WriteLine($"{dbg} type {packetType} contents: {Convert.ToHexString(packetTypeBuffer.First.Span)}");

var bodyLength = packetLength - 3;

result = await reader.ReadAtLeastAsync(bodyLength);
buffer = result.Buffer;
position = buffer.GetPosition(bodyLength);
var packetDataBuffer = buffer;
reader.AdvanceTo(packetDataBuffer.End);
if (result.IsCompleted)
    return null;

Console.WriteLine($"{dbg} body contents: {Encoding.UTF8.GetString(packetDataBuffer.First.Span)}");
// throws exception here ^^^
// doesn't throw if i read bodyLength - 1 bytes, but i need that last byte

return new(packetType, packetDataBuffer);

i'm writing a tcp server using system.io.pipelines, and i'm having trouble getting the last byte the client has sent before idling, seemingly matching this issue.