Fixes goroutine leaks in tests (#2169)

Adds main_test.go with noleak.CheckMain(m) into more packages and fixes leaks in tests. Signed-off-by: Alexander Yastrebov <[email protected]> Signed-off-by: Alexander Yastrebov <[email protected]>
zalando · Dec 19, 2022 · 211b0ec · 211b0ec
1 parent fadff2e
commit 211b0ec
Show file tree

Hide file tree

Showing 28 changed files with 365 additions and 117 deletions.
diff --git a/eskipfile/main_test.go b/eskipfile/main_test.go
@@ -0,0 +1,12 @@
+package eskipfile
+
+import (
+	"os"
+	"testing"
+
+	"github.com/AlexanderYastrebov/noleak"
+)
+
+func TestMain(m *testing.M) {
+	os.Exit(noleak.CheckMain(m))
+}
diff --git a/eskipfile/remote.go b/eskipfile/remote.go
@@ -69,6 +69,7 @@ func RemoteWatch(o *RemoteWatchOptions) (routing.DataClient, error) {
 		err = dataClient.DownloadRemoteFile()
 
 		if err != nil {
+			dataClient.http.Close()
 			return nil, err
 		}
 	} else {
@@ -78,6 +79,7 @@ func RemoteWatch(o *RemoteWatchOptions) (routing.DataClient, error) {
 		}
 
 		if err != nil {
+			dataClient.http.Close()
 			return nil, err
 		}
 		dataClient.preloaded = true

diff --git a/eskipfile/remote_test.go b/eskipfile/remote_test.go
@@ -141,6 +141,10 @@ func TestLoadAllAndUpdate(t *testing.T) {
 
 			options := &RemoteWatchOptions{RemoteFile: testValidServer.URL, Threshold: 10, Verbose: true, FailOnStartup: true}
 			client, err := RemoteWatch(options)
+			if err == nil {
+				defer client.(*remoteEskipFile).Close()
+			}
+
 			if err == nil && test.fail {
 				t.Error("failed to fail")
 				return
@@ -175,7 +179,11 @@ func TestHTTPTimeout(t *testing.T) {
 		time.Sleep(2 * time.Second)
 	}))
 	defer server.Close()
-	_, err := RemoteWatch(&RemoteWatchOptions{RemoteFile: server.URL, HTTPTimeout: 1 * time.Second, FailOnStartup: true})
+	client, err := RemoteWatch(&RemoteWatchOptions{RemoteFile: server.URL, HTTPTimeout: 1 * time.Second, FailOnStartup: true})
+	if err == nil {
+		defer client.(*remoteEskipFile).Close()
+	}
+
 	if err, ok := err.(net.Error); !ok || !err.Timeout() {
 		t.Errorf("got %v, expected net.Error with timeout", err)
 	}

diff --git a/eskipfile/watch.go b/eskipfile/watch.go
@@ -145,15 +145,23 @@ func (c *WatchClient) watch() {
 // LoadAll returns the parsed route definitions found in the file.
 func (c *WatchClient) LoadAll() ([]*eskip.Route, error) {
 	req := make(chan watchResponse)
-	c.getAll <- req
+	select {
+	case c.getAll <- req:
+	case <-c.quit:
+		return nil, nil
+	}
 	rsp := <-req
 	return rsp.routes, rsp.err
 }
 
 // LoadUpdate returns differential updates when a watched file has changed.
 func (c *WatchClient) LoadUpdate() ([]*eskip.Route, []string, error) {
 	req := make(chan watchResponse)
-	c.getUpdates <- req
+	select {
+	case c.getUpdates <- req:
+	case <-c.quit:
+		return nil, nil, nil
+	}
 	rsp := <-req
 	return rsp.routes, rsp.deletedIDs, rsp.err
 }

diff --git a/filters/builtin/compress_test.go b/filters/builtin/compress_test.go
@@ -5,7 +5,6 @@ import (
 	"compress/flate"
 	"compress/gzip"
 	"errors"
-	"github.com/andybalholm/brotli"
 	"io"
 	"math/rand"
 	"net/http"
@@ -19,6 +18,8 @@ import (
 	"github.com/zalando/skipper/filters"
 	"github.com/zalando/skipper/filters/filtertest"
 	"github.com/zalando/skipper/proxy/proxytest"
+
+	"github.com/andybalholm/brotli"
 )
 
 const (
@@ -566,6 +567,16 @@ func TestForwardError(t *testing.T) {
 	}
 }
 
+type readCloser struct {
+	io.Reader
+	done chan struct{}
+}
+
+func (h *readCloser) Close() error {
+	close(h.done)
+	return nil
+}
+
 func TestCompressWithEncodings(t *testing.T) {
 	spec, err := NewCompressWithOptions(CompressOptions{Encodings: []string{"br", "gzip"}})
 	if err != nil {
@@ -576,15 +587,21 @@ func TestCompressWithEncodings(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	done := make(chan struct{})
+
 	req := &http.Request{Header: http.Header{"Accept-Encoding": []string{"gzip,br,deflate"}}}
-	body := io.NopCloser(&io.LimitedReader{R: rand.New(rand.NewSource(0)), N: 100})
+	body := &readCloser{Reader: &io.LimitedReader{R: rand.New(rand.NewSource(0)), N: 100}, done: done}
 	rsp := &http.Response{
 		Header: http.Header{"Content-Type": []string{"application/octet-stream"}},
-		Body:   body}
+		Body:   body,
+	}
 
 	ctx := &filtertest.Context{FRequest: req, FResponse: rsp}
 	f.Response(ctx)
 
+	io.Copy(io.Discard, rsp.Body)
+	<-done
+
 	enc := rsp.Header.Get("Content-Encoding")
 	if enc != "br" {
 		t.Error("unexpected value", enc)

diff --git a/filters/builtin/creationmetrics_test.go b/filters/builtin/creationmetrics_test.go
@@ -265,6 +265,7 @@ func TestRouteCreationMetricsMarkerDropped(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
+			defer dc.Close()
 
 			filtersBefore := make(map[string]bool)
 			er, err := dc.LoadAll()
@@ -284,6 +285,7 @@ func TestRouteCreationMetricsMarkerDropped(t *testing.T) {
 				PostProcessors:  []routing.PostProcessor{NewRouteCreationMetrics(&metricstest.MockMetrics{})},
 				SignalFirstLoad: true,
 			})
+			defer rt.Close()
 
 			<-rt.FirstLoad()
 			r, _ := rt.Route(&http.Request{URL: &url.URL{Path: "/"}})

diff --git a/filters/builtin/header_test.go b/filters/builtin/header_test.go
@@ -1,14 +1,15 @@
 package builtin
 
 import (
-	"github.com/zalando/skipper/eskip"
-	"github.com/zalando/skipper/filters"
-	"github.com/zalando/skipper/filters/filtertest"
-	"github.com/zalando/skipper/proxy/proxytest"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
+
+	"github.com/zalando/skipper/eskip"
+	"github.com/zalando/skipper/filters"
+	"github.com/zalando/skipper/filters/filtertest"
+	"github.com/zalando/skipper/proxy/proxytest"
 )
 
 type testContext struct {
@@ -522,6 +523,7 @@ func TestHeader(t *testing.T) {
 						t.Error(err)
 						return
 					}
+					defer rsp.Body.Close()
 
 					if ti.valid && rsp.StatusCode != http.StatusOK ||
 						!ti.valid && rsp.StatusCode != http.StatusNotFound {

diff --git a/filters/builtin/main_test.go b/filters/builtin/main_test.go
@@ -0,0 +1,12 @@
+package builtin
+
+import (
+	"os"
+	"testing"
+
+	"github.com/AlexanderYastrebov/noleak"
+)
+
+func TestMain(m *testing.M) {
+	os.Exit(noleak.CheckMain(m))
+}
diff --git a/filters/builtin/redirect_test.go b/filters/builtin/redirect_test.go
@@ -1,6 +1,7 @@
 package builtin
 
 import (
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -123,54 +124,56 @@ func TestRedirect(t *testing.T) {
 			"not deprecated",
 			filters.RedirectToName,
 		}} {
-			var args []interface{}
-			if ti.skipLocationArg {
-				args = []interface{}{float64(ti.code)}
-			} else {
-				args = []interface{}{float64(ti.code), ti.filterLocation}
-			}
-			dc := testdataclient.New([]*eskip.Route{{
-				Shunt: true,
-				Filters: []*eskip.Filter{{
-					Name: tii.name,
-					Args: args}}}})
-			tl := loggingtest.New()
-			rt := routing.New(routing.Options{
-				FilterRegistry: MakeRegistry(),
-				DataClients:    []routing.DataClient{dc},
-				Log:            tl})
-			p := proxy.WithParams(proxy.Params{
-				Routing: rt,
-			})
+			t.Run(fmt.Sprintf("%s/%s", ti.msg, tii.name), func(t *testing.T) {
+				var args []interface{}
+				if ti.skipLocationArg {
+					args = []interface{}{float64(ti.code)}
+				} else {
+					args = []interface{}{float64(ti.code), ti.filterLocation}
+				}
+				dc := testdataclient.New([]*eskip.Route{{
+					Shunt: true,
+					Filters: []*eskip.Filter{{
+						Name: tii.name,
+						Args: args}}},
+				})
+				defer dc.Close()
+
+				tl := loggingtest.New()
+				defer tl.Close()
+
+				rt := routing.New(routing.Options{
+					FilterRegistry: MakeRegistry(),
+					DataClients:    []routing.DataClient{dc},
+					Log:            tl,
+				})
+				defer rt.Close()
+
+				p := proxy.WithParams(proxy.Params{
+					Routing: rt,
+				})
+				defer p.Close()
 
-			closeAll := func() {
-				p.Close()
-				rt.Close()
-				tl.Close()
-			}
-
-			// pick up routing
-			if err := tl.WaitFor("route settings applied", time.Second); err != nil {
-				t.Error(err)
-				closeAll()
-				continue
-			}
-
-			req := &http.Request{
-				URL:  &url.URL{Path: "/some/path", RawQuery: "foo=1&bar=2"},
-				Host: "incoming.example.org"}
-			w := httptest.NewRecorder()
-			p.ServeHTTP(w, req)
-
-			if w.Code != ti.code {
-				t.Error(ti.msg, tii.msg, "invalid status code", w.Code)
-			}
-
-			if w.Header().Get("Location") != ti.checkLocation {
-				t.Error(ti.msg, tii.msg, "invalid location", w.Header().Get("Location"))
-			}
-
-			closeAll()
+				// pick up routing
+				if err := tl.WaitFor("route settings applied", time.Second); err != nil {
+					t.Error(err)
+					return
+				}
+
+				req := &http.Request{
+					URL:  &url.URL{Path: "/some/path", RawQuery: "foo=1&bar=2"},
+					Host: "incoming.example.org"}
+				w := httptest.NewRecorder()
+				p.ServeHTTP(w, req)
+
+				if w.Code != ti.code {
+					t.Error("invalid status code", w.Code)
+				}
+
+				if w.Header().Get("Location") != ti.checkLocation {
+					t.Error("invalid location", w.Header().Get("Location"))
+				}
+			})
 		}
 	}
 }
@@ -209,47 +212,49 @@ func TestRedirectLower(t *testing.T) {
 			"lowercase",
 			filters.RedirectToLowerName,
 		}} {
-			dc := testdataclient.New([]*eskip.Route{{
-				Shunt: true,
-				Filters: []*eskip.Filter{{
-					Name: tii.name,
-					Args: []interface{}{float64(ti.code), ti.filterLocation}}}}})
-			tl := loggingtest.New()
-			rt := routing.New(routing.Options{
-				FilterRegistry: MakeRegistry(),
-				DataClients:    []routing.DataClient{dc},
-				Log:            tl})
-			p := proxy.WithParams(proxy.Params{
-				Routing: rt,
-			})
+			t.Run(fmt.Sprintf("%s/%s", ti.msg, tii.name), func(t *testing.T) {
+				dc := testdataclient.New([]*eskip.Route{{
+					Shunt: true,
+					Filters: []*eskip.Filter{{
+						Name: tii.name,
+						Args: []interface{}{float64(ti.code), ti.filterLocation}}}},
+				})
+				defer dc.Close()
+
+				tl := loggingtest.New()
+				defer tl.Close()
+
+				rt := routing.New(routing.Options{
+					FilterRegistry: MakeRegistry(),
+					DataClients:    []routing.DataClient{dc},
+					Log:            tl,
+				})
+				defer rt.Close()
+
+				p := proxy.WithParams(proxy.Params{
+					Routing: rt,
+				})
+				defer p.Close()
 
-			closeAll := func() {
-				p.Close()
-				rt.Close()
-				tl.Close()
-			}
-
-			if err := tl.WaitFor("route settings applied", time.Second); err != nil {
-				t.Error(err)
-				closeAll()
-				continue
-			}
-
-			req := &http.Request{
-				URL:  &url.URL{Path: "/some/path", RawQuery: "foo=1&bar=2"},
-				Host: "incoming.example.org"}
-			w := httptest.NewRecorder()
-			p.ServeHTTP(w, req)
-
-			if w.Code != ti.code {
-				t.Error(ti.msg, tii.msg, "invalid status code", w.Code)
-			}
-
-			if w.Header().Get("Location") != ti.checkLocation {
-				t.Error(ti.msg, tii.msg, "invalid location", w.Header().Get("Location"))
-			}
-
-			closeAll()
+				if err := tl.WaitFor("route settings applied", time.Second); err != nil {
+					t.Error(err)
+					return
+				}
+
+				req := &http.Request{
+					URL:  &url.URL{Path: "/some/path", RawQuery: "foo=1&bar=2"},
+					Host: "incoming.example.org"}
+				w := httptest.NewRecorder()
+				p.ServeHTTP(w, req)
+
+				if w.Code != ti.code {
+					t.Error("invalid status code", w.Code)
+				}
+
+				if w.Header().Get("Location") != ti.checkLocation {
+					t.Error("invalid location", w.Header().Get("Location"))
+				}
+			})
 		}
 	}
 }