From 0b764a2a27133687142487c9b45da51ebcf8360c Mon Sep 17 00:00:00 2001
From: Matthias Radestock <matthias.radestock@gmail.com>
Date: Wed, 19 Aug 2015 10:22:11 +0100
Subject: [PATCH] plug GossipSender leak

GossipChannel.sendDown is the (only) place which creates
GossipSenders. It has two call sites, Send and SendDown, only the
former of which performed a gc of existing senders. Therefore, in
configurations with

- a lot of connection churn, and
- SendDown invocations for these connections, and
- few or no Send invocations

we can leak GossipSenders.

The most likely scenario for this to occur is a peer that, while
having no other connections, is making/accepting connections which get
torn down post handshake, e.g. because IPAM detected a clash.

Fixes #1329.
---
 router/gossip_channel.go | 46 +++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/router/gossip_channel.go b/router/gossip_channel.go
index 39de093a..d91d93bd 100644
--- a/router/gossip_channel.go
+++ b/router/gossip_channel.go
@@ -113,17 +113,32 @@ func (c *GossipChannel) Send(srcName PeerName, data GossipData) {
 	connections := c.ourself.Connections()
 	c.Lock()
 	defer c.Unlock()
-	// GC - randomly (courtesy of go's map iterator) pick some
-	// existing entries and stop&remove them if the associated
-	// connection is no longer active.  We stop as soon as we
-	// encounter a valid entry; the idea being that when there is
-	// little or no garbage then this executes close to O(1)[1],
-	// whereas when there is lots of garbage we remove it quickly.
-	//
-	// [1] TODO Unfortunately, due to the desire to avoid nested
-	// locks, instead of simply invoking Peer.ConnectionTo(name)
-	// below, we have that Peer.Connections() invocation above. That
-	// is O(n_our_connections) at best.
+	c.gcSenders(connections)
+	for conn := range selectedConnections {
+		c.sendDown(conn, data)
+	}
+}
+
+func (c *GossipChannel) SendDown(conn Connection, data GossipData) {
+	connections := c.ourself.Connections()
+	c.Lock()
+	defer c.Unlock()
+	c.gcSenders(connections)
+	c.sendDown(conn, data)
+}
+
+// GC - randomly (courtesy of go's map iterator) pick some existing
+// senders and stop&remove them if the associated connection is no
+// longer active.  We stop as soon as we encounter a valid entry; the
+// idea being that when there is little or no garbage then this
+// executes close to O(1)[1], whereas when there is lots of garbage we
+// remove it quickly.
+//
+// [1] TODO Unfortunately, due to the desire to avoid nested locks,
+// instead of simply invoking LocalPeer.ConnectionTo(name), we pass in
+// the result of LocalPeer.Connections(). That is O(n_our_connections)
+// at best.
+func (c *GossipChannel) gcSenders(connections ConnectionSet) {
 	for conn, sender := range c.senders {
 		if _, found := connections[conn]; !found {
 			delete(c.senders, conn)
@@ -132,15 +147,6 @@ func (c *GossipChannel) Send(srcName PeerName, data GossipData) {
 			break
 		}
 	}
-	for conn := range selectedConnections {
-		c.sendDown(conn, data)
-	}
-}
-
-func (c *GossipChannel) SendDown(conn Connection, data GossipData) {
-	c.Lock()
-	c.sendDown(conn, data)
-	c.Unlock()
 }
 
 func (c *GossipChannel) sendDown(conn Connection, data GossipData) {