From d887b74a504a8e25e1c4b36661ca42f4050edf5b Mon Sep 17 00:00:00 2001 From: Will Sewell Date: Sat, 23 Mar 2024 01:10:54 +0100 Subject: [PATCH] Fix test goroutine leaks in ./cmd/collector/app/ (#5292) ## Which problem is this PR solving? Solves part of https://github.com/jaegertracing/jaeger/issues/5006. ## Description of the changes I introduced `testutils.VerifyGoLeaks(m)` and then ensured all leaked goroutines were stopped. ## How was this change tested? `make lint` and `make test` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Will Sewell --- cmd/collector/app/collector.go | 12 +++++++++--- cmd/collector/app/collector_test.go | 6 ++++++ cmd/collector/app/metrics_test.go | 3 +++ cmd/collector/app/package_test.go | 14 ++++++++++++++ cmd/collector/app/span_handler_builder_test.go | 1 + cmd/collector/app/span_processor_test.go | 5 +++++ internal/metricstest/local.go | 4 ++++ 7 files changed, 42 insertions(+), 3 deletions(-) create mode 100644 cmd/collector/app/package_test.go diff --git a/cmd/collector/app/collector.go b/cmd/collector/app/collector.go index 34f543d0dee..b8cfdc3b3cb 100644 --- a/cmd/collector/app/collector.go +++ b/cmd/collector/app/collector.go @@ -218,9 +218,15 @@ func (c *Collector) Close() error { } // watchers actually never return errors from Close - _ = c.tlsGRPCCertWatcherCloser.Close() - _ = c.tlsHTTPCertWatcherCloser.Close() - _ = c.tlsZipkinCertWatcherCloser.Close() + if c.tlsGRPCCertWatcherCloser != nil { + _ = c.tlsGRPCCertWatcherCloser.Close() + } + if c.tlsHTTPCertWatcherCloser != nil { + _ = c.tlsHTTPCertWatcherCloser.Close() + } + if c.tlsZipkinCertWatcherCloser != nil { + _ = c.tlsZipkinCertWatcherCloser.Close() + } return nil } diff --git a/cmd/collector/app/collector_test.go b/cmd/collector/app/collector_test.go index fbaad803e75..6e08ce76a19 100644 --- a/cmd/collector/app/collector_test.go +++ b/cmd/collector/app/collector_test.go @@ -52,6 +52,7 @@ func TestNewCollector(t *testing.T) { hc := healthcheck.New() logger := zap.NewNop() baseMetrics := metricstest.NewFactory(time.Hour) + defer baseMetrics.Backend.Stop() spanWriter := &fakeSpanWriter{} strategyStore := &mockStrategyStore{} tm := &tenancy.Manager{} @@ -78,6 +79,7 @@ func TestCollector_StartErrors(t *testing.T) { hc := healthcheck.New() logger := zap.NewNop() baseMetrics := metricstest.NewFactory(time.Hour) + defer baseMetrics.Backend.Stop() spanWriter := &fakeSpanWriter{} strategyStore := &mockStrategyStore{} tm := &tenancy.Manager{} @@ -94,6 +96,7 @@ func TestCollector_StartErrors(t *testing.T) { err := c.Start(options) require.Error(t, err) assert.Contains(t, err.Error(), expErr) + require.NoError(t, c.Close()) }) } @@ -131,7 +134,9 @@ func TestCollector_PublishOpts(t *testing.T) { hc := healthcheck.New() logger := zap.NewNop() baseMetrics := metricstest.NewFactory(time.Second) + defer baseMetrics.Backend.Stop() forkFactory := metricstest.NewFactory(time.Second) + defer forkFactory.Backend.Stop() metricsFactory := fork.New("internal", forkFactory, baseMetrics) spanWriter := &fakeSpanWriter{} strategyStore := &mockStrategyStore{} @@ -168,6 +173,7 @@ func TestAggregator(t *testing.T) { hc := healthcheck.New() logger := zap.NewNop() baseMetrics := metricstest.NewFactory(time.Hour) + defer baseMetrics.Backend.Stop() spanWriter := &fakeSpanWriter{} strategyStore := &mockStrategyStore{} agg := &mockAggregator{} diff --git a/cmd/collector/app/metrics_test.go b/cmd/collector/app/metrics_test.go index 06c644b8a76..8c0990f76d9 100644 --- a/cmd/collector/app/metrics_test.go +++ b/cmd/collector/app/metrics_test.go @@ -29,6 +29,7 @@ import ( func TestProcessorMetrics(t *testing.T) { baseMetrics := metricstest.NewFactory(time.Hour) + defer baseMetrics.Backend.Stop() serviceMetrics := baseMetrics.Namespace(jaegerM.NSOptions{Name: "service", Tags: nil}) hostMetrics := baseMetrics.Namespace(jaegerM.NSOptions{Name: "host", Tags: nil}) spm := NewSpanProcessorMetrics(serviceMetrics, hostMetrics, []processor.SpanFormat{processor.SpanFormat("scruffy")}) @@ -63,6 +64,7 @@ func TestProcessorMetrics(t *testing.T) { func TestNewTraceCountsBySvc(t *testing.T) { baseMetrics := metricstest.NewFactory(time.Hour) + defer baseMetrics.Backend.Stop() metrics := newTraceCountsBySvc(baseMetrics, "not_on_my_level", 3) metrics.countByServiceName("fry", false, model.SamplerTypeUnrecognized) @@ -95,6 +97,7 @@ func TestNewTraceCountsBySvc(t *testing.T) { func TestNewSpanCountsBySvc(t *testing.T) { baseMetrics := metricstest.NewFactory(time.Hour) + defer baseMetrics.Backend.Stop() metrics := newSpanCountsBySvc(baseMetrics, "not_on_my_level", 3) metrics.countByServiceName("fry", false) metrics.countByServiceName("leela", false) diff --git a/cmd/collector/app/package_test.go b/cmd/collector/app/package_test.go new file mode 100644 index 00000000000..5946e183ad1 --- /dev/null +++ b/cmd/collector/app/package_test.go @@ -0,0 +1,14 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "testing" + + "github.com/jaegertracing/jaeger/pkg/testutils" +) + +func TestMain(m *testing.M) { + testutils.VerifyGoLeaks(m) +} diff --git a/cmd/collector/app/span_handler_builder_test.go b/cmd/collector/app/span_handler_builder_test.go index 3dbbeb8fced..93b994bd744 100644 --- a/cmd/collector/app/span_handler_builder_test.go +++ b/cmd/collector/app/span_handler_builder_test.go @@ -61,6 +61,7 @@ func TestNewSpanHandlerBuilder(t *testing.T) { assert.NotNil(t, spanHandlers.JaegerBatchesHandler) assert.NotNil(t, spanHandlers.GRPCHandler) assert.NotNil(t, spanProcessor) + require.NoError(t, spanProcessor.Close()) } func TestDefaultSpanFilter(t *testing.T) { diff --git a/cmd/collector/app/span_processor_test.go b/cmd/collector/app/span_processor_test.go index 1334b08a953..9d7b5f21bb9 100644 --- a/cmd/collector/app/span_processor_test.go +++ b/cmd/collector/app/span_processor_test.go @@ -83,6 +83,7 @@ func TestBySvcMetrics(t *testing.T) { for _, test := range tests { mb := metricstest.NewFactory(time.Hour) + defer mb.Backend.Stop() logger := zap.NewNop() serviceMetrics := mb.Namespace(metrics.NSOptions{Name: "service", Tags: nil}) hostMetrics := mb.Namespace(metrics.NSOptions{Name: "host", Tags: nil}) @@ -258,6 +259,7 @@ func TestSpanProcessorErrors(t *testing.T) { err: fmt.Errorf("some-error"), } mb := metricstest.NewFactory(time.Hour) + defer mb.Backend.Stop() serviceMetrics := mb.Namespace(metrics.NSOptions{Name: "service", Tags: nil}) p := NewSpanProcessor(w, nil, @@ -342,6 +344,7 @@ func TestSpanProcessorBusy(t *testing.T) { func TestSpanProcessorWithNilProcess(t *testing.T) { mb := metricstest.NewFactory(time.Hour) + defer mb.Backend.Stop() serviceMetrics := mb.Namespace(metrics.NSOptions{Name: "service", Tags: nil}) w := &fakeSpanWriter{} @@ -440,6 +443,7 @@ func TestSpanProcessorCountSpan(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mb := metricstest.NewFactory(time.Hour) + defer mb.Backend.Stop() m := mb.Namespace(metrics.NSOptions{}) w := &fakeSpanWriter{} @@ -606,6 +610,7 @@ func TestStartDynQueueSizeUpdater(t *testing.T) { } assert.EqualValues(t, 104857, p.queue.Capacity()) + require.NoError(t, p.Close()) } func TestAdditionalProcessors(t *testing.T) { diff --git a/internal/metricstest/local.go b/internal/metricstest/local.go index d725661bad3..7036b9ae3a7 100644 --- a/internal/metricstest/local.go +++ b/internal/metricstest/local.go @@ -392,3 +392,7 @@ func (l *Factory) Namespace(scope metrics.NSOptions) metrics.Factory { Backend: l.Backend, } } + +func (l *Factory) Stop() { + l.Backend.Stop() +}