From e639007b62419ba1f8533999d87bd1a75f8682e2 Mon Sep 17 00:00:00 2001
From: Matthias Radestock <matthias.radestock@gmail.com>
Date: Thu, 9 Apr 2015 06:34:35 +0100
Subject: [PATCH] eliminate AddConnection/RemoveConnection race We were
 invoking AddConnection in a separate goroutine, and this could result in
 RemoveConnection getting invoked first, resulting in bogus entries in our
 connection map, which would effectively prevent connectivity to the other
 peer until the stale entry got removed through a lost tie break. And if, per
 chance, the remote end ran into exactly the same race condition, then
 connectivity between that peer and ourself was broken permanently.

Hitting this race condition required a combination of a) no UDP
heartbeat being received for HeartbeatTimeout(30s) - this causes the
actorLoop to terminate and RemoveConnection to be invoked, and b) the
AddConnection invocation getting delayed for at least that long.
---
 router/connection.go | 80 +++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 35 deletions(-)

diff --git a/router/connection.go b/router/connection.go
index 9c478342..a42f04ce 100644
--- a/router/connection.go
+++ b/router/connection.go
@@ -297,51 +297,61 @@ func (conn *LocalConnection) run(actionChan <-chan ConnectionAction, finished ch
 	}
 	log.Printf("->[%s] completed handshake with %s\n", conn.remoteTCPAddr, conn.remote.FullName())
 
-	// We invoke AddConnection in the same goroutine that subsequently
-	// becomes the tcp receive loop, rather than outside, because a)
-	// the ordering relative to the receive loop is the only one that
-	// matters [1], b) it prevents unnecessary delays in entering the
-	// main connection loop, and c) it guards against potential
-	// deadlocks.
-	go func() {
-		conn.Router.Ourself.AddConnection(conn)
-		conn.receiveTCP(dec)
-	}()
+	if err := conn.initHeartbeats(); err != nil { // [1]
+		conn.Log("connection shutting down due to error:", err)
+		return
+	}
 
+	conn.Router.Ourself.AddConnection(conn) // [1]
+
+	go conn.receiveTCP(dec) // [1]
+
+	if err := conn.actorLoop(actionChan); err != nil { // [1]
+		conn.Log("connection shutting down due to error:", err)
+		return
+	}
+	conn.Log("connection shutting down")
+}
+
+// [1] Ordering constraints:
+//
+// (a) initHeartbeats should precede AddConnection. That way we can
+// guarantee that a) the forwarder gets initialised before the router
+// can discover the connection and attempt to invoke Forward, and b)
+// Heartbeats are the first thing that get sent on connections.
+// Neither of these are essential, but they are desirable.
+//
+// (b) AddConnection must precede receiveTCP. In the absence of any
+// indirect connectivity to the remote peer, the first we hear about
+// it (and any peers reachable from it) is through topology gossip it
+// sends us on the connection. We must ensure that the connection has
+// been added to Ourself prior to processing any such gossip,
+// otherwise we risk immediately gc'ing part of that newly received
+// portion of the topology (though not the remote peer itself, since
+// that will have a positive ref count), leaving behind dangling
+// references to peers. Hence we must invoke AddConnection, which is
+// *synchronous*, first.
+//
+// (c) AddConnection must precede actorLoop. More precisely, it must
+// precede shutdown, since that invokes DeleteConnection and is
+// invoked on termination of this entire function. Essentially this
+// boils down to a prohibition on running AddConnection in a separate
+// goroutine, at least not without some synchronisation.
+
+func (conn *LocalConnection) initHeartbeats() error {
+	conn.heartbeatTimeout = time.NewTimer(HeartbeatTimeout)
 	heartbeatFrameBytes := make([]byte, EthernetOverhead+8)
 	binary.BigEndian.PutUint64(heartbeatFrameBytes[EthernetOverhead:], conn.uid)
 	conn.heartbeatFrame = &ForwardedFrame{
 		srcPeer: conn.local,
 		dstPeer: conn.remote,
 		frame:   heartbeatFrameBytes}
-
-	if conn.remoteUDPAddr != nil {
-		if err := conn.sendFastHeartbeats(); err != nil {
-			conn.Log("connection shutting down due to error:", err)
-			return
-		}
-	}
-
-	conn.heartbeatTimeout = time.NewTimer(HeartbeatTimeout)
-
-	if err := conn.actorLoop(actionChan); err != nil {
-		conn.Log("connection shutting down due to error:", err)
-	} else {
-		conn.Log("connection shutting down")
+	if conn.remoteUDPAddr == nil {
+		return nil
 	}
+	return conn.sendFastHeartbeats()
 }
 
-// [1] In the absence of any indirect connectivity to the remote peer,
-// the first we hear about it (and any peers reachable from it) is
-// through topology gossip it sends us on the connection. We must
-// ensure that the connection has been added to Ourself prior to
-// processing any such gossip, otherwise we risk immediately gc'ing
-// part of that newly received portion of the topology (though not the
-// remote peer itself, since that will have a positive ref count),
-// leaving behind dangling references to peers. Therefore we invoke
-// AddConnection, which is *synchronous*, before entering the tcp
-// receive loop.
-
 func (conn *LocalConnection) actorLoop(actionChan <-chan ConnectionAction) (err error) {
 	for err == nil {
 		select {