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

Detect HTTP/2 connection sent to HTTP/1.1 endpoint #39402

Merged
merged 9 commits into from
Jan 14, 2022

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jan 10, 2022

Related to #39358 except an HTTP/2 connection sent to HTTP/1.1 endpoint.

image

return Task.CompletedTask;
else
{
return Output.FlushAsync(CancellationToken.None).GetAsTask();
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'm not sure about this. Need somewhere to flush the HTTP/2 GOAWAY frame content written in TryParseRequest.

Copy link
Member

Choose a reason for hiding this comment

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

It does feal weird to flush a response when _connectionAborted is true. If we stick with writing a GOAWAY for h2c connections to an HTTP/1.1 endpoint, I'd probably just make this virtual (it only gets called once per connection anyway) and use a new flag in DetectHttp2Preface (_isInvalidH2cConnection?). If set, I'd write the Http2GoAwayProtocolErrorBytes here and flush it and skip the default behavior. Otherwise, I'd just call into the base implementation.

That way you wouldn't have to set _connectionAborted in DetectHttp2Preface either which feels like a win.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

I'm not resetting the new flag in OnReset. Is that ok? I'm not familiar with HTTP/1.1 reuse so I'm not sure whether it will get reused after keep-alive is set to false.

@JamesNK JamesNK changed the title Detect HTTP version on invalid HTTP/2 connection Detect HTTP/2 connection sent to HTTP/1.1 endpoint Jan 10, 2022
@davidfowl davidfowl modified the milestone: .NET 7.0 Jan 10, 2022
Base automatically changed from jamesnk/kestrel-detect-version to main January 12, 2022 23:10
@JamesNK JamesNK force-pushed the jamesnk/kestrel-detect-version-http11 branch 2 times, most recently from 45458dc to a5be506 Compare January 13, 2022 21:27
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

If you can sort out the flush logic with Stephen

@JamesNK JamesNK force-pushed the jamesnk/kestrel-detect-version-http11 branch from a5be506 to 7521aba Compare January 13, 2022 23:16
@JamesNK JamesNK enabled auto-merge (squash) January 14, 2022 03:10
@JamesNK JamesNK merged commit 2e66c71 into main Jan 14, 2022
@JamesNK JamesNK deleted the jamesnk/kestrel-detect-version-http11 branch January 14, 2022 04:42
@ghost ghost added this to the 7.0-preview1 milestone Jan 14, 2022
@davidfowl
Copy link
Member

Did this affect the performance of the HTTP/1.1 connection close benchmark?

cc @sebastienros

@davidfowl
Copy link
Member

Ah I see this only affects the error case.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants