Merge pull request #8011 from tamird/gossip-deadlock

*: use go-deadlock in race builds

GLOCKFILE

@@ -85,6 +85,7 @@ github.com/prometheus/common 610701b0cb96fe82d0166b8e07979b174b6f06ae
 github.com/rcrowley/go-metrics bdb33529eca3e55eac7328e07c57012a797af602
 github.com/robfig/glock c7fb89f56fbead624f7424f81f70af3a742433c5
 github.com/russross/blackfriday 93622da34e54fb6529bfb7c57e710f37a8d9cbd8
+github.com/sasha-s/go-deadlock 2248e9899ae84e5299201f9a4f8c75ac727f0772
 github.com/satori/go.uuid 0aa62d5ddceb50dbcb909d790b5345affd3669b6
 github.com/shurcooL/sanitized_anchor_name 10ef21a441db47d8b13ebcc5fd2310f636973c77
 github.com/spf13/cobra f62e98d28ab7ad31d707ba837a966378465c7b57

acceptance/chaos_test.go

@@ -24,7 +24,6 @@ import (
 	"math/rand"
 	"strconv"
 	"strings"
-	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -36,6 +35,7 @@ import (
 	"github.com/cockroachdb/cockroach/util"
 	"github.com/cockroachdb/cockroach/util/log"
 	"github.com/cockroachdb/cockroach/util/randutil"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 	"github.com/cockroachdb/cockroach/util/timeutil"
 )
 
@@ -45,7 +45,7 @@ var maxTransfer = flag.Int("max-transfer", 999, "Maximum amount to transfer in o
 var numAccounts = flag.Int("num-accounts", 999, "Number of accounts.")
 
 type testClient struct {
-	sync.RWMutex
+	syncutil.RWMutex
 	db    *gosql.DB
 	count uint64
 }

acceptance/cluster/docker.go

@@ -102,7 +102,7 @@ func hasImage(l *LocalCluster, ref string) bool {
 }
 
 func pullImage(l *LocalCluster, ref string, options types.ImagePullOptions) error {
-	// Hack: on CircleCI, docker pulls the image on the first access from an
+	// HACK: on CircleCI, docker pulls the image on the first access from an
 	// acceptance test even though that image is already present. So we first
 	// check to see if our image is present in order to avoid this slowness.
 	if hasImage(l, ref) {

acceptance/cluster/localcluster.go

@@ -28,7 +28,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"sync"
 	"testing"
 	"time"
 
@@ -49,6 +48,7 @@ import (
 	"github.com/cockroachdb/cockroach/util"
 	"github.com/cockroachdb/cockroach/util/log"
 	"github.com/cockroachdb/cockroach/util/stop"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 	"github.com/cockroachdb/cockroach/util/timeutil"
 )
 
@@ -128,7 +128,7 @@ type testNode struct {
 type LocalCluster struct {
 	client               client.APIClient
 	stopper              chan struct{}
-	mu                   sync.Mutex // Protects the fields below
+	mu                   syncutil.Mutex // Protects the fields below
 	vols                 *Container
 	config               TestConfig
 	Nodes                []*testNode

acceptance/dynamic_client.go

@@ -19,14 +19,14 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
-	"sync"
 
 	"golang.org/x/net/context"
 
 	"github.com/cockroachdb/cockroach/acceptance/cluster"
 	"github.com/cockroachdb/cockroach/testutils"
 	"github.com/cockroachdb/cockroach/util/log"
 	"github.com/cockroachdb/cockroach/util/stop"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 )
 
 // dynamicClient should be used in acceptance tests when connecting to a
@@ -36,7 +36,7 @@ type dynamicClient struct {
 	stopper *stop.Stopper
 
 	mu struct {
-		sync.Mutex
+		syncutil.Mutex
 		// clients is a map from node indexes used by methods passed to the
 		// cluster `c` to db clients.
 		clients map[int]*gosql.DB

build/check-style.sh

@@ -19,6 +19,11 @@ TestEnvutil() {
   ! git grep -nF 'os.Getenv' -- '*.go' | grep -vE '^((util/(log|envutil|sdnotify))|acceptance(/.*)?)/\w+\.go\b'
 }
 
+TestGrpc() {
+  echo "checking for grpc.NewServer calls (use rpc.NewServer instead)"
+  ! git grep -nF 'grpc.NewServer()' -- '*.go' | grep -vE '^rpc/context(_test)?\.go\b'
+}
+
 TestProtoClone() {
   echo "checking for proto.Clone calls (use protoutil.Clone instead)"
   ! git grep -nE '\.Clone\([^)]+\)' -- '*.go' | grep -vF 'protoutil.Clone' | grep -vE '^util/protoutil/clone(_test)?\.go\b'
@@ -29,9 +34,9 @@ TestProtoMarshal() {
   ! git grep -nE '\.Marshal\([^)]+\)' -- '*.go' | grep -vE '(json|yaml|protoutil)\.Marshal' | grep -vE '^util/protoutil/marshal(_test)?\.go\b'
 }
 
-TestGrpc() {
-  echo "checking for grpc.NewServer calls (use rpc.NewServer instead)"
-  ! git grep -nF 'grpc.NewServer()' -- '*.go' | grep -vE '^rpc/context(_test)?\.go\b'
+TestSyncMutex() {
+  echo "checking for sync.{,RW}Mutex usage (use syncutil.{,RW}Mutex instead)"
+  ! git grep -nE 'sync\.(RW)?Mutex' -- '*.go' | grep -vE '^util/syncutil/mutex_sync\.go\b'
 }
 
 TestMissingLeakTest() {

config/testutil.go

@@ -17,22 +17,23 @@
 package config
 
 import (
-	"sync"
-
 	"github.com/cockroachdb/cockroach/util/stop"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 )
 
 var (
 	testingZoneConfig   map[uint32]*ZoneConfig
 	testingHasHook      bool
 	testingPreviousHook func(SystemConfig, uint32) (*ZoneConfig, error)
-	testingLock         sync.Mutex
+	testingLock         syncutil.Mutex
 )
 
 // TestingSetupZoneConfigHook initializes the zone config hook
 // to 'testingZoneConfigHook' which uses 'testingZoneConfig'.
 // Settings go back to their previous values when the stopper runs our closer.
 func TestingSetupZoneConfigHook(stopper *stop.Stopper) {
+	stopper.AddCloser(stop.CloserFn(testingResetZoneConfigHook))
+
 	testingLock.Lock()
 	defer testingLock.Unlock()
 	if testingHasHook {
@@ -55,8 +56,6 @@ func TestingSetupZoneConfigHook(stopper *stop.Stopper) {
 		}
 		return
 	}
-
-	stopper.AddCloser(stop.CloserFn(testingResetZoneConfigHook))
 }
 
 // testingResetZoneConfigHook resets the zone config hook back to what it was

gossip/gossip.go

@@ -76,6 +76,7 @@ import (
 	"github.com/cockroachdb/cockroach/util/log"
 	"github.com/cockroachdb/cockroach/util/protoutil"
 	"github.com/cockroachdb/cockroach/util/stop"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 	"github.com/cockroachdb/cockroach/util/timeutil"
 )
 
@@ -142,7 +143,7 @@ type Gossip struct {
 	// Note that access to each client's internal state is serialized by the
 	// embedded server's mutex. This is surprising!
 	clientsMu struct {
-		sync.Mutex
+		syncutil.Mutex
 		clients []*client
 	}
 
@@ -160,7 +161,7 @@ type Gossip struct {
 	// main gossip lock.
 	systemConfig         config.SystemConfig
 	systemConfigSet      bool
-	systemConfigMu       sync.RWMutex
+	systemConfigMu       syncutil.RWMutex
 	systemConfigChannels []chan<- struct{}
 
 	// resolvers is a list of resolvers used to determine
@@ -259,17 +260,16 @@ func (g *Gossip) SetCullInterval(interval time.Duration) {
 // storage. This should be invoked as early in the lifecycle of a
 // gossip instance as possible, but can be called at any time.
 func (g *Gossip) SetStorage(storage Storage) error {
-	g.mu.Lock()
-	defer g.mu.Unlock()
-	g.storage = storage
-
-	// Read the bootstrap info from the persistent store.
+	// Maintain lock ordering.
 	var storedBI BootstrapInfo
-	err := storage.ReadBootstrapInfo(&storedBI)
-	if err != nil {
+	if err := storage.ReadBootstrapInfo(&storedBI); err != nil {
 		log.Warningf(context.TODO(), "failed to read gossip bootstrap info: %s", err)
 	}
 
+	g.mu.Lock()
+	defer g.mu.Unlock()
+	g.storage = storage
+
 	// Merge the stored bootstrap info addresses with any we've become
 	// aware of through gossip.
 	existing := map[string]struct{}{}
@@ -491,10 +491,13 @@ func (g *Gossip) updateNodeAddress(_ string, content roachpb.Value) {
 
 	// Add new address (if it's not already there) to bootstrap info and
 	// persist if possible.
-	if g.maybeAddBootstrapAddress(desc.Address) && g.storage != nil {
+	if storage := g.storage; g.maybeAddBootstrapAddress(desc.Address) && storage != nil {
+		// HACK: gymnastics to maintain lock ordering.
+		g.mu.Unlock()
+		defer g.mu.Lock()
 		// TODO(spencer): need to clean up ancient gossip nodes, which
 		//   will otherwise stick around in the bootstrap info forever.
-		if err := g.storage.WriteBootstrapInfo(&g.bootstrapInfo); err != nil {
+		if err := storage.WriteBootstrapInfo(&g.bootstrapInfo); err != nil {
 			log.Error(context.TODO(), err)
 		}
 	}

gossip/infostore.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"math"
 	"regexp"
-	"sync"
 	"time"
 
 	"golang.org/x/net/context"
@@ -33,6 +32,7 @@ import (
 	"github.com/cockroachdb/cockroach/util/hlc"
 	"github.com/cockroachdb/cockroach/util/log"
 	"github.com/cockroachdb/cockroach/util/stop"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 	"github.com/cockroachdb/cockroach/util/timeutil"
 )
 
@@ -71,13 +71,13 @@ type infoStore struct {
 	highWaterStamps map[roachpb.NodeID]int64 // Per-node information for gossip peers
 	callbacks       []*callback
 
-	callbackMu     sync.Mutex // Serializes callbacks
-	callbackWorkMu sync.Mutex // Protects callbackWork
+	callbackMu     syncutil.Mutex // Serializes callbacks
+	callbackWorkMu syncutil.Mutex // Protects callbackWork
 	callbackWork   []func()
 }
 
 var monoTime struct {
-	sync.Mutex
+	syncutil.Mutex
 	last int64
 }
 

gossip/infostore_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/cockroachdb/cockroach/util"
 	"github.com/cockroachdb/cockroach/util/leaktest"
 	"github.com/cockroachdb/cockroach/util/stop"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 	"github.com/gogo/protobuf/proto"
 )
 
@@ -343,7 +344,7 @@ func TestLeastUseful(t *testing.T) {
 type callbackRecord struct {
 	keys []string
 	wg   *sync.WaitGroup
-	sync.Mutex
+	syncutil.Mutex
 }
 
 func (cr *callbackRecord) Add(key string, _ roachpb.Value) {

gossip/server.go

@@ -32,6 +32,7 @@ import (
 	"github.com/cockroachdb/cockroach/util"
 	"github.com/cockroachdb/cockroach/util/log"
 	"github.com/cockroachdb/cockroach/util/stop"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 	"github.com/cockroachdb/cockroach/util/timeutil"
 )
 
@@ -45,7 +46,7 @@ type serverInfo struct {
 type server struct {
 	stopper *stop.Stopper
 
-	mu       sync.Mutex                         // Protects the fields below
+	mu       syncutil.Mutex                     // Protects the fields below
 	is       *infoStore                         // The backing infostore
 	incoming nodeSet                            // Incoming client node IDs
 	nodeMap  map[util.UnresolvedAddr]serverInfo // Incoming client's local address -> serverInfo

gossip/storage_test.go

@@ -20,7 +20,6 @@ import (
 	"reflect"
 	"sort"
 	"strings"
-	"sync"
 	"testing"
 	"time"
 
@@ -32,10 +31,11 @@ import (
 	"github.com/cockroachdb/cockroach/util"
 	"github.com/cockroachdb/cockroach/util/leaktest"
 	"github.com/cockroachdb/cockroach/util/protoutil"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 )
 
 type testStorage struct {
-	sync.Mutex
+	syncutil.Mutex
 	read, write bool
 	info        gossip.BootstrapInfo
 }

internal/client/client_test.go

@@ -48,6 +48,7 @@ import (
 	"github.com/cockroachdb/cockroach/util/hlc"
 	"github.com/cockroachdb/cockroach/util/leaktest"
 	"github.com/cockroachdb/cockroach/util/stop"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 	"github.com/cockroachdb/cockroach/util/timeutil"
 )
 
@@ -159,7 +160,7 @@ func TestClientRetryNonTxn(t *testing.T) {
 	// Set up a command filter which tracks which one of our test keys have
 	// been attempted to be pushed.
 	mu := struct {
-		sync.Mutex
+		syncutil.Mutex
 		m map[string]struct{}
 	}{
 		m: make(map[string]struct{}),

kv/dist_sender.go

@@ -427,7 +427,7 @@ func (ds *DistSender) getDescriptors(
 func (ds *DistSender) sendSingleRange(
 	ctx context.Context, ba roachpb.BatchRequest, desc *roachpb.RangeDescriptor,
 ) (*roachpb.BatchResponse, *roachpb.Error) {
-	// Hack: avoid formatting the message passed to Span.LogEvent for
+	// HACK: avoid formatting the message passed to Span.LogEvent for
 	// opentracing.noopSpans. We can't actually tell if we have a noopSpan, but
 	// we can see if the span as a NoopTracer. Note that this particular
 	// invocation is expensive because we're pretty-printing keys.

kv/leaseholder_cache.go

@@ -17,15 +17,14 @@
 package kv
 
 import (
-	"sync"
-
 	"github.com/cockroachdb/cockroach/roachpb"
 	"github.com/cockroachdb/cockroach/util/cache"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 )
 
 // A leaseHolderCache is a cache of replica descriptors keyed by range ID.
 type leaseHolderCache struct {
-	mu    sync.Mutex
+	mu    syncutil.Mutex
 	cache *cache.UnorderedCache
 }
 

kv/range_cache.go

@@ -28,6 +28,7 @@ import (
 	"github.com/cockroachdb/cockroach/roachpb"
 	"github.com/cockroachdb/cockroach/util/cache"
 	"github.com/cockroachdb/cockroach/util/log"
+	"github.com/cockroachdb/cockroach/util/syncutil"
 )
 
 // rangeCacheKey is the key type used to store and sort values in the
@@ -99,7 +100,7 @@ type rangeDescriptorCache struct {
 	// filled while servicing read and write requests to the key value
 	// store.
 	rangeCache struct {
-		sync.RWMutex
+		syncutil.RWMutex
 		cache *cache.OrderedCache
 	}
 	// lookupRequests stores all inflight requests retrieving range
@@ -108,7 +109,7 @@ type rangeDescriptorCache struct {
 	// multiplexed onto the same database lookup. See makeLookupRequestKey
 	// for details on this inference.
 	lookupRequests struct {
-		sync.Mutex
+		syncutil.Mutex
 		inflight map[lookupRequestKey]lookupRequest
 	}
 }