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

Jump to bottom

[HTTP/3] Reenable interop tests #56867

Merged
merged 3 commits into from
Aug 6, 2021
Merged

[HTTP/3] Reenable interop tests #56867

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63283a9
Reenabled tests
ManickaP Aug 4, 2021
3cf5c1b
Fixed interop tests, fixed handling of 0-byte DATA frame
ManickaP Aug 4, 2021
e7f0623
Addressed comments
ManickaP Aug 5, 2021
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
5 changes: 5 additions & 0 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1142,6 +1142,11 @@ private async ValueTask<bool> ReadNextDataFrameAsync(HttpResponseMessage respons
switch (frameType)
{
case Http3FrameType.Data:
// Ignore DATA frames with 0 length.
if (payloadLength == 0)
{
continue;
}
_responseDataPayloadRemaining = payloadLength;
return true;
case Http3FrameType.Headers:
Expand Down
50 changes: 42 additions & 8 deletions src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,6 @@ public async Task ServerCertificateCustomValidationCallback_Succeeds()
[OuterLoop]
[ConditionalTheory(nameof(IsMsQuicSupported))]
[MemberData(nameof(InteropUris))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/54726")]
public async Task Public_Interop_ExactVersion_Success(string uri)
{
if (UseQuicImplementationProvider == QuicImplementationProviders.Mock)
Expand All @@ -402,10 +401,33 @@ public async Task Public_Interop_ExactVersion_Success(string uri)
Assert.Equal(3, response.Version.Major);
}

[OuterLoop]
[ConditionalTheory(nameof(IsMsQuicSupported))]
[MemberData(nameof(InteropUrisWithContent))]
public async Task Public_Interop_ExactVersion_BufferContent_Success(string uri)
{
if (UseQuicImplementationProvider == QuicImplementationProviders.Mock)
{
return;
}

using HttpClient client = CreateHttpClient();
using HttpRequestMessage request = new HttpRequestMessage
{
Method = HttpMethod.Get,
RequestUri = new Uri(uri, UriKind.Absolute),
Version = HttpVersion.Version30,
VersionPolicy = HttpVersionPolicy.RequestVersionExact
};
using HttpResponseMessage response = await client.SendAsync(request, HttpCompletionOption.ResponseContentRead).WaitAsync(TimeSpan.FromSeconds(20));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we validate the actual content too? I don't know how these endpoints work. If they have fixed content that will never change, seems like we could validate that we receive the correct content.

Or if not, can we at least validate that the content exists, i.e. we receive > 0 bytes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a check for non-empty content.
I don't think we should check for a particular content value since these are external and can change any time.


Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.Equal(3, response.Version.Major);
}

[OuterLoop]
[ConditionalTheory(nameof(IsMsQuicSupported))]
[MemberData(nameof(InteropUris))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/54726")]
public async Task Public_Interop_Upgrade_Success(string uri)
{
if (UseQuicImplementationProvider == QuicImplementationProviders.Mock)
Expand All @@ -421,7 +443,7 @@ public async Task Public_Interop_Upgrade_Success(string uri)
{
Method = HttpMethod.Get,
RequestUri = new Uri(uri, UriKind.Absolute),
Version = HttpVersion.Version30,
Version = HttpVersion.Version20,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually matter? The first request should not be processed via HTTP3 regardless, because we haven't received the Alt-Svc header yet. In fact isn't that the point of setting Version30 here -- to validate that even though we requested 30, we won't get it on the first request because we haven't gotten an Alt-Svc yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

They were all failing with the RequestA being directly processed as H/3. The problem was in the test using a handler replacing HttpVersionPolicy.RequestVersionOrLower with HttpVersionPolicy.RequestVersionExact. I fixed that.

VersionPolicy = HttpVersionPolicy.RequestVersionOrLower
})
{
Expand All @@ -443,7 +465,7 @@ public async Task Public_Interop_Upgrade_Success(string uri)
using HttpResponseMessage responseB = await client.SendAsync(requestB).WaitAsync(TimeSpan.FromSeconds(20));

Assert.Equal(HttpStatusCode.OK, responseB.StatusCode);
Assert.NotEqual(3, responseB.Version.Major);
Assert.Equal(3, responseB.Version.Major);
Copy link
Contributor

Choose a reason for hiding this comment

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

How did this test ever work?? :)

}
}

Expand Down Expand Up @@ -754,14 +776,26 @@ private SslApplicationProtocol ExtractMsQuicNegotiatedAlpn(Http3LoopbackConnecti

/// <summary>
/// These are public interop test servers for various QUIC and HTTP/3 implementations,
/// taken from https://github.com/quicwg/base-drafts/wiki/Implementations
/// taken from https://github.com/quicwg/base-drafts/wiki/Implementations and https://bagder.github.io/HTTP3-test/.
/// </summary>
public static TheoryData<string> InteropUris() =>
new TheoryData<string>
{
{ "https://quic.rocks:4433/" }, // Chromium
{ "https://http3-test.litespeedtech.com:4433/" }, // LiteSpeed
{ "https://quic.tech:8443/" } // Cloudflare
{ "https://www.litespeedtech.com/" }, // LiteSpeed
{ "https://quic.tech:8443/" }, // Cloudflare
{ "https://quic.aiortc.org:443/" }, // aioquic
{ "https://h2o.examp1e.net/" } // h2o/quicly
};

/// <summary>
/// These are public interop test servers for various QUIC and HTTP/3 implementations,
/// taken from https://github.com/quicwg/base-drafts/wiki/Implementations and https://bagder.github.io/HTTP3-test/.
/// </summary>
public static TheoryData<string> InteropUrisWithContent() =>
new TheoryData<string>
{
{ "https://cloudflare-quic.com/" }, // Cloudflare with content
{ "https://pgjones.dev/" }, // aioquic with content
};
}
}