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.

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

Conversation

pedrobsaila
Copy link
Contributor

Fixes #51519

@pedrobsaila pedrobsaila requested a review from a team October 22, 2021 14:25
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 22, 2021
@ghost
Copy link

ghost commented Oct 22, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #51519

Author: pedrobsaila
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@@ -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

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.

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

There's similar edge case handling in loopback server:
https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/System/Net/Http/Http3LoopbackStream.cs#L171

Perhaps, it would be worth it to optimize the comparison there as well.

Otherwise looks good and thanks for the contribution.

@karelz karelz merged commit 6d5f59b into dotnet:main Nov 9, 2021
@karelz
Copy link
Member

karelz commented Nov 9, 2021

Build Analysis failed with:

runtime-libraries enterprise-linux 20211022.1 / Job
❌ An image label with the label ubuntu-16.04 does not exist.

This is also from other PRs. We should address it. @antonfirsov @wfurt can you please help here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/3: Unit test variable length encoding
5 participants