From 5c38f38dd9a4f1006c60daaa3f31eebb5f118833 Mon Sep 17 00:00:00 2001 From: Asim Aslam Date: Fri, 18 Oct 2019 10:29:14 +0100 Subject: [PATCH 1/2] No need to lock here since Topology read locks and makes copies --- network/node.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/network/node.go b/network/node.go index 8b4d3f57..194dd6d2 100644 --- a/network/node.go +++ b/network/node.go @@ -170,9 +170,6 @@ func (n *node) Nodes() []Node { // GetPeerNode returns a node from node MaxDepth topology // It returns nil if the peer was not found func (n *node) GetPeerNode(id string) *node { - n.RLock() - defer n.RUnlock() - // get node topology up to MaxDepth top := n.Topology(MaxDepth) From 3d5d9be02a494dcb1e7081571c7d920b3342aa0b Mon Sep 17 00:00:00 2001 From: Milos Gajdos Date: Fri, 18 Oct 2019 11:23:28 +0100 Subject: [PATCH 2/2] Avoid recursive calls to RLock() Topology calls itsel recursively invoking RLock. This, according to go documentation is wrong. This commit moves the body of Topology function to a non-thread safe unexported function to keep locsk at check! --- network/node.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/network/node.go b/network/node.go index 194dd6d2..e8632ccc 100644 --- a/network/node.go +++ b/network/node.go @@ -237,12 +237,9 @@ func (n *node) PruneStalePeerNodes(pruneTime time.Duration) map[string]*node { return pruned } -// Topology returns a copy of the node topology down to given depth -// NOTE: the returned node is a node graph - not a single node -func (n *node) Topology(depth uint) *node { - n.RLock() - defer n.RUnlock() - +// getTopology traverses node graph and builds node topology +// NOTE: this function is not thread safe +func (n *node) getTopology(depth uint) *node { // make a copy of yourself node := &node{ id: n.id, @@ -262,7 +259,7 @@ func (n *node) Topology(depth uint) *node { // iterate through our peers and update the node peers for _, peer := range n.peers { - nodePeer := peer.Topology(depth) + nodePeer := peer.getTopology(depth) if _, ok := node.peers[nodePeer.id]; !ok { node.peers[nodePeer.id] = nodePeer } @@ -271,6 +268,15 @@ func (n *node) Topology(depth uint) *node { return node } +// Topology returns a copy of the node topology down to given depth +// NOTE: the returned node is a node graph - not a single node +func (n *node) Topology(depth uint) *node { + n.RLock() + defer n.RUnlock() + + return n.getTopology(depth) +} + // Peers returns node peers up to MaxDepth func (n *node) Peers() []Node { n.RLock() @@ -278,7 +284,7 @@ func (n *node) Peers() []Node { var peers []Node for _, nodePeer := range n.peers { - peer := nodePeer.Topology(MaxDepth) + peer := nodePeer.getTopology(MaxDepth) peers = append(peers, peer) }