From e1fe7350a2667079b34c2643fd74da11f70354e9 Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Sat, 19 Dec 2015 18:25:26 +0100 Subject: [PATCH 1/3] tests: update + enable check for leaked goroutines Go 1.4 landed a new testing.M type [1][1] which allows for start-up and shutdown hooks when running tests. The standard library now uses this for checking for leaked goroutines in net/http [2][2]. This patch essentially re-ports the updated code from the net/http test (we were using an older version of it) - in detail: - updates the test to use `TestMain` instead of relying on `TestGoroutinesRunning` to be implicitly run after all other tests - adds a few new goroutines to the list of exceptions (the test itself, as well as the golang/glog package and pkg/log.MergeLogger, both of which spin off goroutines to handle log flushing/merging respectively) - removes a couple of TODOs in the test for extra goroutines that's run after individual tests (one of these re-enables the http package's `.readLoop` and the other was an out-of-date TODO) - re-enables the test [1]: https://golang.org/pkg/testing/#M [2]: https://github.com/golang/go/blob/release-branch.go1.4/src/net/http/main_test.go#L18 --- integration/{z_last_test.go => main_test.go} | 45 +++++++++++--------- 1 file changed, 26 insertions(+), 19 deletions(-) rename integration/{z_last_test.go => main_test.go} (66%) diff --git a/integration/z_last_test.go b/integration/main_test.go similarity index 66% rename from integration/z_last_test.go rename to integration/main_test.go index 54d954930..8c8cbfa6d 100644 --- a/integration/z_last_test.go +++ b/integration/main_test.go @@ -5,7 +5,9 @@ package integration import ( + "fmt" "net/http" + "os" "runtime" "sort" "strings" @@ -26,6 +28,9 @@ func interestingGoroutines() (gs []string) { strings.Contains(stack, "created by testing.RunTests") || strings.Contains(stack, "testing.Main(") || strings.Contains(stack, "runtime.goexit") || + strings.Contains(stack, "github.com/coreos/etcd/integration.interestingGoroutines") || + strings.Contains(stack, "github.com/coreos/etcd/pkg/logutil.(*MergeLogger).outputLoop") || + strings.Contains(stack, "github.com/golang/glog.(*loggingT).flushDaemon") || strings.Contains(stack, "created by runtime.gc") || strings.Contains(stack, "runtime.MHeap_Scavenger") { continue @@ -37,11 +42,18 @@ func interestingGoroutines() (gs []string) { } // Verify the other tests didn't leave any goroutines running. -// This is in a file named z_last_test.go so it sorts at the end. -func TestGoroutinesRunning(t *testing.T) { - t.Skip("TODO: etcdserver.Sender may still dial closed remote endpoint and need some time to timeout.") +func TestMain(m *testing.M) { + v := m.Run() + if v == 0 && goroutineLeaked() { + os.Exit(1) + } + os.Exit(v) +} + +func goroutineLeaked() bool { if testing.Short() { - t.Skip("not counting goroutines for leakage in -short mode") + // not counting goroutines for leakage in -short mode + return false } gs := interestingGoroutines() @@ -52,13 +64,14 @@ func TestGoroutinesRunning(t *testing.T) { n++ } - t.Logf("num goroutines = %d", n) - if n > 0 { - t.Error("Too many goroutines.") - for stack, count := range stackCount { - t.Logf("%d instances of:\n%s", count, stack) - } + if n == 0 { + return false } + fmt.Fprintf(os.Stderr, "Too many goroutines running after integration test(s).\n") + for stack, count := range stackCount { + fmt.Fprintf(os.Stderr, "%d instances of:\n%s\n", count, stack) + } + return true } func afterTest(t *testing.T) { @@ -68,18 +81,12 @@ func afterTest(t *testing.T) { } var bad string badSubstring := map[string]string{ - // TODO: there might exist a bug in http package, which will leave - // readLoop without writeLoop after close all idle connections. - // comment this line until we have time to dig into it. - // ").readLoop(": "a Transport", + ").readLoop(": "a Transport", ").writeLoop(": "a Transport", "created by net/http/httptest.(*Server).Start": "an httptest.Server", "timeoutHandler": "a TimeoutHandler", - // TODO: dial goroutines leaks even if the request is cancelled. - // It needs to wait dial timeout to recycle the goroutine. - // comment this line until we have time to dig into it. - "net.(*netFD).connect(": "a timing out dial", - ").noteClientGone(": "a closenotifier sender", + "net.(*netFD).connect(": "a timing out dial", + ").noteClientGone(": "a closenotifier sender", } var stacks string for i := 0; i < 6; i++ { From d50fbe384a804e1f1c80fea766d55afbf06e56e3 Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Sat, 19 Dec 2015 23:05:46 +0100 Subject: [PATCH 2/3] tests: ignore leaked readLoop on go <1.5 --- integration/main_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/integration/main_test.go b/integration/main_test.go index 8c8cbfa6d..6d4cd5f4d 100644 --- a/integration/main_test.go +++ b/integration/main_test.go @@ -81,13 +81,22 @@ func afterTest(t *testing.T) { } var bad string badSubstring := map[string]string{ - ").readLoop(": "a Transport", ").writeLoop(": "a Transport", "created by net/http/httptest.(*Server).Start": "an httptest.Server", "timeoutHandler": "a TimeoutHandler", "net.(*netFD).connect(": "a timing out dial", ").noteClientGone(": "a closenotifier sender", } + + // readLoop was buggy before go1.5: + // https://github.com/golang/go/issues/10457 + var major, minor int + var discard string + i, err := fmt.Sscanf(runtime.Version(), "go%d.%d%s", &major, &minor, &discard) + if err == nil && i == 3 && (major > 1 || major == 1 && minor >= 5) { + badSubstring[").readLoop("] = "a Transport" + } + var stacks string for i := 0; i < 6; i++ { bad = "" From b126ff77fb43f2917b2e908c8f9c3611a14efaaf Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Sat, 19 Dec 2015 23:08:16 +0100 Subject: [PATCH 3/3] tests: only check for go1.5+ once --- integration/main_test.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/integration/main_test.go b/integration/main_test.go index 6d4cd5f4d..6fe9beb9b 100644 --- a/integration/main_test.go +++ b/integration/main_test.go @@ -15,6 +15,15 @@ import ( "time" ) +var atLeastGo15 bool = false + +func init() { + var major, minor int + var discard string + i, err := fmt.Sscanf(runtime.Version(), "go%d.%d%s", &major, &minor, &discard) + atLeastGo15 = (err == nil && i == 3 && (major > 1 || major == 1 && minor >= 5)) +} + func interestingGoroutines() (gs []string) { buf := make([]byte, 2<<20) buf = buf[:runtime.Stack(buf, true)] @@ -90,10 +99,7 @@ func afterTest(t *testing.T) { // readLoop was buggy before go1.5: // https://github.com/golang/go/issues/10457 - var major, minor int - var discard string - i, err := fmt.Sscanf(runtime.Version(), "go%d.%d%s", &major, &minor, &discard) - if err == nil && i == 3 && (major > 1 || major == 1 && minor >= 5) { + if atLeastGo15 { badSubstring[").readLoop("] = "a Transport" }