From d428e1b05ec9f322d7f48868c2e9abd4a515e45d Mon Sep 17 00:00:00 2001 From: Erik Dubbelboer Date: Thu, 17 Oct 2019 19:06:14 +0200 Subject: [PATCH] Fix race condition in tests, fix URI bug - Some tests can't be run in parallel. - `URI` had a pointer to `RequestHeader` which was updated with `RequestHeader.CopyTo` which resulted in the URI pointing to the wrong `RequestHeader` causing bugs and race conditions. The only reason `URI` contained a pointer to `RequestHeader` was to delay the call to `RequestHeader.Host()` until really needed. But these days instead of parsing all headers, `RequestHeader.Host()` uses `RequestHeader.peekRawHeader()` which is rather fast. So we can remove the pointer in `URI` and completely decouple the two structs improving code quality and fixing the bug. For some reason this results in faster code on average as well: benchmark old ns/op new ns/op delta BenchmarkClientGetEndToEnd1Inmemory-8 1189 1369 +15.14% BenchmarkClientGetEndToEnd10Inmemory-8 1143 1161 +1.57% BenchmarkClientGetEndToEnd100Inmemory-8 1228 1236 +0.65% BenchmarkClientGetEndToEnd1000Inmemory-8 1213 1213 +0.00% BenchmarkClientGetEndToEnd10KInmemory-8 1362 1350 -0.88% BenchmarkClientEndToEndBigResponse1Inmemory-8 139967 130070 -7.07% BenchmarkClientEndToEndBigResponse10Inmemory-8 142233 131809 -7.33% BenchmarkServerGet1ReqPerConn-8 1726 1593 -7.71% BenchmarkServerGet2ReqPerConn-8 882 927 +5.10% BenchmarkServerGet10ReqPerConn-8 440 436 -0.91% BenchmarkServerGet10KReqPerConn-8 341 339 -0.59% BenchmarkServerPost1ReqPerConn-8 1728 1706 -1.27% BenchmarkServerPost2ReqPerConn-8 968 963 -0.52% BenchmarkServerPost10ReqPerConn-8 506 505 -0.20% BenchmarkServerPost10KReqPerConn-8 424 420 -0.94% BenchmarkServerGet1ReqPerConn10KClients-8 1117 1051 -5.91% BenchmarkServerGet2ReqPerConn10KClients-8 565 514 -9.03% BenchmarkServerGet10ReqPerConn10KClients-8 390 387 -0.77% BenchmarkServerGet100ReqPerConn10KClients-8 355 348 -1.97% BenchmarkServerHijack-8 339 348 +2.65% BenchmarkServerMaxConnsPerIP-8 326 325 -0.31% BenchmarkServerTimeoutError-8 24355 24180 -0.72% --- client_test.go | 10 ++++++++-- fs_test.go | 6 ++++-- http.go | 2 +- uri.go | 25 +++++-------------------- 4 files changed, 18 insertions(+), 25 deletions(-) diff --git a/client_test.go b/client_test.go index 00765ab..1eab344 100644 --- a/client_test.go +++ b/client_test.go @@ -610,24 +610,30 @@ func TestClientDoWithCustomHeaders(t *testing.T) { } func TestPipelineClientDoSerial(t *testing.T) { + t.Parallel() + testPipelineClientDoConcurrent(t, 1, 0, 0) } func TestPipelineClientDoConcurrent(t *testing.T) { + t.Parallel() + testPipelineClientDoConcurrent(t, 10, 0, 1) } func TestPipelineClientDoBatchDelayConcurrent(t *testing.T) { + t.Parallel() + testPipelineClientDoConcurrent(t, 10, 5*time.Millisecond, 1) } func TestPipelineClientDoBatchDelayConcurrentMultiConn(t *testing.T) { + t.Parallel() + testPipelineClientDoConcurrent(t, 10, 5*time.Millisecond, 3) } func testPipelineClientDoConcurrent(t *testing.T, concurrency int, maxBatchDelay time.Duration, maxConns int) { - t.Parallel() - ln := fasthttputil.NewInmemoryListener() s := &Server{ diff --git a/fs_test.go b/fs_test.go index 5d53f68..19d1f30 100644 --- a/fs_test.go +++ b/fs_test.go @@ -435,6 +435,8 @@ func testParseByteRangeError(t *testing.T, v string, contentLength int) { } func TestFSCompressConcurrent(t *testing.T) { + // This test can't run parallel as files in / might by changed by other tests. + fs := &FS{ Root: ".", GenerateIndexPages: true, @@ -465,7 +467,7 @@ func TestFSCompressConcurrent(t *testing.T) { } func TestFSCompressSingleThread(t *testing.T) { - t.Parallel() + // This test can't run parallel as files in / might by changed by other tests. fs := &FS{ Root: ".", @@ -525,7 +527,7 @@ func testFSCompress(t *testing.T, h RequestHandler, filePath string) { t.Fatalf("unexpected error when gunzipping response body: %s. filePath=%q", err, filePath) } if string(zbody) != body { - t.Fatalf("unexpected body %q. Expected %q. FilePath=%q", zbody, body, filePath) + t.Fatalf("unexpected body len=%d. Expected len=%d. FilePath=%q", len(zbody), len(body), filePath) } } diff --git a/http.go b/http.go index d10c9b1..ddec400 100644 --- a/http.go +++ b/http.go @@ -704,7 +704,7 @@ func (req *Request) parseURI() { } req.parsedURI = true - req.uri.parseQuick(req.Header.RequestURI(), &req.Header, req.isTLS) + req.uri.parse(req.Header.Host(), req.Header.RequestURI(), req.isTLS) } // PostArgs returns POST arguments. diff --git a/uri.go b/uri.go index 913c86b..1b78416 100644 --- a/uri.go +++ b/uri.go @@ -62,8 +62,6 @@ type URI struct { username []byte password []byte - - h *RequestHeader } // CopyTo copies uri contents to dst. @@ -84,7 +82,6 @@ func (u *URI) CopyTo(dst *URI) { // fullURI and requestURI shouldn't be copied, since they are created // from scratch on each FullURI() and RequestURI() call. - dst.h = u.h } // Hash returns URI hash, i.e. qwe of http://aaa.com/foo/bar?baz=123#qwe . @@ -232,19 +229,12 @@ func (u *URI) Reset() { // There is no need in u.requestURI = u.requestURI[:0], since requestURI // is calculated on each call to RequestURI(). - - u.h = nil } // Host returns host part, i.e. aaa.com of http://aaa.com/foo/bar?baz=123#qwe . // // Host is always lowercased. func (u *URI) Host() []byte { - if len(u.host) == 0 && u.h != nil { - u.host = append(u.host[:0], u.h.Host()...) - lowercaseBytes(u.host) - u.h = nil - } return u.host } @@ -267,23 +257,18 @@ func (u *URI) SetHostBytes(host []byte) { // // uri may contain e.g. RequestURI without scheme and host if host is non-empty. func (u *URI) Parse(host, uri []byte) { - u.parse(host, uri, nil) + u.parse(host, uri, false) } -func (u *URI) parseQuick(uri []byte, h *RequestHeader, isTLS bool) { - u.parse(nil, uri, h) - if isTLS { - u.scheme = append(u.scheme[:0], strHTTPS...) - } -} - -func (u *URI) parse(host, uri []byte, h *RequestHeader) { +func (u *URI) parse(host, uri []byte, isTLS bool) { u.Reset() - u.h = h scheme, host, uri := splitHostURI(host, uri) u.scheme = append(u.scheme, scheme...) lowercaseBytes(u.scheme) + if isTLS { + u.scheme = append(u.scheme[:0], strHTTPS...) + } if n := bytes.Index(host, strAt); n >= 0 { auth := host[:n]