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

fix: Status Line parsing and writing #1135

Merged
merged 18 commits into from Nov 8, 2021
Merged

fix: Status Line parsing and writing #1135

merged 18 commits into from Nov 8, 2021

Conversation

ShivanshVij
Copy link
Contributor

@ShivanshVij ShivanshVij commented Oct 24, 2021

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 the statusLines 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 new formatStatusLine function (loopholelabs@36ba831#diff-99087af835ba003ecaa8e8e178095e102304dd3b2aef74476ab06ea1e615601cR176).

Finally, in order to convert the statusCode integer to a byte slice with zero-allocations, a new i2b function has been created (loopholelabs@36ba831#diff-24779539a7f9c30594f395bab66c981b5069e92b84157cea9fa77ca1a6dbc9f1R369), which is just a slightly modified version of strconv.formatBits

header.go Show resolved Hide resolved
header.go Outdated Show resolved Hide resolved
status.go Outdated Show resolved Hide resolved
status.go Outdated Show resolved Hide resolved
status.go Outdated Show resolved Hide resolved
header.go Outdated Show resolved Hide resolved
ShivanshVij and others added 5 commits Oct 24, 2021
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
…idStatusLine, and making `appendStatusLine` an unexported function

Issue: #1132
@ShivanshVij ShivanshVij requested a review from erikdubbelboer Oct 24, 2021
@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Oct 25, 2021

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 formatStatusLine takes around 22 nanoseconds on my machine vs 3 nanoseconds for the pre-computed lookup. The code being 20 nanoseconds slower is something body is ever going to notice.

Can you modify your pull request to change statusLines and just always call formatStatusLine. That way this whole if statement isn't needed either and it can just always call SetStatusMessage:

if len(b) > n+1 && !bytes.Equal(b[n+1:], s2b(StatusMessage(h.statusCode))) {

@ShivanshVij
Copy link
Contributor Author

ShivanshVij commented Oct 25, 2021

@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 StatusMessage function so that it returns a byte slice (as opposed to a string), that we can then feed directly into formatStatusLine.

Then, as suggested, I changed the parseFirstLine function so it always calls the SetStatusMessage function.

The reason I didn't just delete the StatusMessage function and instead modified it, is because it's used in the server.writeFastError function:

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 msg parameter in the server.writeFastError function to a byte slice, and use that instead of calling StatusMessage:

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 writeFastError is often called with non-standard messages:

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
	}
	...
}

header.go Outdated Show resolved Hide resolved
header.go Show resolved Hide resolved
status.go Outdated Show resolved Hide resolved
@ShivanshVij ShivanshVij requested a review from erikdubbelboer Nov 2, 2021
header.go Outdated Show resolved Hide resolved
header.go Outdated Show resolved Hide resolved
header.go Show resolved Hide resolved
ShivanshVij and others added 4 commits Nov 6, 2021
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
@ShivanshVij ShivanshVij requested a review from erikdubbelboer Nov 6, 2021
@ShivanshVij
Copy link
Contributor Author

ShivanshVij commented Nov 6, 2021

Alright, I think that should be everything. Sorry for the delay in those last few changes!

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Just one last test that is failing, after that I'll merge it.

header_test.go Outdated Show resolved Hide resolved
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
@ShivanshVij ShivanshVij requested a review from erikdubbelboer Nov 8, 2021
@erikdubbelboer erikdubbelboer merged commit 2ca01c7 into valyala:master Nov 8, 2021
8 of 11 checks passed
@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Nov 8, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants