From 6dc7cf5f29678ea50142c0c8f28d15995ca86e82 Mon Sep 17 00:00:00 2001 From: Lawrence Gripper Date: Mon, 22 Oct 2018 11:24:21 +0100 Subject: [PATCH 1/4] Ignore httpkeepalive in leaktest --- leaktest.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/leaktest.go b/leaktest.go index 6b3f12a..219e930 100644 --- a/leaktest.go +++ b/leaktest.go @@ -41,6 +41,9 @@ func interestingGoroutine(g string) (*goroutine, error) { } if stack == "" || + // Ignore HTTP keep alives + strings.Contains(stack, ").readLoop(") || + strings.Contains(stack, ").writeLoop(") || // Below are the stacks ignored by the upstream leaktest code. strings.Contains(stack, "testing.Main(") || strings.Contains(stack, "testing.(*T).Run(") || From 8c9695437a05417673183c906fe396c69af84293 Mon Sep 17 00:00:00 2001 From: Lawrence Gripper Date: Tue, 23 Oct 2018 13:44:09 +0100 Subject: [PATCH 2/4] Added tests for keep alive --- .travis.yml | 2 +- leaktest_test.go | 153 +++++++++++++++++++++++++++-------------- leaktest_utils_test.go | 42 +++++++++++ 3 files changed, 145 insertions(+), 52 deletions(-) create mode 100644 leaktest_utils_test.go diff --git a/.travis.yml b/.travis.yml index bc45a51..a9bc32f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,7 +8,7 @@ go: - tip script: - - go test -v -race -parallel 5 -coverprofile=coverage.txt -covermode=atomic ./ + - go test -v -race -coverprofile=coverage.txt -covermode=atomic ./ - go test github.com/fortytw2/leaktest -run ^TestEmptyLeak$ before_install: diff --git a/leaktest_test.go b/leaktest_test.go index d6c70cd..da19e6d 100644 --- a/leaktest_test.go +++ b/leaktest_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net/http" "sync" "testing" "time" @@ -19,60 +20,106 @@ func (tr *testReporter) Errorf(format string, args ...interface{}) { tr.msg = fmt.Sprintf(format, args...) } -var leakyFuncs = []func(){ - // Infinite for loop - func() { - for { - time.Sleep(time.Second) - } - }, - // Select on a channel not referenced by other goroutines. - func() { - c := make(chan struct{}) - <-c - }, - // Blocked select on channels not referenced by other goroutines. - func() { - c := make(chan struct{}) - c2 := make(chan struct{}) - select { - case <-c: - case c2 <- struct{}{}: - } - }, - // Blocking wait on sync.Mutex that isn't referenced by other goroutines. - func() { - var mu sync.Mutex - mu.Lock() - mu.Lock() - }, - // Blocking wait on sync.RWMutex that isn't referenced by other goroutines. - func() { - var mu sync.RWMutex - mu.RLock() - mu.Lock() - }, - func() { - var mu sync.Mutex - mu.Lock() - c := sync.NewCond(&mu) - c.Wait() - }, -} - func TestCheck(t *testing.T) { + leakyFuncs := []struct { + f func() + name string + expectLeak bool + }{ + { + name: "Infinite for loop", + expectLeak: true, + f: func() { + for { + time.Sleep(time.Second) + } + }, + }, + { + name: "Select on a channel not referenced by other goroutines.", + expectLeak: true, + f: func() { + c := make(chan struct{}) + <-c + }, + }, + { + name: "Blocked select on channels not referenced by other goroutines.", + expectLeak: true, + f: func() { + c := make(chan struct{}) + c2 := make(chan struct{}) + select { + case <-c: + case c2 <- struct{}{}: + } + }, + }, + { + name: "Blocking wait on sync.Mutex that isn't referenced by other goroutines.", + expectLeak: true, + f: func() { + var mu sync.Mutex + mu.Lock() + mu.Lock() + }, + }, + { + name: "Blocking wait on sync.RWMutex that isn't referenced by other goroutines.", + expectLeak: true, + f: func() { + var mu sync.RWMutex + mu.RLock() + mu.Lock() + }, + }, + { + name: "HTTP Client with KeepAlive Disabled.", + expectLeak: false, + f: func() { + http.DefaultTransport.(*http.Transport).DisableKeepAlives = true + http.Get("http://localhost:8091") + }, + }, + { + name: "HTTP Client with KeepAlive Enabled.", + expectLeak: true, + f: func() { + http.DefaultTransport.(*http.Transport).DisableKeepAlives = false + + _, err := http.Get("http://localhost:8091") + if err != nil { + t.Error(err) + } + + }, + }, + } + + // Start our keep alive server for keep alive tests + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go startKeepAliveEnabledServer(ctx) + // this works because the running goroutine is left running at the // start of the next test case - so the previous leaks don't affect the // check for the next one - for i, fn := range leakyFuncs { - checker := &testReporter{} - snapshot := CheckTimeout(checker, time.Second) - go fn() - - snapshot() - if !checker.failed { - t.Errorf("didn't catch sleeping goroutine, test #%d", i) - } + for _, leakyTestcase := range leakyFuncs { + + t.Run(leakyTestcase.name, func(t *testing.T) { + checker := &testReporter{} + snapshot := CheckTimeout(checker, time.Second) + go leakyTestcase.f() + + snapshot() + + if !checker.failed && leakyTestcase.expectLeak { + t.Error("didn't catch sleeping goroutine") + } + if checker.failed && !leakyTestcase.expectLeak { + t.Error("got leak but didn't expect it") + } + }) } } @@ -80,7 +127,7 @@ func TestCheck(t *testing.T) { // be based on time after the test finishes rather than time after the test's // start. func TestSlowTest(t *testing.T) { - defer CheckTimeout(t, 1000 * time.Millisecond)() + defer CheckTimeout(t, 1000*time.Millisecond)() go time.Sleep(1500 * time.Millisecond) time.Sleep(750 * time.Millisecond) @@ -133,6 +180,10 @@ func TestInterestingGoroutine(t *testing.T) { stack: "goroutine 123 [running]:", err: errors.New(`error parsing stack: "goroutine 123 [running]:"`), }, + { + stack: "goroutine 299 [IO wait]:\nnet/http.(*persistConn).readLoop(0xc420556240)", + err: nil, + }, { stack: "goroutine 123 [running]:\ntesting.RunTests", err: nil, diff --git a/leaktest_utils_test.go b/leaktest_utils_test.go new file mode 100644 index 0000000..77a1d1f --- /dev/null +++ b/leaktest_utils_test.go @@ -0,0 +1,42 @@ +package leaktest + +import ( + "context" + "log" + "net/http" + "time" +) + +func index() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) +} + +func startKeepAliveEnabledServer(ctx context.Context) { + router := http.NewServeMux() + router.Handle("/", index()) + + server := &http.Server{ + Addr: ":8091", + ReadTimeout: 5 * time.Second, + WriteTimeout: 10 * time.Second, + IdleTimeout: 15 * time.Second, + } + + go func() { + <-ctx.Done() + + server.SetKeepAlivesEnabled(false) + if err := server.Shutdown(ctx); err != nil { + log.Fatalf("Could not gracefully shutdown the server: %v\n", err) + } + }() + + log.Println("Server is ready to handle requests at", server.Addr) + if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { + log.Fatalf("Could not listen on %s: %v\n", server.Addr, err) + } + + log.Println("Server stopped") +} From ee806af359869e555f1e6a0e68316235944e629e Mon Sep 17 00:00:00 2001 From: Lawrence Gripper Date: Tue, 23 Oct 2018 14:13:47 +0100 Subject: [PATCH 3/4] Fix race condition --- .travis.yml | 3 +-- leaktest_test.go | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/.travis.yml b/.travis.yml index a9bc32f..5a791be 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,5 @@ language: go go: - - 1.7 - 1.8 - 1.9 - "1.10" @@ -8,7 +7,7 @@ go: - tip script: - - go test -v -race -coverprofile=coverage.txt -covermode=atomic ./ + - go test -v -race -parallel 5 -coverprofile=coverage.txt -covermode=atomic ./ - go test github.com/fortytw2/leaktest -run ^TestEmptyLeak$ before_install: diff --git a/leaktest_test.go b/leaktest_test.go index da19e6d..7ffbffc 100644 --- a/leaktest_test.go +++ b/leaktest_test.go @@ -77,21 +77,28 @@ func TestCheck(t *testing.T) { name: "HTTP Client with KeepAlive Disabled.", expectLeak: false, f: func() { - http.DefaultTransport.(*http.Transport).DisableKeepAlives = true - http.Get("http://localhost:8091") + tr := &http.Transport{ + DisableKeepAlives: true, + } + client := &http.Client{Transport: tr} + _, err := client.Get("http://localhost:8091") + if err != nil { + t.Error(err) + } }, }, { name: "HTTP Client with KeepAlive Enabled.", expectLeak: true, f: func() { - http.DefaultTransport.(*http.Transport).DisableKeepAlives = false - - _, err := http.Get("http://localhost:8091") + tr := &http.Transport{ + DisableKeepAlives: false, + } + client := &http.Client{Transport: tr} + _, err := client.Get("http://localhost:8091") if err != nil { t.Error(err) } - }, }, } From c38c66b6616763e8b8eba0c86e93ef011be06dce Mon Sep 17 00:00:00 2001 From: Lawrence Gripper Date: Tue, 6 Nov 2018 16:12:39 +0000 Subject: [PATCH 4/4] Fix feedback - move to using httptest for server. --- leaktest_test.go | 12 ++++++++---- leaktest_utils_test.go | 32 ++++++++++---------------------- 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/leaktest_test.go b/leaktest_test.go index 7ffbffc..6cecb0b 100644 --- a/leaktest_test.go +++ b/leaktest_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "net/http/httptest" "sync" "testing" "time" @@ -20,6 +21,9 @@ func (tr *testReporter) Errorf(format string, args ...interface{}) { tr.msg = fmt.Sprintf(format, args...) } +// Client for the TestServer +var testServer *httptest.Server + func TestCheck(t *testing.T) { leakyFuncs := []struct { f func() @@ -81,7 +85,7 @@ func TestCheck(t *testing.T) { DisableKeepAlives: true, } client := &http.Client{Transport: tr} - _, err := client.Get("http://localhost:8091") + _, err := client.Get(testServer.URL) if err != nil { t.Error(err) } @@ -95,7 +99,7 @@ func TestCheck(t *testing.T) { DisableKeepAlives: false, } client := &http.Client{Transport: tr} - _, err := client.Get("http://localhost:8091") + _, err := client.Get(testServer.URL) if err != nil { t.Error(err) } @@ -106,7 +110,7 @@ func TestCheck(t *testing.T) { // Start our keep alive server for keep alive tests ctx, cancel := context.WithCancel(context.Background()) defer cancel() - go startKeepAliveEnabledServer(ctx) + testServer = startKeepAliveEnabledServer(ctx) // this works because the running goroutine is left running at the // start of the next test case - so the previous leaks don't affect the @@ -134,7 +138,7 @@ func TestCheck(t *testing.T) { // be based on time after the test finishes rather than time after the test's // start. func TestSlowTest(t *testing.T) { - defer CheckTimeout(t, 1000*time.Millisecond)() + defer CheckTimeout(t, 1000 * time.Millisecond)() go time.Sleep(1500 * time.Millisecond) time.Sleep(750 * time.Millisecond) diff --git a/leaktest_utils_test.go b/leaktest_utils_test.go index 77a1d1f..78dccbc 100644 --- a/leaktest_utils_test.go +++ b/leaktest_utils_test.go @@ -2,8 +2,8 @@ package leaktest import ( "context" - "log" "net/http" + "net/http/httptest" "time" ) @@ -13,30 +13,18 @@ func index() http.Handler { }) } -func startKeepAliveEnabledServer(ctx context.Context) { - router := http.NewServeMux() - router.Handle("/", index()) - - server := &http.Server{ - Addr: ":8091", - ReadTimeout: 5 * time.Second, - WriteTimeout: 10 * time.Second, - IdleTimeout: 15 * time.Second, - } +func startKeepAliveEnabledServer(ctx context.Context) *httptest.Server { + server := httptest.NewUnstartedServer(index()) + server.Config.ReadTimeout = 5 * time.Second + server.Config.WriteTimeout = 10 * time.Second + server.Config.IdleTimeout = 15 * time.Second + server.Config.SetKeepAlivesEnabled(true) + server.Start() go func() { <-ctx.Done() - - server.SetKeepAlivesEnabled(false) - if err := server.Shutdown(ctx); err != nil { - log.Fatalf("Could not gracefully shutdown the server: %v\n", err) - } + server.Close() }() - log.Println("Server is ready to handle requests at", server.Addr) - if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { - log.Fatalf("Could not listen on %s: %v\n", server.Addr, err) - } - - log.Println("Server stopped") + return server }