-
Notifications
You must be signed in to change notification settings - Fork 199
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
feat: h2 server support #96
base: master
Are you sure you want to change the base?
Conversation
@@ -243,10 +261,16 @@ func matchHTTP2Field(w io.Writer, r io.Reader, name string, matches func(string) | |||
// Sender acknoweldged the SETTINGS frame. No need to write | |||
// SETTINGS again. | |||
if f.IsAck() { | |||
// Avoid causing golang.org/x/net/http2.serverConn.unackedSettings PROTOCOL_ERROR | |||
sr.discard() |
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.
Hi @lukeo3o1, I don't understand why call sr.discard
here. The purpose of bufferedReader
as I understand is that it keeps all data read from the connection to not miss anything for upper level reader of the connection. Am I right? I'm just curious and not much familiar with http2 protocol.
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.
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.
bypass https://cs.opensource.google/go/x/net/+/refs/tags/v0.30.0:http2/server.go;l=1738-1747
Thanks for reply! I can see the point. Is it because there is a bufferreader which causes the ack_mystery
error or sr.discard
is just a mitigation for that problem? I thought the bufferreader should be transparent for later reader of the connection.
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.
In fact, it's not the bufferedReader
that causes the issue. Rather, some clients will wait to receive the SETTINGS frame first, and once the SETTINGS frame is sent, the client will immediately respond with an ACK frame to confirm receipt. Without using sr.discard
as a workaround, it would lead to the issue described in https://cs.opensource.google/go/x/net/+/refs/tags/v0.30.0:http2/server.go;l=1738-1747.
The purpose of sr.discard
is to handle the ACK frame response and prevent it from being passed to the application layer, as the ACK frame does not contain meaningful data but is merely used to synchronize the protocol state. Without using sr.discard to discard the ACK frame, the ACK might enter subsequent processing, leading to logical errors or protocol inconsistencies. This is why this operation is necessary to bypass potential issues.
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 see. Thanks for your explaination.It's more complicated than I thought.
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 look through the start process of http2 server (https://cs.opensource.google/go/x/net/+/refs/tags/v0.30.0:http2/server.go;l=916-933). The server always sends setting frame first and wait for preface of clients. It seems weird that when received setting frame without ack a setting frame instead of a ack setting frame is sent to the client. Because of it cilents need to send back ack setting frame.And bufferreader keeps the ack setting frame which is extra for the application layer.
Maybe It is not necessary to send setting frame(without ack flag) here and ack setting frame wouldn't be read at this time.
Fix #95