diff --git a/router/local_peer.go b/router/local_peer.go index f20b0676..09498629 100644 --- a/router/local_peer.go +++ b/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 diff --git a/router/peer.go b/router/peer.go index 29ffe6d7..5fcf277a 100644 --- a/router/peer.go +++ b/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() } } } diff --git a/router/peers.go b/router/peers.go index 1aac62ef..ab921c17 100644 --- a/router/peers.go +++ b/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) diff --git a/router/routes.go b/router/routes.go index 92197fb5..4c68e34d 100644 --- a/router/routes.go +++ b/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 }