Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add unit test for variable length encoding HTTP/3 #60766

Merged
merged 1 commit into from
Nov 9, 2021
Merged

add unit test for variable length encoding HTTP/3 #60766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions ...raries/Common/src/System/Net/Http/aspnetcore/Http3/Helpers/VariableLengthIntegerHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public static bool TryWrite(Span<byte> buffer, long longToEncode, out int bytesW
Debug.Assert(longToEncode >= 0);
Debug.Assert(longToEncode <= EightByteLimit);

if (longToEncode < OneByteLimit)
if (longToEncode <= OneByteLimit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug in header encoding? If so, what happens when an integer that is right at the boundary between limits triggers the bug?

Copy link
Contributor Author

@pedrobsaila pedrobsaila Oct 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, It's not bug, because we still produce correct values with more bytes (example 63 is encoded as 010000000 ,00111111 but can be juste 00111111). https://tools.ietf.org/html/draft-ietf-quic-transport-16#section-16 tells values at the edge can be encoded in less bytes so why using more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so those edge values are just not as efficient as they could be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that something that we should fix? If so, let's make sure we have an issue to track it, even if we think it's low-priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a bug, it's rather an optimization because we could encode for example 1 on 2 bytes or 4 bytes. Nothing prohibit that in the specs : Length ranges are cumulative

2Bit Length Usable Bits Range
00 1 6 0-63
01 2 14 0-16383
10 4 30 0-1073741823
11 8 62 0-4611686018427387903

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand it's not a bug; I'm asking if we can optimize this better than we do currently.

{
if (buffer.Length != 0)
{
Expand All @@ -158,15 +158,15 @@ public static bool TryWrite(Span<byte> buffer, long longToEncode, out int bytesW
return true;
}
}
else if (longToEncode < TwoByteLimit)
else if (longToEncode <= TwoByteLimit)
{
if (BinaryPrimitives.TryWriteUInt16BigEndian(buffer, (ushort)((uint)longToEncode | TwoByteLengthMask)))
{
bytesWritten = 2;
return true;
}
}
else if (longToEncode < FourByteLimit)
else if (longToEncode <= FourByteLimit)
{
if (BinaryPrimitives.TryWriteUInt32BigEndian(buffer, (uint)longToEncode | FourByteLengthMask))
{
Expand Down Expand Up @@ -200,9 +200,9 @@ public static int GetByteCount(long value)
Debug.Assert(value <= EightByteLimit);

return
value < OneByteLimit ? 1 :
value < TwoByteLimit ? 2 :
value < FourByteLimit ? 4 :
value <= OneByteLimit ? 1 :
value <= TwoByteLimit ? 2 :
value <= FourByteLimit ? 4 :
8; // EightByteLimit
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/Common/tests/Common.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@
Link="Common\System\Net\Http\aspnetcore\Http2\Hpack\H2StaticTable.Http2.cs" />
<Compile Include="$(CommonPath)System\Net\Http\aspnetcore\Http2\Hpack\StatusCodes.cs"
Link="Common\System\Net\Http\aspnetcore\Http2\Hpack\StatusCodes.cs" />
<Compile Include="$(CommonPath)System\Net\Http\aspnetcore\Http3\Helpers\VariableLengthIntegerHelper.cs"
Link="Common\System\Net\Http\aspnetcore\Http3\Helpers\VariableLengthIntegerHelper.cs" />
<Compile Include="$(CommonPath)System\Text\SimpleRegex.cs"
Link="Common\System\Text\SimpleRegex.cs" />
<Compile Include="$(CommonPath)System\Text\ValueStringBuilder.cs"
Expand Down Expand Up @@ -95,6 +97,7 @@
<Compile Include="Tests\System\Net\aspnetcore\Http2\HPackDecoderTest.cs" />
<Compile Include="Tests\System\Net\aspnetcore\Http2\HPackIntegerTest.cs" />
<Compile Include="Tests\System\Net\aspnetcore\Http2\HuffmanDecodingTests.cs" />
<Compile Include="Tests\System\Net\aspnetcore\Http3\VariableLengthIntegerHelperTests.cs" />
<Compile Include="System\Net\Sockets\Fletcher32.cs"
Link="System\Net\Sockets\Fletcher32.cs" />
<Compile Include="$(CommonPath)System\Net\Logging\NetEventSource.Common.cs"
Expand Down
Loading