-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Detect HTTP/2 connection sent to HTTP/1.1 endpoint #39402
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
Conversation
return Task.CompletedTask; | ||
else | ||
{ | ||
return Output.FlushAsync(CancellationToken.None).GetAsTask(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
45458dc
to
a5be506
Compare
There was a problem hiding this 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
a5be506
to
7521aba
Compare
Co-authored-by: Stephen Halter <halter73@gmail.com>
Did this affect the performance of the HTTP/1.1 connection close benchmark? |
Ah I see this only affects the error case. |
Related to #39358 except an HTTP/2 connection sent to HTTP/1.1 endpoint.