API Proposal: Utf8Parser overloads for ReadOnlySequence<byte>

[davidfowl]

These text parsers are very convenient when parsing text based protocols (like HTTP/1.1 and RESP). It would be great to have overloads of the existing APIs that support ReadOnlySequence<byte>. I end up writing code like this:

private static bool TryParse(in ReadOnlySequence<byte> bytes, out int value, out int bytesConsumed)
{
    if (bytes.IsSingleSegment)
    {
        if (!Utf8Parser.TryParse(bytes.FirstSpan, out value, out bytesConsumed))
        {
            return false;
        }
    }
    else
    {
        Span<byte> textSpan= stackalloc byte[128];
        bytes.CopyTo(textSpan);

        if (!Utf8Parser.TryParse(textSpan, out value, out bytesConsumed))
        {
            return false;
        }
    }

    return true;
}

I would be great if this was built into the existing parser.

public partial static class Utf8Parser
{
    public static bool TryParse(in ReadOnlySequence<byte> source, out bool value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out byte value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out DateTime value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out DateTimeOffset value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out decimal value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out double value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out Guid value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out short value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out int value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out long value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out sbyte value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out float value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out TimeSpan value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out ushort value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out uint value, char standardFormat = '\0');
    public static bool TryParse(in ReadOnlySequence<byte> source, out ulong value, char standardFormat = '\0');
}

cc @ahsonkhan @GrabYourPitchforks

[ahsonkhan]

Should we consider adding these APIs on SequenceReader instead (or potentially in addition to)?

Something like these extension methods:
https://github.com/dotnet/corefxlab/blob/94248fb2ea9881936d6338b0d2d62ca8c79cbd79/src/System.Buffers.ReaderWriter/System/Buffers/Reader/BufferReader_text.cs#L239-L275

The bytesConsumed parameter doesn't make too much sense for a SequencePosition. How about consumedPosition (or sequenceConsumed, consumedUntil, etc.)?

cc @JeremyKuhne

[davidfowl]

I originally had that but the problem is that with text based protocols are usually delimited. For example, say you wanted to parse an HTTP/1.1 response (e.g. HTTP/1.1 200 OK\r\n). You want to write code that looks like this:

// HTTP/1.1
if (!sequenceReader.TryReadTo(out ReadOnlySpan<byte> version, (byte)' '))
{
    return;
}

// 200
if (!sequenceReader.TryReadTo(out ReadOnlySpan<byte> statusCodeText, (byte)' '))
{
    return;
}
else if (!Utf8Parser.TryParse(statusCodeText, out int statusCode, out _))
{
    return;
}

// OK
if (!sequenceReader.TryReadTo(out ReadOnlySequence<byte> statusText, NewLine))
{
    return;
}

If we add these parser APIs, how do you avoid parsing the status code into an integer until you have the full payload?

// HTTP/1.1
if (!sequenceReader.TryReadTo(out ReadOnlySpan<byte> version, (byte)' '))
{
    return;
}

// 200
// This code is incorrect because it'll parse 2, 20, and 200 successfully. So we need to scan for a space before we start parsing to make sure we have the full payload.
if (!sequenceReader.TryParse(out int statusCode))
{
    return;
}

// OK
if (!sequenceReader.TryReadTo(out ReadOnlySequence<byte> statusText, NewLine))
{
    return;
}

[GrabYourPitchforks]

(Standard grumbling that this belongs on an AsciiParser class, not a Utf8Parser class, since presumably you want these APIs to fail if they encounter non-ASCII data.)

The APIs seem reasonable. We'd have to be a little careful with the implementation because each method would need to know the worst-case input length based on the out T and the char standardFormat parameters. It's likely that any introduced APIs will have bugs in edge case handling, especially for types like double where the input buffer could be arbitrarily long. That doesn't mean we shouldn't do them, but it does potentially mean we should consider carefully which overloads we want to ship with.

[scalablecory]

I'd love to find a more general way to tackle this. Adding these convenience overloads for everything parsing a Span will get tedious, and it would be tacit encouragement to parse ReadOnlySequence directly.

[scalablecory]

I don't know if I'd want it in corefx but an extension on ReadOnlySequence could make this simple:

// return FirstSpan if single element, otherwise copy to storage and return storage.
ReadOnlySpan<byte> ReadOnlySequence.MaybeCopyTo(Span<byte> storage);

Utf8Parser.TryParse(byteSequence.MaybeCopyTo(stackalloc byte[128]), out int statusCode, out int bytesConsumed);

[davidfowl]

I'd love to find a more general way to tackle this. Adding these convenience overloads for everything parsing a Span will get tedious, and it would be tacit encouragement to parse ReadOnlySequence directly.

Case by case seems fine but if we're looking at language features then I rather not block that to add these very simple overloads that I keep re-writing 😄 .

[davidfowl]

The APIs seem reasonable. We'd have to be a little careful with the implementation because each method would need to know the worst-case input length based on the out T and the char standardFormat parameters. It's likely that any introduced APIs will have bugs in edge case handling, especially for types like double where the input buffer could be arbitrarily long. That doesn't mean we shouldn't do them, but it does potentially mean we should consider carefully which overloads we want to ship with.

We're just delegating to the Span overloads for most of these (I would assume all check for overflow). Isn't double the only special case? Doesn't it already have to be handled by the span overload?

[GrabYourPitchforks]

Isn't double the only special case? Doesn't it already have to be handled by the span overload?

Not sure if it's the only special case. And the span overloads can assume that they have all the data, which means that edge case handling is simpler. As an example of something that might be error-prone: what happens if the input is a 100GB stream consisting of all zeroes and you pass such a stream to the double parser? We can't delegate to the span parser because it can't handle payloads that large.

Again, not an argument that we shouldn't do it, but more of an admission that the more we try to support the higher the odds of this being a bug farm. So we'd want to weigh carefully the tradeoffs of having an API that works directly with sequences vs. telling customers to find the delimiters themselves, slice, and use the span-based APIs.

[davidfowl]

Not sure if it's the only special case. And the span overloads can assume that they have all the data, which means that edge case handling is simpler.

Why can't these overloads assume the same thing?

As an example of something that might be error-prone: what happens if the input is a 100GB stream consisting of all zeroes and you pass such a stream to the double parser? We can't delegate to the span parser because it can't handle payloads that large.

What happens if you do that for the span overload?

[GrabYourPitchforks]

What happens if you do that for the span overload?

If you pass int.MaxValue worth of zeroes to the span overload? It should handle them just fine and return 0.0.

[davidfowl]

Constrain the input size to int.MaxValue for ReadOnlySequence

[jkotas]

For example, say you wanted to parse an HTTP/1.1 response (e.g. HTTP/1.1 200 OK\r\n). You want to write code that looks like this:

Is this example how you imagine writing production quality high-performance HTTP response parser, e.g. are you going to use these APIs in Kestrel if we add them?

[davidfowl]

That would be ideal if the performance drop wasn't terrible. We started to write an HTTP client for performance testing here https://github.com/sebastienros/httpsocket/blob/38f3ee6d4a357fa60fec4a8b6cf802ce7e921924/src/HttpSocket.cs#L146-L175 and it uses the above code.

[jkotas]

So what is this code fragment going to look like with this API?