Resetting a Utf8JsonReader at a certain point causes discrepancy in BytePositionInLine

Execute the following program:

try
{
	var jsonBytes = Encoding.UTF8.GetBytes("{/");
	var readOnlySequence = Util.CreateReadOnlySequence(Util.SplitMemory(jsonBytes, 1));
	var reader = new Utf8JsonReader(readOnlySequence, new JsonReaderOptions() { CommentHandling = JsonCommentHandling.Skip });
	reader.Read();
	reader = new Utf8JsonReader(readOnlySequence.Slice(1), true, reader.CurrentState); //reset reader, comment this out
	reader.Read();
}
catch (Exception ex)
{
	Console.WriteLine(ex);
}

Next, comment out the marked line. The error message changes from

System.Text.Json.JsonReaderException: '/' is an invalid start of a value. LineNumber: 0 | BytePositionInLine: 1.

to

System.Text.Json.JsonReaderException: Unexpected end of data while reading a comment. LineNumber: 0 | BytePositionInLine: 2.

Note, that multi-segment mode is used and JsonCommentHandling.Skip is set.

This code requires the following utility class:

static class Util
{
    public static IEnumerable<Memory<byte>> SplitMemory(Memory<byte> memory, int chunkSize)
    {
        for (int startIndex = 0; startIndex < memory.Length; startIndex += chunkSize)
            yield return memory.Slice(startIndex, Math.Min(chunkSize, memory.Length - startIndex));
    }

    public static ReadOnlySequence<T> CreateReadOnlySequence<T>(IEnumerable<Memory<T>> buffers) => SimpleReadOnlySequenceSegment<T>.Create(buffers);

    class SimpleReadOnlySequenceSegment<T> : ReadOnlySequenceSegment<T>
    {
        internal static ReadOnlySequence<T> Create(IEnumerable<Memory<T>> buffers)
        {
            SimpleReadOnlySequenceSegment<T> segment = null;
            SimpleReadOnlySequenceSegment<T> first = null;
            foreach (Memory<T> buffer in buffers)
            {
                var newSegment = new SimpleReadOnlySequenceSegment<T>()
                {
                    Memory = buffer,
                };

                if (segment != null)
                {
                    segment.Next = newSegment;
                    newSegment.RunningIndex = segment.RunningIndex + segment.Memory.Length;
                }
                else
                {
                    first = newSegment;
                }

                segment = newSegment;
            }

            if (first == null)
            {
                first = segment = new SimpleReadOnlySequenceSegment<T>();
            }

            return new ReadOnlySequence<T>(first, 0, segment, segment.Memory.Length);
        }
    }
}

This discrepancy can cause behavior changes depending on how big chunks of data read from the network are. Recreating a reader from a state is required in streaming scenarios with async IO. I quote my reasoning for fixing this from my previous issue:

I found a few cases where the values for BytesConsumed, BytePositionInLine or the error message deviate between modes. Depending on the segmentation chunking, the error message and position numbers can be different for the same JSON. This could be an issue for debuggability and diagnosing production errors. It seems desirable that the segment mode should not affect parsing outcome.

Also, is BytePositionInLine used to make parsing decisions? It seems that CheckLiteralMultiSegment uses it. In that case, this might actually be a functional parsing bug.

@GSPP

The error message changes

This discrepancy is because of the differences in the code-paths for single-segment vs multi-segment payloads. When you slice the sequence during the creation of another Utf8JsonReader, it becomes single-segment. Without that, it remains multi-segment. That's a good test case, @GSPP.

Single-segment doesn't increment _bytePositionInLine before checking if there is any data left.
https://github.com/dotnet/corefx/blob/70b4d01e18236c925c2d44d49fff7cbb4919dc43/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs#L2290-L2297

Multi-segment does increment it:
https://github.com/dotnet/corefx/blob/70b4d01e18236c925c2d44d49fff7cbb4919dc43/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs#L2223-L2232

We changed the multi-segment code path for comment handling (dotnet/corefx#38036). We should make similar changes to the single-segment path as well.

System.Text.Json.JsonReaderException: Unexpected end of data while reading a comment. LineNumber: 0 | BytePositionInLine: 2.

I think this should be the expected exception thrown along with byte position set to 2, in both cases.

cc @krwq

Given this is only affecting the exception thrown for invalid JSON in an edge case (and where we are in the product for 3.0), moving to 5.0. I don't think this behavioral discrepancy would have significant impact on callers.

Also, is BytePositionInLine used to make parsing decisions? It seems that CheckLiteralMultiSegment uses it. In that case, this might actually be a functional parsing bug.

That's a good point. However, I don't see any feasible link between when the byte position gets mismatched and when the parser hits the code path in CheckLiteralMultiSegment (particularly because the bug in byte position only occurs on invalid JSON parsing which throws/stops the parser, leaving it an undefined state to continue further anyway).

I see! Thanks for the detailed reply.

Indeed, all repro cases, which I can generate, have the / at the end. This is what triggers single-segment mode.

I was experimenting with resetting the reader after each token. I was unable to generate other issues that way.

Closing this since there is nothing to do.

Closing this since there is nothing to do.

I think its still worth making things consistent between the two code paths for completeness (hence re-opening). That said, it's unlikely for this issue to cause much concern for most use cases.

We changed the multi-segment code path for comment handling (dotnet/corefx#38036). We should make similar changes to the single-segment path as well.

System.Text.Json.JsonReaderException: Unexpected end of data while reading a comment. LineNumber: 0 | BytePositionInLine: 2.

I think this should be the expected exception thrown along with byte position set to 2, in both cases.

@eiriktsarpalis

@eiriktsarpalis I'm pinging you on this issue as well so that you can make a scheduling decision rather than letting the bot make it by default.

@ahsonkhan

It still reproduces in .NET 6, here's a standalone reproduction:

using System;
using System.Text;
using System.Text.Json;
using System.Buffers;

var readOnlySequence = Utils.CreateReadOnlySequence(Encoding.UTF8.GetBytes("{"), Encoding.UTF8.GetBytes("/"));
var reader = new Utf8JsonReader(readOnlySequence, new JsonReaderOptions() { CommentHandling = JsonCommentHandling.Skip });
reader.Read();

var clonedReader = new Utf8JsonReader(readOnlySequence.Slice(1), true, reader.CurrentState);
try { reader.Read(); } catch (JsonException ex) { Console.WriteLine($"Expected error message: {ex.Message}"); }
try { clonedReader.Read(); } catch (JsonException ex) { Console.WriteLine($"Actual error message: {ex.Message}"); };

public static class Utils
{
    public static ReadOnlySequence<T> CreateReadOnlySequence<T>(params T[][] buffers)
    {
        if (buffers == null || buffers.Length == 0)
        {
            return default;
        }

        if (buffers.Length == 1)
        {
            return new ReadOnlySequence<T>(buffers[0]);
        }

        var tail = new MySegment<T>(buffers[^1]);
        MySegment<T>? current = tail;

        for (int i = buffers.Length - 2; i >= 0; i--)
        {
            current = new MySegment<T>(buffers[i], current);
        }

        MySegment<T> head = current!;
        long runningIndex = 0;

        while (current != null)
        {
            current.SetRunningIndex(runningIndex);
            runningIndex += current.Memory.Length;
            current = (MySegment<T>?)current.Next;
        }

        return new ReadOnlySequence<T>(head, 0, tail, tail.Memory.Length);
    }

    private class MySegment<T> : ReadOnlySequenceSegment<T>
    {
        public MySegment(ReadOnlyMemory<T> memory, MySegment<T>? next = null)
        {
            Memory = memory;
            Next = next;
        }

        public void SetRunningIndex(long index) => RunningIndex = index;
    }
}

Per @ahsonkhan's comments we should investigate the discrepancy although it doesn't seem like a high-enough priority issue.

Navigation Menu

Repository navigation

Resetting a Utf8JsonReader at a certain point causes discrepancy in BytePositionInLine #30751

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Resetting a Utf8JsonReader at a certain point causes discrepancy in BytePositionInLine #30751

Description

Activity

ahsonkhan commented on Sep 4, 2019

GSPP commented on Sep 8, 2019

ahsonkhan commented on Sep 9, 2019

GSPP commented on Oct 21, 2021

eiriktsarpalis commented on Oct 21, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions