Fix some data races

agent/agent_test.go

@@ -18,6 +18,7 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -3729,10 +3730,27 @@ func TestAgent_SecurityChecks(t *testing.T) {
 	defer a.Shutdown()
 
 	data := make([]byte, 0, 8192)
-	bytesBuffer := bytes.NewBuffer(data)
-	a.LogOutput = bytesBuffer
+	buf := &syncBuffer{b: bytes.NewBuffer(data)}
+	a.LogOutput = buf
 	assert.NoError(t, a.Start(t))
-	assert.Contains(t, bytesBuffer.String(), "using enable-script-checks without ACLs and without allow_write_http_from is DANGEROUS")
+	assert.Contains(t, buf.String(), "using enable-script-checks without ACLs and without allow_write_http_from is DANGEROUS")
+}
+
+type syncBuffer struct {
+	lock sync.RWMutex
+	b    *bytes.Buffer
+}
+
+func (b *syncBuffer) Write(data []byte) (int, error) {
+	b.lock.Lock()
+	defer b.lock.Unlock()
+	return b.b.Write(data)
+}
+
+func (b *syncBuffer) String() string {
+	b.lock.Lock()
+	defer b.lock.Unlock()
+	return b.b.String()
 }
 
 func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) {

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)

agent/http_test.go

@@ -20,17 +20,18 @@ import (
 	"time"
 
 	"github.com/NYTimes/gziphandler"
+	cleanhttp "github.com/hashicorp/go-cleanhttp"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/net/http2"
+
 	"github.com/hashicorp/consul/agent/config"
 	"github.com/hashicorp/consul/agent/structs"
 	tokenStore "github.com/hashicorp/consul/agent/token"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/consul/testrpc"
-	cleanhttp "github.com/hashicorp/go-cleanhttp"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"golang.org/x/net/http2"
 )
 
 func TestHTTPServer_UnixSocket(t *testing.T) {
@@ -632,7 +633,7 @@ func TestHTTP_wrap_obfuscateLog(t *testing.T) {
 	}
 
 	t.Parallel()
-	buf := new(bytes.Buffer)
+	buf := &syncBuffer{b: new(bytes.Buffer)}
 	a := StartTestAgent(t, TestAgent{LogOutput: buf})
 	defer a.Shutdown()
 

agent/service_manager_test.go

@@ -8,11 +8,12 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/consul/testrpc"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestServiceManager_RegisterService(t *testing.T) {
@@ -330,26 +331,27 @@ func TestServiceManager_PersistService_API(t *testing.T) {
 
 	testrpc.WaitForLeader(t, a.RPC, "dc1")
 
-	// Now register a sidecar proxy via the API.
-	svc := &structs.NodeService{
-		Kind:    structs.ServiceKindConnectProxy,
-		ID:      "web-sidecar-proxy",
-		Service: "web-sidecar-proxy",
-		Port:    21000,
-		Proxy: structs.ConnectProxyConfig{
-			DestinationServiceName: "web",
-			DestinationServiceID:   "web",
-			LocalServiceAddress:    "127.0.0.1",
-			LocalServicePort:       8000,
-			Upstreams: structs.Upstreams{
-				{
-					DestinationName:      "redis",
-					DestinationNamespace: "default",
-					LocalBindPort:        5000,
+	newNodeService := func() *structs.NodeService {
+		return &structs.NodeService{
+			Kind:    structs.ServiceKindConnectProxy,
+			ID:      "web-sidecar-proxy",
+			Service: "web-sidecar-proxy",
+			Port:    21000,
+			Proxy: structs.ConnectProxyConfig{
+				DestinationServiceName: "web",
+				DestinationServiceID:   "web",
+				LocalServiceAddress:    "127.0.0.1",
+				LocalServicePort:       8000,
+				Upstreams: structs.Upstreams{
+					{
+						DestinationName:      "redis",
+						DestinationNamespace: "default",
+						LocalBindPort:        5000,
+					},
 				},
 			},
-		},
-		EnterpriseMeta: *structs.DefaultEnterpriseMeta(),
+			EnterpriseMeta: *structs.DefaultEnterpriseMeta(),
+		}
 	}
 
 	expectState := &structs.NodeService{
@@ -385,6 +387,7 @@ func TestServiceManager_PersistService_API(t *testing.T) {
 		EnterpriseMeta: *structs.DefaultEnterpriseMeta(),
 	}
 
+	svc := newNodeService()
 	svcID := svc.CompoundServiceID()
 
 	svcFile := filepath.Join(a.Config.DataDir, servicesDir, svcID.StringHash())
@@ -443,6 +446,7 @@ func TestServiceManager_PersistService_API(t *testing.T) {
 	}
 
 	// Updates service definition on disk
+	svc = newNodeService()
 	svc.Proxy.LocalServicePort = 8001
 	require.NoError(a.addServiceFromSource(svc, nil, true, "mytoken", ConfigSourceRemote))
 	requireFileIsPresent(t, svcFile)

agent/testagent.go

@@ -53,6 +53,8 @@ type TestAgent struct {
 	Config *config.RuntimeConfig
 
 	// LogOutput is the sink for the logs. If nil, logs are written to os.Stderr.
+	// The io.Writer must allow concurrent reads and writes. Note that
+	// bytes.Buffer is not safe for concurrent reads and writes.
 	LogOutput io.Writer
 
 	// DataDir may be set to a directory which exists. If is it not set,
@@ -343,8 +345,8 @@ func (a *TestAgent) Client() *api.Client {
 // DNSDisableCompression disables compression for all started DNS servers.
 func (a *TestAgent) DNSDisableCompression(b bool) {
 	for _, srv := range a.dnsServers {
-		cfg := srv.config.Load().(*dnsConfig)
-		cfg.DisableCompression = b
+		a.config.DNSDisableCompression = b
+		srv.ReloadConfig(a.config)
 	}
 }
 

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()
+	}
 }