coarser lock granularity

- get rid of individual Peer locks
- hold Peers read (or write) lock and LocalPeer read lock while
  performing route calculations

This eliminates the deadlock resulting from the inconsistent lock
acquisition order in route calculcation.

Fixes #643.

router/local_peer.go

@@ -4,10 +4,12 @@ import (
 	"fmt"
 	"log"
 	"net"
+	"sync"
 	"time"
 )
 
 type LocalPeer struct {
+	sync.RWMutex
 	*Peer
 	router     *Router
 	actionChan chan<- LocalPeerAction

router/peer.go

@@ -3,11 +3,9 @@ package router
 import (
 	"fmt"
 	"sort"
-	"sync"
 )
 
 type Peer struct {
-	sync.RWMutex
 	Name          PeerName
 	NameByte      []byte
 	NickName      string
@@ -61,13 +59,9 @@ func (peer *Peer) Info() string {
 // reaches that peer. The boolean return indicates whether that has
 // happened.
 //
-// We acquire read locks on peers as we encounter them during the
-// traversal. This prevents the connectivity graph from changing
-// underneath us in ways that would invalidate the result. Thus the
-// answer returned may be out of date, but never inconsistent.
+// NB: This function should generally be invoked while holding a read
+// lock on Peers and LocalPeer.
 func (peer *Peer) Routes(stopAt *Peer, establishedAndSymmetric bool) (bool, map[PeerName]PeerName) {
-	peer.RLock()
-	defer peer.RUnlock()
 	routes := make(map[PeerName]PeerName)
 	routes[peer.Name] = UnknownPeerName
 	nextWorklist := []*Peer{peer}
@@ -88,11 +82,7 @@ func (peer *Peer) Routes(stopAt *Peer, establishedAndSymmetric bool) (bool, map[
 					continue
 				}
 				remote := conn.Remote()
-				remote.RLock()
-				// DANGER holding rlock on remote, possibly taking
-				// rlock on remoteConn
 				if remoteConn, found := remote.connections[curName]; !establishedAndSymmetric || (found && remoteConn.Established()) {
-					defer remote.RUnlock()
 					nextWorklist = append(nextWorklist, remote)
 					// We now know how to get to remoteName: the same
 					// way we get to curPeer. Except, if curPeer is
@@ -103,8 +93,6 @@ func (peer *Peer) Routes(stopAt *Peer, establishedAndSymmetric bool) (bool, map[
 					} else {
 						routes[remoteName] = routes[curName]
 					}
-				} else {
-					remote.RUnlock()
 				}
 			}
 		}

router/peers.go

@@ -182,7 +182,9 @@ func (peers *Peers) String() string {
 
 func (peers *Peers) garbageCollect() []*Peer {
 	removed := []*Peer{}
+	peers.ourself.RLock()
 	_, reached := peers.ourself.Routes(nil, false)
+	peers.ourself.RUnlock()
 	for name, peer := range peers.table {
 		if _, found := reached[peer.Name]; !found && peer.localRefCount == 0 {
 			delete(peers.table, name)

router/routes.go

@@ -166,12 +166,17 @@ func (routes *Routes) run(recalculate <-chan *struct{}, wait <-chan chan struct{
 }
 
 func (routes *Routes) calculate() {
+	routes.peers.RLock()
+	routes.ourself.RLock()
 	var (
 		unicast      = routes.calculateUnicast(true)
 		unicastAll   = routes.calculateUnicast(false)
 		broadcast    = routes.calculateBroadcast(true)
 		broadcastAll = routes.calculateBroadcast(false)
 	)
+	routes.ourself.RUnlock()
+	routes.peers.RUnlock()
+
 	routes.Lock()
 	routes.unicast = unicast
 	routes.unicastAll = unicastAll
@@ -214,16 +219,13 @@ func (routes *Routes) calculateUnicast(establishedAndSymmetric bool) map[PeerNam
 // where <= is the subset relationship on keys of the returned map.
 func (routes *Routes) calculateBroadcast(establishedAndSymmetric bool) map[PeerName][]PeerName {
 	broadcast := make(map[PeerName][]PeerName)
-	ourself := routes.ourself
-	ourConnections := ourself.Connections()
-
-	routes.peers.ForEach(func(peer *Peer) {
+	for _, peer := range routes.peers.table {
 		hops := []PeerName{}
-		if found, reached := peer.Routes(ourself.Peer, establishedAndSymmetric); found {
+		if found, reached := peer.Routes(routes.ourself.Peer, establishedAndSymmetric); found {
 			// This is rather similar to the inner loop on
 			// peer.Routes(...); the main difference is in the
 			// locking.
-			for conn := range ourConnections {
+			for _, conn := range routes.ourself.connections {
 				if establishedAndSymmetric && !conn.Established() {
 					continue
 				}
@@ -235,12 +237,12 @@ func (routes *Routes) calculateBroadcast(establishedAndSymmetric bool) map[PeerN
 				// peer.connections requires a write lock on Peers,
 				// and since we are holding a read lock (due to the
 				// ForEach), access without locking the peer is safe.
-				if remoteConn, found := conn.Remote().connections[ourself.Name]; !establishedAndSymmetric || (found && remoteConn.Established()) {
+				if remoteConn, found := conn.Remote().connections[routes.ourself.Name]; !establishedAndSymmetric || (found && remoteConn.Established()) {
 					hops = append(hops, remoteName)
 				}
 			}
 		}
 		broadcast[peer.Name] = hops
-	})
+	}
 	return broadcast
 }