fix RawHeaders for POST #29
Conversation
| @@ -129,16 +127,54 @@ func TestRequestRawHeaders(t *testing.T) { | |||
| if string(h.Host()) != "foobar" { | |||
| t.Fatalf("unexpected host: %q. Expecting %q", h.Host(), "foobar") | |||
| } | |||
| v1 := h.Peek("EmptyValue") | |||
| if len(v1) > 0 { | |||
| t.Fatalf("expecting empty value. Got %q", v1) | |||
There was a problem hiding this comment.
Why the need to remove EmptyValue testing?
There was a problem hiding this comment.
EmptyValue check is not the purpose if this Test. It has been copied alongside other checks (by me, in #27) from a general headers tests that remains unchanged.
In other words EmptyValue is tested elsewhere, the removal is my attempt to DRY my previous PR.
|
I'm observing odd behaviour with non-trivial binary bodies in my fork, where rawHeaders is taken right from the body. Please consider this PR work in progress until I'm able to identify and fix the root cause of this issue. |
header.go
Outdated
| @@ -1620,6 +1620,7 @@ func (h *RequestHeader) parse(buf []byte) (int, error) { | |||
| if err != nil { | |||
| return 0, err | |||
| } | |||
| h.rawHeaders = buf[m : m+n] | |||
There was a problem hiding this comment.
I don't think this is going to work. parse is called from tryRead where it's buffer argument comes from mustPeekBuffered which as you can see gets its return value from bufio.Reader.Peek which will change its return value after a next read (for example for the body) making rawHeaders contain invalid data.
I haven't tested this myself but I see your tests don't cover this case. Can you add a test here a whole POST request including a body of at least 4kb is parsed instead of just the RequestHeader.
There was a problem hiding this comment.
Thanks! I have just followed the same trail literally minutes ago, will add a test shortly.
|
While this fixed the issue, the performance implications are unfortunately not trivial: @erikdubbelboer For my particular use case this loss is well worth it, but I understand if you would like to postpone the merge. Ideas how we can work around Peek's sliding window for this particular code path are definitely welcome. |
|
How are you benchmarking this? I wouldn't expect a big difference seeing as |
|
Fair point! I'm comparing go test -bench='BenchmarkServer(Post|Get)10KReq' -count=10between my branch and your master. Laptop unfortunately. Here's a rerun, this time with lower difference. |
|
Could it be that you're mixing up old an new? On my benchmarks the new code always seems to be faster: I'm using Go version go1.10 darwin/amd64. |
|
Funny. No, I'm not mixing it, but this is also not the first time laptop benchmarks fail me. It's Go v1.10 on darwin as well. I'll post stats from linux/amd64 @ ec2 later today. |
~> benchstat --geomean fhttp postfix # 10 runs, linux/amd4 @ c4.8xlarge
name old time/op new time/op delta
ServerGet10KReqPerConn-36 610ns ± 8% 624ns ± 9% ~ (p=0.289 n=10+10)
ServerPost10KReqPerConn-36 654ns ±12% 637ns ± 9% ~ (p=0.403 n=10+10)
[Geo mean] 632ns 630ns -0.29%I'm getting convinced any real difference will be noticeable only on the most stable setups. I feel comfortable merging it at this point. |
|
On my Ubuntu server with 1.10 the new code is still faster. I also think it’s ready but just to be sure I just put it live on one of our servers to test. If nothing strange happens I’ll merge it tomorrow. |
I'm sorry for the hassle, but apparently fhttp has fast path header parser for HEAD and GET and
rawHeadersisn't properly set for other methods. This PR is a fix to #27.rawHeadersto a proper sub-slice for methods with possible body.headerScannerto properly return parsed length.Admittedly this affects global hot path, but any difference is lost in noise:
Intuitively, one int counter doesn't change much.