-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: automatically add content-length for small responses #3989
Conversation
…all enough and doesn't call Flush
Codecov Report
@@ Coverage Diff @@
## master #3989 +/- ##
==========================================
+ Coverage 82.94% 82.95% +0.01%
==========================================
Files 147 147
Lines 14781 14801 +20
==========================================
+ Hits 12260 12278 +18
- Misses 2022 2025 +3
+ Partials 499 498 -1
|
@marten-seemann Do you have time to take a look at this pr? |
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.
Thank you for this PR!
Can you add an integration test (in http_test.go) that tests end-to-end that this actually works?
I'm also wondering if we should have any logic to send out large (> 1 MTU?) headers without waiting for the write call. What do you think?
I don't known if we should flush headers immediately for large headers 😅 , as far as I know, I use this branch and previous one without encountering any problems at all. |
I don't know why these connection tests failed 🤷 . |
http3/response_writer.go
Outdated
@@ -15,7 +15,12 @@ import ( | |||
"github.com/quic-go/qpack" | |||
) | |||
|
|||
// headerWriter wrap the stream, so first Write will flush header to the stream | |||
// The maximum length of an encoded quic frame header is 10. | |||
// 1 (header type) + variable (frame length, the length of the largest encoded quic varint is 9). |
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's 8.
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.
The type is not guaranteed to be 1 byte though, is it?
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.
16 comes from the original code for encoding frame header. The type is 1 byte for header and data frame according to the code.
It's 8 byte. My mistake.
…all enough and doesn't call Flush
Another iteration of 3954.