Merge pull request #253 from rade/refactor_and_explain_route_calculation

LGTM, allowing that the results are unbalanced (and unchanged in this respect from before this PR)

router/peer.go

@@ -65,42 +65,71 @@ func (peer *Peer) ForEachConnection(fun func(PeerName, Connection)) {
 	}
 }
 
-func (peer *Peer) HasPathTo(target *Peer, symmetric bool) (bool, map[PeerName]bool) {
+// Calculate the routing table from this peer to all peers reachable
+// from it, returning a "next hop" map of PeerNameX -> PeerNameY,
+// which says "in order to send a message to X, the peer should send
+// the message to its neighbour Y".
+//
+// Because currently we do not have weightings on the connections
+// between peers, there is no need to use a minimum spanning tree
+// algorithm. Instead we employ the simpler and cheaper breadth-first
+// widening. The computation is deterministic, which ensures that when
+// it is performed on the same data by different peers, they get the
+// same result. This is important since otherwise we risk message loss
+// or routing cycles.
+//
+// When the 'symmetric' flag is set, only symmetric connections are
+// considered, i.e. where both sides indicate they have a connection
+// to the other.
+//
+// When a non-nil stopAt peer is supplied, the widening stops when it
+// reaches that peer. The boolean return indicates whether that has
+// happened.
+//
+// We acquire read locks on peers as we encounter them during the
+// traversal. This prevents the connectivity graph from changing
+// underneath us in ways that would invalidate the result. Thus the
+// answer returned may be out of date, but never inconsistent.
+func (peer *Peer) Routes(stopAt *Peer, symmetric bool) (bool, map[PeerName]PeerName) {
 	peer.RLock()
 	defer peer.RUnlock()
-	reached := make(map[PeerName]bool)
+	routes := make(map[PeerName]PeerName)
+	routes[peer.Name] = UnknownPeerName
 	nextWorklist := []*Peer{peer}
-	reached[peer.Name] = true
 	for len(nextWorklist) > 0 {
 		worklist := nextWorklist
 		sort.Sort(ListOfPeers(worklist))
 		nextWorklist = []*Peer{}
 		for _, curPeer := range worklist {
-			if curPeer == target {
-				return true, reached
+			if curPeer == stopAt {
+				return true, routes
 			}
+			curName := curPeer.Name
 			for remoteName, conn := range curPeer.connections {
-				if _, found := reached[remoteName]; found {
+				if _, found := routes[remoteName]; found {
 					continue
 				}
-				// By taking this lock, we ensure no changes can occur
-				// to any peer we have reached. As a result, if/when
-				// we reach the target, we know the entire path from
-				// peer to target is valid - no changes can have been
-				// made to that path during our calculation.
 				remote := conn.Remote()
 				remote.RLock()
-				if _, found := remote.connections[curPeer.Name]; !symmetric || found {
+				if _, found := remote.connections[curName]; !symmetric || found {
 					defer remote.RUnlock()
 					nextWorklist = append(nextWorklist, remote)
-					reached[remoteName] = true
+					// We now know how to get to remoteName: the same
+					// way we get to curPeer. Except, if curPeer is
+					// the starting peer in which case we know we can
+					// reach remoteName directly.
+					if curPeer == peer {
+						routes[remoteName] = remoteName
+					} else {
+						routes[remoteName] = routes[curName]
+					}
 				} else {
 					remote.RUnlock()
 				}
 			}
 		}
 	}
-	return false, reached
+	return false, routes
 }
 
 func (peer *Peer) SetVersionAndConnections(version uint64, connections map[PeerName]Connection) {

router/peer_cache.go

@@ -131,7 +131,7 @@ func (cache *PeerCache) fetchAlias(peer *Peer) (*Peer, bool) {
 func (cache *PeerCache) garbageCollect(ourself *Peer) []*Peer {
 	removed := []*Peer{}
 	for name, peer := range cache.table {
-		found, _ := ourself.HasPathTo(peer, false)
+		found, _ := ourself.Routes(peer, false)
 		if !found && !peer.IsLocallyReferenced() {
 			cache.onGC(peer)
 			delete(cache.table, name)

router/topology.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"fmt"
 	"log"
-	"sort"
 )
 
 func (topo *Topology) Unicast(name PeerName) (PeerName, bool) {
@@ -97,69 +96,45 @@ func (topo *Topology) queryLoop(queryChan <-chan *Interaction) {
 	}
 }
 
+// Calculate all the routes for the question: if *we* want to send a
+// packet to Peer X, what is the next hop?
+//
+// When we sniff a packet, we determine the destination peer
+// ourself. Consequently, we can relay the packet via any
+// arbitrary peers - the intermediate peers do not have to have
+// any knowledge of the MAC address at all. Thus there's no need
+// to exchange knowledge of MAC addresses, nor any constraints on
+// the routes that we construct.
 func (topo *Topology) buildUnicastRoutes() map[PeerName]PeerName {
-	// He we are calculating all the routes for the question: if *we*
-	// want to send a packet to Peer X, what is the next hop?
-	//
-	// When we sniff a packet, we determine the destination peer
-	// ourself. Consequently, we can relay the packet via any
-	// arbitrary peers - the intermediate peers do not have to have
-	// any knowledge of the MAC address at all. Thus there's no need
-	// to exchange knowledge of MAC addresses, nor any constraints on
-	// the routes that we construct.
-	//
-	// Because currently we do not have weightings on the connections
-	// between peers, there is no need to use Dijkstra's
-	// algorithm. Instead, we can use the simpler and cheaper
-	// breadth-first widening.
-	//
-	// Currently we make no attempt to perform any balancing of any
-	// sort. That could happen in the future.
-
-	unicast := make(map[PeerName]PeerName)
-	ourself := topo.router.Ourself
-	unicast[ourself.Name] = UnknownPeerName
-	nextWorklist := []*Peer{}
-	ourself.ForEachConnection(func(remoteName PeerName, conn Connection) {
-		unicast[remoteName] = remoteName
-		nextWorklist = append(nextWorklist, conn.Remote())
-	})
-	for len(nextWorklist) > 0 {
-		worklist := nextWorklist
-		sort.Sort(ListOfPeers(worklist))
-		nextWorklist = []*Peer{}
-		for _, peer := range worklist {
-			localName := peer.Name
-			peer.ForEachConnection(func(remoteName PeerName, conn Connection) {
-				if _, found := unicast[remoteName]; found {
-					return
-				}
-				// We now know how to get to remoteName: the same
-				// way we get to peer. However, we only record this as
-				// a valid path if remoteName has peer in its
-				// connections. I.e. it must be a symmetric
-				// connection.
-				if _, found := conn.Remote().ConnectionTo(localName); found {
-					unicast[remoteName] = unicast[localName]
-					nextWorklist = append(nextWorklist, conn.Remote())
-				}
-			})
-		}
-	}
-	return unicast
+	_, routes := topo.router.Ourself.Routes(nil, true)
+	return routes
 }
 
+// Calculate all the routes for the question: if we receive a
+// broadcast originally from Peer X, which peers should we pass the
+// frames on to?
+//
+// When the topology is stable, and thus all peers perform route
+// calculations based on the same data, the algorithm ensures that
+// broadcasts reach every peer exactly once.
+//
+// This is largely due to properties of the Peer.Routes algorithm. In
+// particular:
+//
+// ForAll X,Y,Z in Peers.
+//     X.Routes(Y) <= X.Routes(Z) \/
+//     X.Routes(Z) <= X.Routes(Y)
+// ForAll X,Y,Z in Peers.
+//     Y =/= Z /\ X.Routes(Y) <= X.Routes(Z) =>
+//     X.Routes(Y) u [P | Y.HasSymmetricConnectionTo(P)] <= X.Routes(Z)
+// where <= is the subset relationship on keys of the returned map.
 func (topo *Topology) buildBroadcastRoutes() map[PeerName][]PeerName {
-	// For broadcast, we need to construct which of our connected
-	// peers we should pass frames on to should we receive any
-	// broadcast originally from peer X
 	broadcast := make(map[PeerName][]PeerName)
 	ourself := topo.router.Ourself
 
 	topo.router.Peers.ForEach(func(name PeerName, peer *Peer) {
 		hops := []PeerName{}
-		found, reached := peer.HasPathTo(ourself, true)
-		if found {
+		if found, reached := peer.Routes(ourself, true); found {
 			ourself.ForEachConnection(func(remoteName PeerName, conn Connection) {
 				if _, found := reached[remoteName]; found {
 					return