From 9b2581564111d0d2c62346f5ea6867bf36004a3c Mon Sep 17 00:00:00 2001
From: Matthias Radestock <matthias.radestock@gmail.com>
Date: Wed, 15 Apr 2015 20:04:40 +0100
Subject: [PATCH] don't send heartbeats on new but ultimately invalid
 connections It is wasteful and can trip up crypto at the other end. See
 comment for why.

To accomplish this we move the responsibility for shutting down the
new, invalid connection from LocalPeer.AddConnection to its caller,
i.e. LocalConnection.run. So rather than always firing up all the
connection machinery and getting told asynchronously by AddConnection
to shut it all down again, we now only fire it up if AddConnection
succeeded.
---
 router/connection.go | 31 +++++++++++++++++++++++--------
 router/local_peer.go | 23 ++++++++++-------------
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/router/connection.go b/router/connection.go
index 240abddd..fdadfac9 100644
--- a/router/connection.go
+++ b/router/connection.go
@@ -298,11 +298,13 @@ func (conn *LocalConnection) run(actionChan <-chan ConnectionAction, finished ch
 	}
 	conn.Log("completed handshake")
 
-	if err = conn.initHeartbeats(); err != nil { // [1]
+	if err = conn.Router.Ourself.AddConnection(conn); err != nil { // [1]
 		return
 	}
 
-	conn.Router.Ourself.AddConnection(conn) // [1]
+	if err = conn.initHeartbeats(); err != nil { // [1]
+		return
+	}
 
 	go conn.receiveTCP(dec) // [1]
 
@@ -311,11 +313,14 @@ func (conn *LocalConnection) run(actionChan <-chan ConnectionAction, finished ch
 
 // [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.
+// (a) AddConnections must precede initHeartbeats. It is only after
+// the former completes that we know the connection is valid, in
+// particular is not a duplicate connection to the same peer. Sending
+// heartbeats on a duplicate connection can trip up crypto at the
+// other end, since the associated UDP packets may get decoded by the
+// other connection. It is also generally wasteful to engage in any
+// interaction with the remote on a connection that turns out to be
+// invald.
 //
 // (b) AddConnection must precede receiveTCP. In the absence of any
 // indirect connectivity to the remote peer, the first we hear about
@@ -332,7 +337,17 @@ func (conn *LocalConnection) run(actionChan <-chan ConnectionAction, finished ch
 // 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.
+// goroutine, at least not without some synchronisation. Which in turn
+// requires us the launching of the receiveTCP goroutine to precede
+// actorLoop.
+//
+// (d) AddConnection should precede receiveTCP. There is no point
+// starting the latter if the former fails.
+//
+// (e) initHeartbeats should precede actorLoop. The former is setting
+// LocalConnection fields accessed by the latter. Since the latter
+// runs in a separate goroutine, we'd have to add some synchronisation
+// if initHeartbeats isn't run first.
 
 func (conn *LocalConnection) initHeartbeats() error {
 	conn.heartbeatTCP = time.NewTicker(TCPHeartbeat)
diff --git a/router/local_peer.go b/router/local_peer.go
index 64f16c8f..69d71481 100644
--- a/router/local_peer.go
+++ b/router/local_peer.go
@@ -115,13 +115,12 @@ func (peer *LocalPeer) CreateConnection(peerAddr string, acceptNewPeer bool) err
 // ACTOR client API
 
 // Sync.
-func (peer *LocalPeer) AddConnection(conn *LocalConnection) {
-	resultChan := make(chan interface{})
+func (peer *LocalPeer) AddConnection(conn *LocalConnection) error {
+	resultChan := make(chan error)
 	peer.actionChan <- func() {
-		peer.handleAddConnection(conn)
-		resultChan <- nil
+		resultChan <- peer.handleAddConnection(conn)
 	}
-	<-resultChan
+	return <-resultChan
 }
 
 // Async.
@@ -155,7 +154,7 @@ func (peer *LocalPeer) actorLoop(actionChan <-chan LocalPeerAction) {
 	}
 }
 
-func (peer *LocalPeer) handleAddConnection(conn Connection) {
+func (peer *LocalPeer) handleAddConnection(conn Connection) error {
 	if peer.Peer != conn.Local() {
 		log.Fatal("Attempt made to add connection to peer where peer is not the source of connection")
 	}
@@ -167,26 +166,23 @@ func (peer *LocalPeer) handleAddConnection(conn Connection) {
 	// deliberately non symmetrical
 	if dupConn, found := peer.connections[toName]; found {
 		if dupConn == conn {
-			return
+			return nil
 		}
 		switch conn.BreakTie(dupConn) {
 		case TieBreakWon:
 			dupConn.Shutdown(dupErr)
 			peer.handleDeleteConnection(dupConn)
 		case TieBreakLost:
-			conn.Shutdown(dupErr)
-			return
+			return dupErr
 		case TieBreakTied:
 			// oh good grief. Sod it, just kill both of them.
-			conn.Shutdown(dupErr)
 			dupConn.Shutdown(dupErr)
 			peer.handleDeleteConnection(dupConn)
-			return
+			return dupErr
 		}
 	}
 	if err := peer.checkConnectionLimit(); err != nil {
-		conn.Shutdown(err)
-		return
+		return err
 	}
 	_, isConnectedPeer := peer.router.Routes.Unicast(toName)
 	peer.addConnection(conn)
@@ -197,6 +193,7 @@ func (peer *LocalPeer) handleAddConnection(conn Connection) {
 		peer.router.SendAllGossipDown(conn)
 	}
 	peer.broadcastPeerUpdate(conn.Remote())
+	return nil
 }
 
 func (peer *LocalPeer) handleConnectionEstablished(conn Connection) {