From d5eb76f32ec61cefbe6270759a46f25d4911f4ae Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Tue, 17 Mar 2015 16:24:19 +0000 Subject: [PATCH] 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 | 11 ++++++----- router/gossip.go | 7 +------ router/json.go | 6 +----- router/peer.go | 8 +++++--- router/peers.go | 6 +++--- router/routes.go | 5 +++-- 6 files changed, 19 insertions(+), 24 deletions(-) diff --git a/router/connection_maker.go b/router/connection_maker.go index 6042db51..236c7650 100644 --- a/router/connection_maker.go +++ b/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) { - ourConnectedPeers[peer] = true + for _, conn := range cm.ourself.Connections() { + 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 diff --git a/router/gossip.go b/router/gossip.go index 1e50b707..21e1d373 100644 --- a/router/gossip.go +++ b/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 - connections := make([]Connection, 0) - c.ourself.ForEachConnection(func(_ PeerName, conn Connection) { - connections = append(connections, conn) - }) - for _, conn := range connections { + for _, conn := range c.ourself.Connections() { conn.(ProtocolSender).SendProtocolMsg(protocolMsg) } } diff --git a/router/json.go b/router/json.go index 9d27fdde..dadd4ff6 100644 --- a/router/json.go +++ b/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) { diff --git a/router/peer.go b/router/peer.go index 86047e3a..3e61e529 100644 --- a/router/peer.go +++ b/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 { - fun(name, conn) + connections := make([]Connection, 0, len(peer.connections)) + for _, conn := range peer.connections { + connections = append(connections, conn) } + return connections } func (peer *Peer) SetVersionAndConnections(version uint64, connections map[PeerName]Connection) { diff --git a/router/peers.go b/router/peers.go index 514de2fd..9d9928ec 100644 --- a/router/peers.go +++ b/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() } diff --git a/router/routes.go b/router/routes.go index 0e109fe4..89c639a6 100644 --- a/router/routes.go +++ b/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 })