-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: Status Line parsing and writing #1135
Conversation
… ResponseHeader.SetStatusLine function call signature
I have been thinking about this and I'm really wondering if we should get rid of the pre-computed status messages so our code gets a bit cleaner. I have been running some benchmarks and calling Can you modify your pull request to change Line 1905 in 0c70e19
|
@erikdubbelboer That makes sense. I'm sure doing the whole bytes.Equal is taking a couple nanoseconds as well that it won't anymore. I've gone ahead and changed the Then, as suggested, I changed the The reason I didn't just delete the func (s *Server) writeFastError(w io.Writer, statusCode int, msg string) {
w.Write(formatStatusLine(nil, strHTTP11, statusCode, StatusMessage(statusCode))) //nolint:errcheck
...
} Another option is for us to convert the func (s *Server) writeFastError(w io.Writer, statusCode int, msg string) {
w.Write(formatStatusLine(nil, strHTTP11, statusCode, s2b(msg))) //nolint:errcheck
...
} But I don't think that's a valid approach since func (s *Server) ServeConn(c net.Conn) error {
...
if n > uint32(s.getConcurrency()) {
atomic.AddUint32(&s.concurrency, ^uint32(0))
s.writeFastError(c, StatusServiceUnavailable, "The connection cannot be served because Server.Concurrency limit exceeded")
c.Close()
return ErrConcurrencyLimit
}
...
} |
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
Alright, I think that should be everything. Sorry for the delay in those last few changes! |
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.
Just one last test that is failing, after that I'll merge it.
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
Thanks! |
This is a reference to #1132 and it corrects a number of bugs (while keeping zero-allocation)
First, it corrects loopholelabs@36ba831#diff-445b7d49a79bb217c576f137077e18262a0a2d58e424239fcc01a437397188f7R1883 where instead of comparing the status message to the formatted status message (which is what is returned by the
statusLine
function), it instead compares the status message to thestatusLines
map entry.Next, it modifies the
SetStatusLine
function (loopholelabs@36ba831#diff-445b7d49a79bb217c576f137077e18262a0a2d58e424239fcc01a437397188f7R149) to require a status code as well as a status message, and this is used to generate a properly formatted status line using the newformatStatusLine
function (loopholelabs@36ba831#diff-99087af835ba003ecaa8e8e178095e102304dd3b2aef74476ab06ea1e615601cR176).Finally, in order to convert the
statusCode
integer to a byte slice with zero-allocations, a newi2b
function has been created (loopholelabs@36ba831#diff-24779539a7f9c30594f395bab66c981b5069e92b84157cea9fa77ca1a6dbc9f1R369), which is just a slightly modified version ofstrconv.formatBits