replace Peer.ForEachConnection with Peer.Connections

The former is prone to triggering deadlocks since the lock is held on
the peer during the iteration.

router/connection_maker.go

@@ -119,10 +119,10 @@ func (cm *ConnectionMaker) checkStateAndAttemptConnections() time.Duration {
 	// copy the set of things we are connected to, so we can access them without locking
 	ourConnectedPeers := make(map[PeerName]bool)
 	ourConnectedTargets := make(map[string]bool)
-	cm.ourself.ForEachConnection(func(peer PeerName, conn Connection) {
+	for _, conn := range cm.ourself.Connections() {
-		ourConnectedPeers[peer] = true
+		ourConnectedPeers[conn.Remote().Name] = true
 		ourConnectedTargets[conn.RemoteTCPAddr()] = true
-	})
+	}
 
 	addTarget := func(address string) {
 		if !ourConnectedTargets[address] {
@@ -139,7 +139,8 @@ func (cm *ConnectionMaker) checkStateAndAttemptConnections() time.Duration {
 	// Add targets for peers that someone else is connected to, but we
 	// aren't
 	cm.peers.ForEach(func(name PeerName, peer *Peer) {
-		peer.ForEachConnection(func(otherPeer PeerName, conn Connection) {
+		for _, conn := range peer.Connections() {
+			otherPeer := conn.Remote().Name
 			if otherPeer == cm.ourself.Name || ourConnectedPeers[otherPeer] {
 				return
 			}
@@ -151,7 +152,7 @@ func (cm *ConnectionMaker) checkStateAndAttemptConnections() time.Duration {
 			if host, _, err := net.SplitHostPort(address); err == nil {
 				addTarget(NormalisePeerAddr(host))
 			}
-		})
+		}
 	})
 
 	now := time.Now() // make sure we catch items just added

router/gossip.go

@@ -117,12 +117,7 @@ func deliverGossip(channel *GossipChannel, srcName PeerName, _ []byte, dec *gob.
 
 func (c *GossipChannel) SendGossipMsg(buf []byte) {
 	protocolMsg := c.gossipMsg(buf)
-	// Copy the connections so we don't hold a lock
+	for _, conn := range c.ourself.Connections() {
-	connections := make([]Connection, 0)
-	c.ourself.ForEachConnection(func(_ PeerName, conn Connection) {
-		connections = append(connections, conn)
-	})
-	for _, conn := range connections {
 		conn.(ProtocolSender).SendProtocolMsg(protocolMsg)
 	}
 }

router/json.go

@@ -61,16 +61,12 @@ func (routes *Routes) MarshalJSON() ([]byte, error) {
 }
 
 func (peer *Peer) MarshalJSON() ([]byte, error) {
-	conns := make([]Connection, 0)
-	peer.ForEachConnection(func(_ PeerName, conn Connection) {
-		conns = append(conns, conn)
-	})
 	return json.Marshal(struct {
 		Name        string
 		UID         uint64
 		Version     uint64
 		Connections []Connection
-	}{peer.Name.String(), peer.UID, peer.version, conns})
+	}{peer.Name.String(), peer.UID, peer.version, peer.Connections()})
 }
 
 func (conn *RemoteConnection) MarshalJSON() ([]byte, error) {

router/peer.go

@@ -73,12 +73,14 @@ func (peer *Peer) ConnectionTo(name PeerName) (Connection, bool) {
 	return conn, found // yes, you really can't inline that. FFS.
 }
 
-func (peer *Peer) ForEachConnection(fun func(PeerName, Connection)) {
+func (peer *Peer) Connections() []Connection {
 	peer.RLock()
 	defer peer.RUnlock()
-	for name, conn := range peer.connections {
+	connections := make([]Connection, 0, len(peer.connections))
-		fun(name, conn)
+	for _, conn := range peer.connections {
+		connections = append(connections, conn)
 	}
+	return connections
 }
 
 func (peer *Peer) SetVersionAndConnections(version uint64, connections map[PeerName]Connection) {

router/peers.go

@@ -123,13 +123,13 @@ func (peers *Peers) String() string {
 	var buf bytes.Buffer
 	peers.ForEach(func(name PeerName, peer *Peer) {
 		buf.WriteString(fmt.Sprint(peer, "\n"))
-		peer.ForEachConnection(func(remoteName PeerName, conn Connection) {
+		for _, conn := range peer.Connections() {
 			established := ""
 			if !conn.Established() {
 				established = " (unestablished)"
 			}
-			buf.WriteString(fmt.Sprintf("   -> %v (%v) [%v%s]\n", remoteName, conn.Remote().NickName, conn.RemoteTCPAddr(), established))
+			buf.WriteString(fmt.Sprintf("   -> %v (%v) [%v%s]\n", conn.Remote().Name, conn.Remote().NickName, conn.RemoteTCPAddr(), established))
-		})
+		}
 	})
 	return buf.String()
 }

router/routes.go

@@ -140,17 +140,18 @@ func (routes *Routes) calculateBroadcast() map[PeerName][]PeerName {
 			// This is rather similar to the inner loop on
 			// peer.Routes(...); the main difference is in the
 			// locking.
-			ourself.ForEachConnection(func(remoteName PeerName, conn Connection) {
+			for _, conn := range ourself.Connections() {
 				if !conn.Established() {
 					return
 				}
+				remoteName := conn.Remote().Name
 				if _, found := reached[remoteName]; found {
 					return
 				}
 				if remoteConn, found := conn.Remote().ConnectionTo(ourself.Name); found && remoteConn.Established() {
 					hops = append(hops, remoteName)
 				}
-			})
+			}
 		}
 		broadcast[name] = hops
 	})