test: use goleak to autodetect leaks

test that redistest does not leak goroutines fix: found goroutine leaks fix: wrap close in sync.once.Do fix: shallow copy can not copy by deref ptr in case it contains sync.Once refactor: *sync-Once to sync.Once as shown in stdlib Go example Signed-off-by: Sandor Szücs <[email protected]>
zalando · Nov 10, 2022 · debdabb · debdabb
1 parent c48eb46
commit debdabb
Show file tree

Hide file tree

Showing 20 changed files with 222 additions and 48 deletions.
diff --git a/dataclients/kubernetes/kube_test.go b/dataclients/kubernetes/kube_test.go
@@ -29,6 +29,7 @@ import (
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/goleak"
 
 	"github.com/zalando/skipper/dataclients/kubernetes/definitions"
 	"github.com/zalando/skipper/eskip"
@@ -645,6 +646,8 @@ func TestIngressClassFilter(t *testing.T) {
 }
 
 func TestIngress(t *testing.T) {
+	defer goleak.VerifyNone(t)
+
 	api := newTestAPI(t, nil, &definitions.IngressList{})
 	defer api.Close()
 

diff --git a/dataclients/kubernetes/main_test.go b/dataclients/kubernetes/main_test.go
@@ -0,0 +1,11 @@
+package kubernetes_test
+
+import (
+	"testing"
+
+	"go.uber.org/goleak"
+)
+
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
diff --git a/dataclients/kubernetes/routegroups_test.go b/dataclients/kubernetes/routegroups_test.go
@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/zalando/skipper/dataclients/kubernetes/kubernetestest"
+	"go.uber.org/goleak"
 )
 
 func TestRouteGroupExamples(t *testing.T) {
@@ -19,6 +20,7 @@ func TestRouteGroupClusterState(t *testing.T) {
 }
 
 func TestRouteGroupTraffic(t *testing.T) {
+	defer goleak.VerifyNone(t)
 	kubernetestest.FixturesToTest(t, "testdata/routegroups/traffic")
 }
 
@@ -27,6 +29,7 @@ func TestRouteGroupEastWest(t *testing.T) {
 }
 
 func TestRouteGroupEastWestRange(t *testing.T) {
+	defer goleak.VerifyNone(t)
 	kubernetestest.FixturesToTest(t, "testdata/routegroups/east-west-range")
 }
 
@@ -35,14 +38,17 @@ func TestRouteGroupHTTPSRedirect(t *testing.T) {
 }
 
 func TestRouteGroupDefaultFilters(t *testing.T) {
+	defer goleak.VerifyNone(t)
 	kubernetestest.FixturesToTest(t, "testdata/routegroups/default-filters")
 }
 
 func TestRouteGroupWithIngress(t *testing.T) {
+	defer goleak.VerifyNone(t)
 	kubernetestest.FixturesToTest(t, "testdata/routegroups/with-ingress")
 }
 
 func TestRouteGroupTracingTag(t *testing.T) {
+	defer goleak.VerifyNone(t)
 	kubernetestest.FixturesToTest(t, "testdata/routegroups/tracing-tag")
 }
 

diff --git a/eskip/eskip_test.go b/eskip/eskip_test.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/sanity-io/litter"
+	"go.uber.org/goleak"
 )
 
 func checkItems(t *testing.T, message string, l, lenExpected int, checkItem func(int) bool) bool {
@@ -172,6 +173,8 @@ func TestParseRouteExpression(t *testing.T) {
 		false,
 	}} {
 		t.Run(ti.msg, func(t *testing.T) {
+			defer goleak.VerifyNone(t)
+
 			stringMapKeys := func(m map[string]string) []string {
 				keys := make([]string, 0, len(m))
 				for k := range m {
@@ -323,13 +326,17 @@ func TestParseFilters(t *testing.T) {
 		[]*Filter{{Name: "filter1", Args: []interface{}{3.14}}, {Name: "filter2", Args: []interface{}{"key", float64(42)}}},
 		false,
 	}} {
-		fs, err := ParseFilters(ti.expression)
-		if err == nil && ti.err || err != nil && !ti.err {
-			t.Error(ti.msg, "failure case", err, ti.err)
-			return
-		}
+		t.Run(ti.msg, func(t *testing.T) {
+			defer goleak.VerifyNone(t)
+
+			fs, err := ParseFilters(ti.expression)
+			if err == nil && ti.err || err != nil && !ti.err {
+				t.Error(ti.msg, "failure case", err, ti.err)
+				return
+			}
 
-		checkFilters(t, ti.msg, fs, ti.check)
+			checkFilters(t, ti.msg, fs, ti.check)
+		})
 	}
 }
 
@@ -362,6 +369,8 @@ func TestPredicateParsing(t *testing.T) {
 		input: `*`,
 	}} {
 		t.Run(test.title, func(t *testing.T) {
+			defer goleak.VerifyNone(t)
+
 			p, err := ParsePredicates(test.input)
 
 			if err == nil && test.fail {
@@ -382,6 +391,8 @@ func TestPredicateParsing(t *testing.T) {
 }
 
 func TestClone(t *testing.T) {
+	defer goleak.VerifyNone(t)
+
 	r := &Route{
 		Id:            "foo",
 		Path:          "/bar",
@@ -493,6 +504,8 @@ func TestDefaultFiltersDo(t *testing.T) {
 			want:   outputPrependAppend2,
 		}} {
 		t.Run(tt.name, func(t *testing.T) {
+			defer goleak.VerifyNone(t)
+
 			if got := tt.df.Do(tt.routes); !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("Want %v, got %v", tt.want, got)
 			}
@@ -502,6 +515,8 @@ func TestDefaultFiltersDo(t *testing.T) {
 }
 
 func TestDefaultFiltersDoCorrectPrependFilters(t *testing.T) {
+	defer goleak.VerifyNone(t)
+
 	filters, err := ParseFilters("status(1) -> status(2) -> status(3)")
 	if err != nil {
 		t.Errorf("Failed to parse filter: %v", err)
@@ -597,6 +612,7 @@ func TestEditorPreProcessor(t *testing.T) {
 			want:   `r1_filter: Source("1.2.3.4/26") -> normalRequestLatency("100ms", "10ms") -> status(201) -> <shunt>`,
 		}} {
 		t.Run(tt.name, func(t *testing.T) {
+			defer goleak.VerifyNone(t)
 			routes, err := Parse(tt.routes)
 			if err != nil {
 				t.Errorf("Failed to parse route: %v", err)
@@ -634,7 +650,7 @@ func TestClonePreProcessor(t *testing.T) {
 				repl: "HostAny($1)",
 			},
 			routes: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>;`,
-			want: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>; 
+			want: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>;
 			clone_r0: HostAny("www[.]example[.]org") -> status(201) -> <shunt>;`,
 		},
 		{
@@ -653,7 +669,7 @@ func TestClonePreProcessor(t *testing.T) {
 				repl: "ClientIP($1)",
 			},
 			routes: `r1: Source("1.2.3.4/26") -> status(201) -> <shunt>;`,
-			want: `r1: Source("1.2.3.4/26") -> status(201) -> <shunt>; 
+			want: `r1: Source("1.2.3.4/26") -> status(201) -> <shunt>;
 			clone_r1: ClientIP("1.2.3.4/26") -> status(201) -> <shunt>;`,
 		},
 		{
@@ -662,11 +678,11 @@ func TestClonePreProcessor(t *testing.T) {
 				reg:  regexp.MustCompile("Source[(](.*)[)]"),
 				repl: "ClientIP($1)",
 			},
-			routes: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>; 
+			routes: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>;
 			r1: Source("1.2.3.4/26") -> status(201) -> <shunt>;`,
 
-			want: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>; 
-			r1: Source("1.2.3.4/26") -> status(201) -> <shunt>; 
+			want: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>;
+			r1: Source("1.2.3.4/26") -> status(201) -> <shunt>;
 			clone_r1: ClientIP("1.2.3.4/26") -> status(201) -> <shunt>;`,
 		},
 		{
@@ -676,7 +692,7 @@ func TestClonePreProcessor(t *testing.T) {
 				repl: "ClientIP($1)",
 			},
 			routes: `rn: Source("1.2.3.4/26", "10.5.5.0/24") -> status(201) -> <shunt>;`,
-			want: `rn: Source("1.2.3.4/26", "10.5.5.0/24") -> status(201) -> <shunt>; 
+			want: `rn: Source("1.2.3.4/26", "10.5.5.0/24") -> status(201) -> <shunt>;
 			clone_rn: ClientIP("1.2.3.4/26", "10.5.5.0/24") -> status(201) -> <shunt>;`,
 		},
 		{
@@ -687,8 +703,8 @@ func TestClonePreProcessor(t *testing.T) {
 			},
 			routes: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>;
 			rn: Source("1.2.3.4/26", "10.5.5.0/24") -> status(201) -> <shunt>;`,
-			want: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>; 
-			rn: Source("1.2.3.4/26", "10.5.5.0/24") -> status(201) -> <shunt>; 
+			want: `r0: Host("www[.]example[.]org") -> status(201) -> <shunt>;
+			rn: Source("1.2.3.4/26", "10.5.5.0/24") -> status(201) -> <shunt>;
 			clone_rn: ClientIP("1.2.3.4/26", "10.5.5.0/24") -> status(201) -> <shunt>;`,
 		},
 		{
@@ -698,10 +714,11 @@ func TestClonePreProcessor(t *testing.T) {
 				repl: "normalRequestLatency($1)",
 			},
 			routes: `r1_filter: Source("1.2.3.4/26") -> uniformRequestLatency("100ms", "10ms") -> status(201) -> <shunt>;`,
-			want: `r1_filter: Source("1.2.3.4/26") -> uniformRequestLatency("100ms", "10ms") -> status(201) -> <shunt>; 
+			want: `r1_filter: Source("1.2.3.4/26") -> uniformRequestLatency("100ms", "10ms") -> status(201) -> <shunt>;
 			clone_r1_filter: Source("1.2.3.4/26") -> normalRequestLatency("100ms", "10ms") -> status(201) -> <shunt>;`,
 		}} {
 		t.Run(tt.name, func(t *testing.T) {
+			defer goleak.VerifyNone(t)
 			routes, err := Parse(tt.routes)
 			if err != nil {
 				t.Errorf("Failed to parse route: %v", err)

diff --git a/eskipfile/remote.go b/eskipfile/remote.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"os"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/zalando/skipper/eskip"
@@ -15,6 +16,7 @@ import (
 )
 
 type remoteEskipFile struct {
+	once            sync.Once
 	preloaded       bool
 	remotePath      string
 	localPath       string
@@ -55,6 +57,7 @@ func RemoteWatch(o *RemoteWatchOptions) (routing.DataClient, error) {
 	}
 
 	dataClient := &remoteEskipFile{
+		once:       sync.Once{},
 		remotePath: o.RemoteFile,
 		localPath:  tempFilename.Name(),
 		threshold:  o.Threshold,
@@ -135,6 +138,19 @@ func (client *remoteEskipFile) LoadUpdate() ([]*eskip.Route, []string, error) {
 	return newRoutes, deletedRoutes, err
 }
 
+func (client *remoteEskipFile) Close() {
+	if client != nil {
+		client.once.Do(func() {
+			if client.http != nil {
+				client.http.Close()
+			}
+			if client.eskipFileClient != nil {
+				client.eskipFileClient.Close()
+			}
+		})
+	}
+}
+
 func isFileRemote(remotePath string) bool {
 	return strings.HasPrefix(remotePath, "http://") || strings.HasPrefix(remotePath, "https://")
 }

diff --git a/eskipfile/remote_test.go b/eskipfile/remote_test.go
@@ -91,6 +91,13 @@ func TestLoadAll(t *testing.T) {
 		t.Run(test.title, func(t *testing.T) {
 			options := &RemoteWatchOptions{RemoteFile: s.URL, Threshold: 10, Verbose: true, FailOnStartup: true}
 			client, err := RemoteWatch(options)
+			defer func() {
+				c, ok := client.(*remoteEskipFile)
+				if ok {
+					c.Close()
+					time.Sleep(time.Second)
+				}
+			}()
 			if err == nil && test.fail {
 				t.Error("failed to fail")
 				return

diff --git a/eskipfile/watch.go b/eskipfile/watch.go
@@ -3,6 +3,7 @@ package eskipfile
 import (
 	"os"
 	"reflect"
+	"sync"
 
 	"github.com/zalando/skipper/eskip"
 )
@@ -21,6 +22,7 @@ type WatchClient struct {
 	getAll     chan (chan<- watchResponse)
 	getUpdates chan (chan<- watchResponse)
 	quit       chan struct{}
+	once       sync.Once
 }
 
 // Watch creates a route configuration client with file watching. Watch doesn't follow file system nodes, it
@@ -31,6 +33,7 @@ func Watch(name string) *WatchClient {
 		getAll:     make(chan (chan<- watchResponse)),
 		getUpdates: make(chan (chan<- watchResponse)),
 		quit:       make(chan struct{}),
+		once:       sync.Once{},
 	}
 
 	go c.watch()
@@ -157,5 +160,7 @@ func (c *WatchClient) LoadUpdate() ([]*eskip.Route, []string, error) {
 
 // Close stops watching the configured file and providing updates.
 func (c *WatchClient) Close() {
-	close(c.quit)
+	c.once.Do(func() {
+		close(c.quit)
+	})
 }
diff --git a/filters/serve/serve_test.go b/filters/serve/serve_test.go
@@ -121,6 +121,7 @@ func TestServe(t *testing.T) {
 	if err != nil || string(b) != strings.Join(parts, "") {
 		t.Error("failed to serve body")
 	}
+	ctx.Response().Body.Close()
 }
 
 func TestStreamBody(t *testing.T) {

diff --git a/go.mod b/go.mod
@@ -33,7 +33,7 @@ require (
 	github.com/sarslanhan/cronmask v0.0.0-20190709075623-766eca24d011
 	github.com/sirupsen/logrus v1.8.1
 	github.com/sony/gobreaker v0.5.0
-	github.com/stretchr/testify v1.7.0
+	github.com/stretchr/testify v1.8.0
 	github.com/szuecs/rate-limit-buffer v0.7.1
 	github.com/testcontainers/testcontainers-go v0.12.0
 	github.com/tidwall/gjson v1.12.1
@@ -43,6 +43,7 @@ require (
 	github.com/yookoala/gofast v0.6.0
 	github.com/yuin/gopher-lua v0.0.0-20210529063254-f4c35e4016d9
 	go.uber.org/atomic v1.9.0
+	go.uber.org/goleak v1.2.0
 	golang.org/x/crypto v0.0.0-20220817201139-bc19a97f63c8
 	golang.org/x/net v0.0.0-20220909164309-bea034e7d591
 	golang.org/x/oauth2 v0.0.0-20211104180415-d3ed0bb246c8

diff --git a/go.sum b/go.sum
@@ -803,6 +803,7 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
 github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48=
 github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=
+github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
 github.com/stretchr/testify v0.0.0-20161117074351-18a02ba4a312/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v0.0.0-20180303142811-b89eecf5ca5d/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
@@ -812,6 +813,9 @@ github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5
 github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk=
+github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/syndtr/gocapability v0.0.0-20170704070218-db04d3cc01c8/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww=
 github.com/syndtr/gocapability v0.0.0-20180916011248-d98352740cb2/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww=
 github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww=
@@ -891,6 +895,8 @@ go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
 go.uber.org/atomic v1.9.0 h1:ECmE8Bn/WFTYwEW/bpKD3M8VtR/zQVbavAoalC1PYyE=
 go.uber.org/atomic v1.9.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
 go.uber.org/goleak v1.1.12/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
+go.uber.org/goleak v1.2.0 h1:xqgm/S+aQvhWFTtR0XK3Jvg7z8kGV8P4X14IzwN3Eqk=
+go.uber.org/goleak v1.2.0/go.mod h1:XJYK+MuIchqpmGmUSAzotztawfKvYLUIgg7guXrwVUo=
 go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
 go.uber.org/zap v1.10.0/go.mod h1:vwi/ZaCAaUcBkycHslxD9B2zi4UTXhF60s6SWpuDF0Q=
 golang.org/x/crypto v0.0.0-20171113213409-9f005a07e0d3/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=