From 7067fe0267a8cea49ddc9863b51ae2f01c3b8c15 Mon Sep 17 00:00:00 2001
From: Tyler Yahn <codingalias@gmail.com>
Date: Mon, 22 Nov 2021 13:15:24 -0800
Subject: [PATCH] Fix shutdown test cleanup

Do not check the second call to the client Stop. There is no guarantee
it will not error in normal operation.
---
 .../internal/otlptracetest/client.go          | 22 ++++++++++---------
 .../otlp/otlptrace/otlptracegrpc/client.go    | 22 +++++++++++++++----
 .../otlptracegrpc/client_unit_test.go         |  2 +-
 3 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/exporters/otlp/otlptrace/internal/otlptracetest/client.go b/exporters/otlp/otlptrace/internal/otlptracetest/client.go
index 0a14f16bfad..aedb8f4a9d2 100644
--- a/exporters/otlp/otlptrace/internal/otlptracetest/client.go
+++ b/exporters/otlp/otlptrace/internal/otlptracetest/client.go
@@ -56,11 +56,12 @@ func initializeExporter(t *testing.T, client otlptrace.Client) *otlptrace.Export
 
 func testClientStopHonorsTimeout(t *testing.T, client otlptrace.Client) {
 	t.Cleanup(func() {
-		// The test is looking for a failed shut down. Make sure the client is
-		// actually closed at the end thought to clean up any used resources.
-		if err := client.Stop(context.Background()); err != nil {
-			t.Fatalf("failed to stop client: %v", err)
-		}
+		// The test is looking for a failed shut down. Call Stop a second time
+		// with an un-expired context to give the client a second chance at
+		// cleaning up. There is not guarantee from the Client interface this
+		// will succeed, therefore, no need to check the error (just give it a
+		// best try).
+		_ = client.Stop(context.Background())
 	})
 	e := initializeExporter(t, client)
 
@@ -75,11 +76,12 @@ func testClientStopHonorsTimeout(t *testing.T, client otlptrace.Client) {
 
 func testClientStopHonorsCancel(t *testing.T, client otlptrace.Client) {
 	t.Cleanup(func() {
-		// The test is looking for a failed shut down. Make sure the client is
-		// actually closed at the end thought to clean up any used resources.
-		if err := client.Stop(context.Background()); err != nil {
-			t.Fatalf("failed to stop client: %v", err)
-		}
+		// The test is looking for a failed shut down. Call Stop a second time
+		// with an un-expired context to give the client a second chance at
+		// cleaning up. There is not guarantee from the Client interface this
+		// will succeed, therefore, no need to check the error (just give it a
+		// best try).
+		_ = client.Stop(context.Background())
 	})
 	e := initializeExporter(t, client)
 
diff --git a/exporters/otlp/otlptrace/otlptracegrpc/client.go b/exporters/otlp/otlptrace/otlptracegrpc/client.go
index 96589d0832c..834f6393887 100644
--- a/exporters/otlp/otlptrace/otlptracegrpc/client.go
+++ b/exporters/otlp/otlptrace/otlptracegrpc/client.go
@@ -109,9 +109,23 @@ func (c *client) Start(ctx context.Context) error {
 	return nil
 }
 
-var errNotStarted = errors.New("client not started")
+var errAlreadyStopped = errors.New("the client is already stopped")
 
-// Stop shuts down the gRPC connection to the collector.
+// Stop shuts down the client.
+//
+// Any active connections to a remote endpoint are closed if they were created
+// by the client. Any gRPC connection passed during creation using
+// WithGRPCConn will not be closed. It is the caller's responsibility to
+// handle cleanup of that resource.
+//
+// This method synchronizes with the UploadTraces method of the client. It
+// will wait for any active calls to that method to complete unimpeded, or it
+// will cancel any active calls if ctx expires. If ctx expires, the context
+// error will be forwarded as the returned error. All client held resources
+// will still be released still in this situation.
+//
+// If the client has already stopped, an error will be returned describing
+// this.
 func (c *client) Stop(ctx context.Context) error {
 	// Acquire the c.tscMu lock within the ctx lifetime.
 	acquired := make(chan struct{})
@@ -143,7 +157,7 @@ func (c *client) Stop(ctx context.Context) error {
 	// client is started before doing anything and let the called know if they
 	// made a mistake.
 	if c.tsc == nil {
-		return errNotStarted
+		return errAlreadyStopped
 	}
 
 	// Clear c.tsc to signal the client is stopped.
@@ -159,7 +173,7 @@ func (c *client) Stop(ctx context.Context) error {
 	return err
 }
 
-var errShutdown = errors.New("exporter is shutdown")
+var errShutdown = errors.New("the client is shutdown")
 
 // UploadTraces sends a batch of spans.
 //
diff --git a/exporters/otlp/otlptrace/otlptracegrpc/client_unit_test.go b/exporters/otlp/otlptrace/otlptracegrpc/client_unit_test.go
index c9d61fe6d99..b700d089eec 100644
--- a/exporters/otlp/otlptrace/otlptracegrpc/client_unit_test.go
+++ b/exporters/otlp/otlptrace/otlptracegrpc/client_unit_test.go
@@ -125,7 +125,7 @@ func TestRetryable(t *testing.T) {
 
 func TestUnstartedStop(t *testing.T) {
 	client := NewClient()
-	assert.ErrorIs(t, client.Stop(context.Background()), errNotStarted)
+	assert.ErrorIs(t, client.Stop(context.Background()), errAlreadyStopped)
 }
 
 func TestUnstartedUploadTrace(t *testing.T) {