Skip to content

Commit

Permalink
Fix race condition in tests, fix URI bug
Browse files Browse the repository at this point in the history
- 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%
  • Loading branch information
erikdubbelboer committed Oct 19, 2019
1 parent c3d82ca commit d428e1b
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 25 deletions.
10 changes: 8 additions & 2 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
6 changes: 4 additions & 2 deletions fs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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: ".",
Expand Down Expand Up @@ -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)
}
}

Expand Down
2 changes: 1 addition & 1 deletion http.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
25 changes: 5 additions & 20 deletions uri.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ type URI struct {

username []byte
password []byte

h *RequestHeader
}

// CopyTo copies uri contents to dst.
Expand All @@ -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 .
Expand Down Expand Up @@ -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
}

Expand All @@ -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]
Expand Down

0 comments on commit d428e1b

Please sign in to comment.