From 5a2c4d762971865f358516be6375413b4e30330c Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Mon, 17 Jun 2024 15:19:55 -0400 Subject: [PATCH 1/5] Remove unnecessary .nocover files Signed-off-by: Yuri Shkuro --- cmd/all-in-one/.nocover | 1 - cmd/anonymizer/app/anonymizer/.nocover | 1 - cmd/internal/flags/.nocover | 2 -- 3 files changed, 4 deletions(-) delete mode 100644 cmd/all-in-one/.nocover delete mode 100644 cmd/anonymizer/app/anonymizer/.nocover delete mode 100644 cmd/internal/flags/.nocover diff --git a/cmd/all-in-one/.nocover b/cmd/all-in-one/.nocover deleted file mode 100644 index 8ab06f28b67..00000000000 --- a/cmd/all-in-one/.nocover +++ /dev/null @@ -1 +0,0 @@ -main is not testable diff --git a/cmd/anonymizer/app/anonymizer/.nocover b/cmd/anonymizer/app/anonymizer/.nocover deleted file mode 100644 index 5b583b79e93..00000000000 --- a/cmd/anonymizer/app/anonymizer/.nocover +++ /dev/null @@ -1 +0,0 @@ -non-critical test utility diff --git a/cmd/internal/flags/.nocover b/cmd/internal/flags/.nocover deleted file mode 100644 index 46911e9377b..00000000000 --- a/cmd/internal/flags/.nocover +++ /dev/null @@ -1,2 +0,0 @@ -FIXME - From 74453ada53b4ec319e726ad3b366e9c5617199bb Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Mon, 17 Jun 2024 15:46:24 -0400 Subject: [PATCH 2/5] fix Signed-off-by: Yuri Shkuro --- cmd/all-in-one/package_test.go | 14 ++++++++++++++ cmd/anonymizer/app/anonymizer/package_test.go | 14 ++++++++++++++ cmd/internal/flags/package_test.go | 14 ++++++++++++++ 3 files changed, 42 insertions(+) create mode 100644 cmd/all-in-one/package_test.go create mode 100644 cmd/anonymizer/app/anonymizer/package_test.go create mode 100644 cmd/internal/flags/package_test.go diff --git a/cmd/all-in-one/package_test.go b/cmd/all-in-one/package_test.go new file mode 100644 index 00000000000..3694f7ca674 --- /dev/null +++ b/cmd/all-in-one/package_test.go @@ -0,0 +1,14 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "testing" + + "github.com/jaegertracing/jaeger/pkg/testutils" +) + +func TestMain(m *testing.M) { + testutils.VerifyGoLeaks(m) +} diff --git a/cmd/anonymizer/app/anonymizer/package_test.go b/cmd/anonymizer/app/anonymizer/package_test.go new file mode 100644 index 00000000000..a550d335fdd --- /dev/null +++ b/cmd/anonymizer/app/anonymizer/package_test.go @@ -0,0 +1,14 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package anonymizer + +import ( + "testing" + + "github.com/jaegertracing/jaeger/pkg/testutils" +) + +func TestMain(m *testing.M) { + testutils.VerifyGoLeaks(m) +} diff --git a/cmd/internal/flags/package_test.go b/cmd/internal/flags/package_test.go new file mode 100644 index 00000000000..da12c0031e5 --- /dev/null +++ b/cmd/internal/flags/package_test.go @@ -0,0 +1,14 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package flags + +import ( + "testing" + + "github.com/jaegertracing/jaeger/pkg/testutils" +) + +func TestMain(m *testing.M) { + testutils.VerifyGoLeaks(m) +} From 541388701796dc76a5a622cf2f8d8d16c274f74d Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Mon, 17 Jun 2024 16:45:37 -0400 Subject: [PATCH 3/5] fix-http-goleaks Signed-off-by: Yuri Shkuro --- cmd/all-in-one/all_in_one_test.go | 66 ++++++++++++------------------- 1 file changed, 26 insertions(+), 40 deletions(-) diff --git a/cmd/all-in-one/all_in_one_test.go b/cmd/all-in-one/all_in_one_test.go index f5186001976..a34f34d6d77 100644 --- a/cmd/all-in-one/all_in_one_test.go +++ b/cmd/all-in-one/all_in_one_test.go @@ -75,7 +75,10 @@ func healthCheck(t *testing.T) { require.Eventuallyf( t, func() bool { - _, err := http.Get(queryAddr + "/") + resp, err := http.Get(queryAddr + "/") + if err == nil { + resp.Body.Close() + } return err == nil }, 10*time.Second, @@ -85,14 +88,23 @@ func healthCheck(t *testing.T) { t.Logf("Server detected at %s", queryAddr) } -func checkWebUI(t *testing.T) { - resp, err := http.Get(queryAddr + "/") +func httpGet(t *testing.T, url string) (*http.Response, []byte) { + req, err := http.NewRequest(http.MethodGet, url, nil) + require.NoError(t, err) + req.Close = true // no presistent connections which leak goroutines + + resp, err := httpClient.Do(req) require.NoError(t, err) - require.NotNil(t, resp) defer resp.Body.Close() - assert.Equal(t, http.StatusOK, resp.StatusCode) + bodyBytes, err := io.ReadAll(resp.Body) require.NoError(t, err) + + return resp, bodyBytes +} + +func checkWebUI(t *testing.T) { + _, bodyBytes := httpGet(t, queryAddr+"/") body := string(bodyBytes) t.Run("Static_files", func(t *testing.T) { pattern := regexp.MustCompile(` Date: Mon, 17 Jun 2024 16:55:36 -0400 Subject: [PATCH 4/5] fix leaks in flags Signed-off-by: Yuri Shkuro --- cmd/internal/flags/admin.go | 6 ++++-- cmd/internal/flags/admin_test.go | 13 ++++++++----- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/cmd/internal/flags/admin.go b/cmd/internal/flags/admin.go index ea4df675a04..974aafdaacb 100644 --- a/cmd/internal/flags/admin.go +++ b/cmd/internal/flags/admin.go @@ -170,6 +170,8 @@ func (s *AdminServer) registerPprofHandlers() { // Close stops the HTTP server func (s *AdminServer) Close() error { - _ = s.tlsCertWatcherCloser.Close() - return s.server.Shutdown(context.Background()) + return errors.Join( + s.tlsCertWatcherCloser.Close(), + s.server.Shutdown(context.Background()), + ) } diff --git a/cmd/internal/flags/admin_test.go b/cmd/internal/flags/admin_test.go index ba4a0e01e1f..3f65ba08934 100644 --- a/cmd/internal/flags/admin_test.go +++ b/cmd/internal/flags/admin_test.go @@ -27,6 +27,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" + "go.uber.org/zap/zaptest" "go.uber.org/zap/zaptest/observer" "github.com/jaegertracing/jaeger/pkg/config" @@ -137,10 +138,8 @@ func TestAdminServerTLS(t *testing.T) { v, command := config.Viperize(adminServer.AddFlags) err := command.ParseFlags(test.serverTLSFlags) require.NoError(t, err) - zapCore, _ := observer.New(zap.InfoLevel) - logger := zap.New(zapCore) - err = adminServer.initFromViper(v, logger) + err = adminServer.initFromViper(v, zaptest.NewLogger(t)) require.NoError(t, err) adminServer.Serve() @@ -148,6 +147,7 @@ func TestAdminServerTLS(t *testing.T) { clientTLSCfg, err0 := test.clientTLS.Config(zap.NewNop()) require.NoError(t, err0) + defer test.clientTLS.Close() dialer := &net.Dialer{Timeout: 2 * time.Second} conn, clientError := tls.DialWithDialer(dialer, "tcp", fmt.Sprintf("localhost:%d", ports.CollectorAdminHTTP), clientTLSCfg) require.NoError(t, clientError) @@ -158,9 +158,12 @@ func TestAdminServerTLS(t *testing.T) { TLSClientConfig: clientTLSCfg, }, } - - response, requestError := client.Get(fmt.Sprintf("https://localhost:%d", ports.CollectorAdminHTTP)) + req, err := http.NewRequest("GET", fmt.Sprintf("https://localhost:%d", ports.CollectorAdminHTTP), nil) + require.NoError(t, err) + req.Close = true // no persistent connections, they cause goroutine leaks + response, requestError := client.Do(req) require.NoError(t, requestError) + defer response.Body.Close() require.NotNil(t, response) }) } From 5136427bfa71133c058858d7ceb7eee3bd968864 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Mon, 17 Jun 2024 17:03:34 -0400 Subject: [PATCH 5/5] delint Signed-off-by: Yuri Shkuro --- cmd/all-in-one/all_in_one_test.go | 2 +- cmd/internal/flags/admin_test.go | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cmd/all-in-one/all_in_one_test.go b/cmd/all-in-one/all_in_one_test.go index a34f34d6d77..95b7ea9bac8 100644 --- a/cmd/all-in-one/all_in_one_test.go +++ b/cmd/all-in-one/all_in_one_test.go @@ -91,7 +91,7 @@ func healthCheck(t *testing.T) { func httpGet(t *testing.T, url string) (*http.Response, []byte) { req, err := http.NewRequest(http.MethodGet, url, nil) require.NoError(t, err) - req.Close = true // no presistent connections which leak goroutines + req.Close = true // avoid persistent connections which leak goroutines resp, err := httpClient.Do(req) require.NoError(t, err) diff --git a/cmd/internal/flags/admin_test.go b/cmd/internal/flags/admin_test.go index 3f65ba08934..87325346074 100644 --- a/cmd/internal/flags/admin_test.go +++ b/cmd/internal/flags/admin_test.go @@ -158,9 +158,10 @@ func TestAdminServerTLS(t *testing.T) { TLSClientConfig: clientTLSCfg, }, } - req, err := http.NewRequest("GET", fmt.Sprintf("https://localhost:%d", ports.CollectorAdminHTTP), nil) + url := fmt.Sprintf("https://localhost:%d", ports.CollectorAdminHTTP) + req, err := http.NewRequest(http.MethodGet, url, nil) require.NoError(t, err) - req.Close = true // no persistent connections, they cause goroutine leaks + req.Close = true // avoid persistent connections which leak goroutines response, requestError := client.Do(req) require.NoError(t, requestError) defer response.Body.Close()