From baa2b8628e41ba577fb893ef490f16480b3f1dc4 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Wed, 2 Jun 2021 18:59:31 -0400
Subject: [PATCH] consul: fix data race in leader CA tests

Some global variables are patched to shorter values in these tests. But the goroutines that read
them can outlive the test because nothing waited for them to exit.

This commit adds a Wait() method to the routine manager, so that tests can wait for the goroutines
to exit. This prevents the data race because the 'reset to original value' can happen
after all other goroutines have stopped.
---
 agent/consul/leader_connect_test.go | 15 ++++++++++++---
 lib/routine/routine.go              | 17 +++++++++++++++--
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go
index 4d713e0316..712a7efd6c 100644
--- a/agent/consul/leader_connect_test.go
+++ b/agent/consul/leader_connect_test.go
@@ -373,7 +373,10 @@ func TestLeader_Vault_PrimaryCA_IntermediateRenew(t *testing.T) {
 		}
 	})
 	defer os.RemoveAll(dir1)
-	defer s1.Shutdown()
+	defer func() {
+		s1.Shutdown()
+		s1.leaderRoutineManager.Wait()
+	}()
 
 	testrpc.WaitForLeader(t, s1.RPC, "dc1")
 
@@ -482,7 +485,10 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 		}
 	})
 	defer os.RemoveAll(dir1)
-	defer s1.Shutdown()
+	defer func() {
+		s1.Shutdown()
+		s1.leaderRoutineManager.Wait()
+	}()
 
 	testrpc.WaitForLeader(t, s1.RPC, "dc1")
 
@@ -493,7 +499,10 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 		c.Build = "1.6.0"
 	})
 	defer os.RemoveAll(dir2)
-	defer s2.Shutdown()
+	defer func() {
+		s2.Shutdown()
+		s2.leaderRoutineManager.Wait()
+	}()
 
 	// Create the WAN link
 	joinWAN(t, s2, s1)
diff --git a/lib/routine/routine.go b/lib/routine/routine.go
index e53bbb59a5..48fc2aef4e 100644
--- a/lib/routine/routine.go
+++ b/lib/routine/routine.go
@@ -24,6 +24,10 @@ func (r *routineTracker) running() bool {
 	}
 }
 
+func (r *routineTracker) wait() {
+	<-r.stoppedCh
+}
+
 type Manager struct {
 	lock   sync.RWMutex
 	logger hclog.Logger
@@ -131,6 +135,8 @@ func (m *Manager) stopInstance(name string) *routineTracker {
 	return instance
 }
 
+// StopAll goroutines. Once StopAll is called, it is no longer safe to add no
+// goroutines to the Manager.
 func (m *Manager) StopAll() {
 	m.lock.Lock()
 	defer m.lock.Unlock()
@@ -142,7 +148,14 @@ func (m *Manager) StopAll() {
 		m.logger.Debug("stopping routine", "routine", name)
 		routine.cancel()
 	}
+}
 
-	// just wipe out the entire map
-	m.routines = make(map[string]*routineTracker)
+// Wait for all goroutines to stop after StopAll is called.
+func (m *Manager) Wait() {
+	m.lock.Lock()
+	defer m.lock.Unlock()
+
+	for _, routine := range m.routines {
+		routine.wait()
+	}
 }