From 9d9768ee6fe603bfc8b5e412c302ea6cc1d40a50 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 16 Dec 2015 12:33:07 +0000 Subject: [PATCH 1/3] fix incorrect peer initialisation on test gossip connections We were creating peers with NewPeerFrom, which copies the Version (but not connections). That's not what happens for a real connection, where remote peers are always created with an initial version of 0. The mistake results in brokenness: when we subsequently receive a topology update for that peer with the same version then we ignore it, even though the update contains connection information which is new to us. Related, it is conceptually wrong to call FetchWithDefault on the remote router. That is not what happens for a real connection - the FetchWithDefault occurs in the symmetric counterpart. --- mesh/gossip_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/mesh/gossip_test.go b/mesh/gossip_test.go index 32ab6472..3d1b90f7 100644 --- a/mesh/gossip_test.go +++ b/mesh/gossip_test.go @@ -61,11 +61,9 @@ func AddTestGossipConnection(r1, r2 *Router) { } func (router *Router) NewTestGossipConnection(r *Router) *mockGossipConnection { - fromPeer := NewPeerFrom(router.Ourself.Peer) - toPeer := NewPeerFrom(r.Ourself.Peer) - - r.Peers.FetchWithDefault(fromPeer) // Has side-effect of incrementing refcount - toPeer = router.Peers.FetchWithDefault(toPeer) // + to := r.Ourself.Peer + toPeer := NewPeer(to.Name, to.NickName, to.UID, 0, to.ShortID) + toPeer = router.Peers.FetchWithDefault(toPeer) // Has side-effect of incrementing refcount return &mockGossipConnection{ RemoteConnection{router.Ourself.Peer, toPeer, "", false, true}, router, r, make(chan struct{})} From 5401037743f84809ff15713f734688b802113e5d Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 16 Dec 2015 12:45:12 +0000 Subject: [PATCH 2/3] refactor: inline mockGossipConnection.Connect Also change Start() order in AddTestGossipConnection to be more pleasing to the eye; the order isn't actually important. --- mesh/gossip_test.go | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/mesh/gossip_test.go b/mesh/gossip_test.go index 3d1b90f7..482dd6e1 100644 --- a/mesh/gossip_test.go +++ b/mesh/gossip_test.go @@ -13,9 +13,8 @@ import ( type mockGossipConnection struct { RemoteConnection - router *Router - dest *Router - start chan struct{} + dest *Router + start chan struct{} } func NewTestRouter(name string) *Router { @@ -32,11 +31,6 @@ func (conn *mockGossipConnection) SendProtocolMsg(protocolMsg ProtocolMsg) { } } -func (conn *mockGossipConnection) Connect() { - conn.router.Ourself.handleAddConnection(conn) - conn.router.Ourself.handleConnectionEstablished(conn) -} - func (conn *mockGossipConnection) Start() { close(conn.start) } @@ -54,10 +48,8 @@ func sendPendingGossip(routers ...*Router) { func AddTestGossipConnection(r1, r2 *Router) { c1 := r1.NewTestGossipConnection(r2) c2 := r2.NewTestGossipConnection(r1) - c1.Connect() - c2.Start() - c2.Connect() c1.Start() + c2.Start() } func (router *Router) NewTestGossipConnection(r *Router) *mockGossipConnection { @@ -65,8 +57,11 @@ func (router *Router) NewTestGossipConnection(r *Router) *mockGossipConnection { toPeer := NewPeer(to.Name, to.NickName, to.UID, 0, to.ShortID) toPeer = router.Peers.FetchWithDefault(toPeer) // Has side-effect of incrementing refcount - return &mockGossipConnection{ - RemoteConnection{router.Ourself.Peer, toPeer, "", false, true}, router, r, make(chan struct{})} + conn := &mockGossipConnection{ + RemoteConnection{router.Ourself.Peer, toPeer, "", false, true}, r, make(chan struct{})} + router.Ourself.handleAddConnection(conn) + router.Ourself.handleConnectionEstablished(conn) + return conn } func (router *Router) DeleteTestGossipConnection(r *Router) { From 81df06b3f4bdb2b2efe6332c0709df705e292320 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 16 Dec 2015 13:16:35 +0000 Subject: [PATCH 3/3] fix DeleteTestGossipConnection This is a one-sided operation, so we shouldn't be touching the remote's Peers. Also fix the associated test... When both 1 and 2 have lost their connection to 3, they no longer have a route to 3 and hence gc it. And because they no longer have a route, gossip from them won't get through to 3. --- mesh/gossip_test.go | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/mesh/gossip_test.go b/mesh/gossip_test.go index 482dd6e1..d5e15afc 100644 --- a/mesh/gossip_test.go +++ b/mesh/gossip_test.go @@ -65,16 +65,9 @@ func (router *Router) NewTestGossipConnection(r *Router) *mockGossipConnection { } func (router *Router) DeleteTestGossipConnection(r *Router) { - fromName := router.Ourself.Peer.Name toName := r.Ourself.Peer.Name - - fromPeer := r.Peers.Fetch(fromName) - toPeer := router.Peers.Fetch(toName) - - r.Peers.Dereference(fromPeer) - router.Peers.Dereference(toPeer) - conn, _ := router.Ourself.ConnectionTo(toName) + router.Peers.Dereference(conn.Remote()) router.Ourself.handleDeleteConnection(conn) } @@ -136,15 +129,10 @@ func TestGossipTopology(t *testing.T) { // Drop the connection from 1 to 3 r1.DeleteTestGossipConnection(r3) sendPendingGossip(r1, r2, r3) - checkTopology(t, r1, r1.tp(r2), r2.tp(r1), r3.tp(r1, r2)) - checkTopology(t, r2, r1.tp(r2), r2.tp(r1), r3.tp(r1, r2)) + checkTopology(t, r1, r1.tp(r2), r2.tp(r1)) + checkTopology(t, r2, r1.tp(r2), r2.tp(r1)) // r3 still thinks r1 has a connection to it checkTopology(t, r3, r1.tp(r2, r3), r2.tp(r1), r3.tp(r1, r2)) - - // On a timer, r3 will gossip to r1 - r3.SendAllGossip() - sendPendingGossip(r1, r2, r3) - checkTopology(t, r1, r1.tp(r2), r2.tp(r1), r3.tp(r1, r2)) } func TestGossipSurrogate(t *testing.T) {