Repository navigation
HTTP3: Re-enable cookie and cancellation tests - #54727
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #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.
|
ManickaP
left a comment
There was a problem hiding this comment.
Some small comments, otherwise LGTM, thanks.
| 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I found it used only here:
But that's within the same context as its assignment, so we can just use local variable instead.
|
@dotnet/dnceng Seems like there are infrastructure issues? |
|
@geoffkizer Indeed, there's an open IcM with AzDO about the issue. |
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.