From efd6034cee19cb1d38b7c01aa3ec78d6ee71dc30 Mon Sep 17 00:00:00 2001
From: Curtis Robert <crobert@splunk.com>
Date: Sat, 3 Feb 2024 09:37:05 -0800
Subject: [PATCH] [receiver/otlpreceiver] Enable goleak check (#9225)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Add `goleak` to detect leaks in tests. Leaking goroutines were detected
that were caused by a dependency that we can ignore
(`go.opencensus.io/stats/view.(*worker).start`), a `Shutdown` call was
also added that was missing.

**Link to tracking Issue:** <Issue number if applicable>
#9165

**Testing:** <Describe what testing was performed and which tests were
added.>
Added check is passing as well as existing tests.
---
 .chloggen/goleak_otlpreceiver.yaml    | 25 +++++++++++++++++++++++++
 receiver/otlpreceiver/factory_test.go |  2 --
 receiver/otlpreceiver/otlp.go         |  7 +++++--
 receiver/otlpreceiver/package_test.go | 17 +++++++++++++++++
 4 files changed, 47 insertions(+), 4 deletions(-)
 create mode 100755 .chloggen/goleak_otlpreceiver.yaml
 create mode 100644 receiver/otlpreceiver/package_test.go

diff --git a/.chloggen/goleak_otlpreceiver.yaml b/.chloggen/goleak_otlpreceiver.yaml
new file mode 100755
index 00000000000..7837fcdaa99
--- /dev/null
+++ b/.chloggen/goleak_otlpreceiver.yaml
@@ -0,0 +1,25 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
+component: otlpreceiver
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix goroutine leak when GRPC server is started but HTTP server is unsuccessful
+
+# One or more tracking issues or pull requests related to the change
+issues: [9165]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
+
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: []
\ No newline at end of file
diff --git a/receiver/otlpreceiver/factory_test.go b/receiver/otlpreceiver/factory_test.go
index b25b2aefe66..545b4d7ca7b 100644
--- a/receiver/otlpreceiver/factory_test.go
+++ b/receiver/otlpreceiver/factory_test.go
@@ -151,7 +151,6 @@ func TestCreateTracesReceiver(t *testing.T) {
 			assert.NoError(t, err)
 			if tt.wantStartErr {
 				assert.Error(t, tr.Start(context.Background(), componenttest.NewNopHost()))
-				assert.NoError(t, tr.Shutdown(context.Background()))
 			} else {
 				assert.NoError(t, tr.Start(context.Background(), componenttest.NewNopHost()))
 				assert.NoError(t, tr.Shutdown(context.Background()))
@@ -370,7 +369,6 @@ func TestCreateLogReceiver(t *testing.T) {
 			assert.NoError(t, err)
 			if tt.wantStartErr {
 				assert.Error(t, mr.Start(context.Background(), componenttest.NewNopHost()))
-				assert.NoError(t, mr.Shutdown(context.Background()))
 			} else {
 				require.NoError(t, mr.Start(context.Background(), componenttest.NewNopHost()))
 				assert.NoError(t, mr.Shutdown(context.Background()))
diff --git a/receiver/otlpreceiver/otlp.go b/receiver/otlpreceiver/otlp.go
index 4dd0d3e0469..da94a6eca71 100644
--- a/receiver/otlpreceiver/otlp.go
+++ b/receiver/otlpreceiver/otlp.go
@@ -168,12 +168,15 @@ func (r *otlpReceiver) startHTTPServer(host component.Host) error {
 
 // Start runs the trace receiver on the gRPC server. Currently
 // it also enables the metrics receiver too.
-func (r *otlpReceiver) Start(_ context.Context, host component.Host) error {
+func (r *otlpReceiver) Start(ctx context.Context, host component.Host) error {
 	if err := r.startGRPCServer(host); err != nil {
 		return err
 	}
 	if err := r.startHTTPServer(host); err != nil {
-		return err
+		// It's possible that a valid GRPC server configuration was specified,
+		// but an invalid HTTP configuration. If that's the case, the successfully
+		// started GRPC server must be shutdown to ensure no goroutines are leaked.
+		return errors.Join(err, r.Shutdown(ctx))
 	}
 
 	return nil
diff --git a/receiver/otlpreceiver/package_test.go b/receiver/otlpreceiver/package_test.go
new file mode 100644
index 00000000000..9d49b5c8f7b
--- /dev/null
+++ b/receiver/otlpreceiver/package_test.go
@@ -0,0 +1,17 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package otlpreceiver
+
+import (
+	"testing"
+
+	"go.uber.org/goleak"
+)
+
+// The IgnoreTopFunction call prevents catching the leak generated by opencensus
+// defaultWorker.Start which at this time is part of the package's init call.
+// See https://github.com/open-telemetry/opentelemetry-collector/issues/9165#issuecomment-1874836336 for more context.
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m, goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start"))
+}