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

HTTP3: Re-enable cookie and cancellation tests #54727

Merged
merged 11 commits into from
Jun 26, 2021

Conversation

geoffkizer
Copy link
Contributor

Fixes #53093

Fix an issue in the GOAWAY frame parsing so we process the GOAWAY properly.

Fix an issue where we were not properly framing the request content when we had already sent the request headers before starting to send request content. This only surfaced when using Expect: 100-continue, since in other cases we either don't send the headers before sending content, or we have no content.

Change the Http3LoopbackServer to behave like HTTP2 for HandleRequestAsync, which is to shutdown the connection after the request (actually, send the GOAWAY after receiving the request but before sending the response).

Add SendPartialResponseHeadersAsync to GenericLoopbackConnection, implement for each protocol version, and use in appropriate tests. Support for sending "partial" headers is removed from SendResponseAsync.

Minor cleanup in loopback server implementations (e.g., statusCode shouldn't be nullable, be consistent about parameter names and default values, etc).

Disable two interop tests which are failing consistently.

Enable Cookie and Cancellation tests for HTTP3.

With this change, outerloop runs for HTTP3 with both MsQuic and Mock are clean, though not 100% reliable.

@ghost
Copy link

ghost commented Jun 25, 2021

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

Issue Details

Fixes #53093

Fix an issue in the GOAWAY frame parsing so we process the GOAWAY properly.

Fix an issue where we were not properly framing the request content when we had already sent the request headers before starting to send request content. This only surfaced when using Expect: 100-continue, since in other cases we either don't send the headers before sending content, or we have no content.

Change the Http3LoopbackServer to behave like HTTP2 for HandleRequestAsync, which is to shutdown the connection after the request (actually, send the GOAWAY after receiving the request but before sending the response).

Add SendPartialResponseHeadersAsync to GenericLoopbackConnection, implement for each protocol version, and use in appropriate tests. Support for sending "partial" headers is removed from SendResponseAsync.

Minor cleanup in loopback server implementations (e.g., statusCode shouldn't be nullable, be consistent about parameter names and default values, etc).

Disable two interop tests which are failing consistently.

Enable Cookie and Cancellation tests for HTTP3.

With this change, outerloop runs for HTTP3 with both MsQuic and Mock are clean, though not 100% reliable.

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

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.

Some small comments, otherwise LGTM, thanks.

@@ -184,7 +187,8 @@ public class HttpRequestData
public string Path;
public Version Version;
public List<HttpHeaderData> Headers { get; }
public int RequestId; // Generic request ID. Currently only used for HTTP/2 to hold StreamId.
public int RequestId; // HTTP/2 StreamId.
public long Http3StreamId;
Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm really unhappy about this, but what about this: https://github.com/dotnet/runtime/compare/main...ManickaP:mapichov/h3_request_id?expand=1???

I can as well make it long, although I'm not sure we'll ever hit such large ID in our tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should definitely fix the current hack in GetRequestId() because that's just going to be super confusing if we keep it as it is.

That said, I looked at the usage of the RequestId field and it seems like it's not really used at all currently. Am I missing something here? If it's not used, let's just remove it and save ourselves the hassle.

For now, I can implement the GOAWAY support without having to add this field, which is probably the cleanest thing to do right now. I'll push an update.

Copy link
Member

Choose a reason for hiding this comment

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

I found it used only here:

return (requestData.RequestId, requestData);

But that's within the same context as its assignment, so we can just use local variable instead.

@geoffkizer
Copy link
Contributor Author

@dotnet/dnceng Seems like there are infrastructure issues?

@lukas-lansky
Copy link
Contributor

@geoffkizer Indeed, there's an open IcM with AzDO about the issue.

@geoffkizer geoffkizer merged commit 7527d93 into dotnet:main Jun 26, 2021
@geoffkizer geoffkizer deleted the enablehttp3tests branch June 26, 2021 01:45
@karelz karelz added this to the 6.0.0 milestone Jul 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HTTP/3] Disabled test classes
4 participants