From 6488b0d11cd131d5c0f2ce0bc892d425024451d8 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 25 Jun 2015 16:45:18 +0100 Subject: [PATCH 1/5] More idiomatic exitChan --- ipam/testutils_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ipam/testutils_test.go b/ipam/testutils_test.go index 6768a121..2ffac518 100644 --- a/ipam/testutils_test.go +++ b/ipam/testutils_test.go @@ -205,7 +205,7 @@ type gossipMessage struct { sender *router.PeerName buf []byte // for unicast data router.GossipData // for broadcast - exitChan chan bool + exitChan chan struct{} } type TestGossipRouter struct { @@ -225,7 +225,7 @@ func (grouter *TestGossipRouter) GossipBroadcast(update router.GossipData) error func (grouter *TestGossipRouter) removePeer(peer router.PeerName) { gossipChan := grouter.gossipChans[peer] - resultChan := make(chan bool) + resultChan := make(chan struct{}) gossipChan <- gossipMessage{exitChan: resultChan} <-resultChan delete(grouter.gossipChans, peer) @@ -245,7 +245,7 @@ func (grouter *TestGossipRouter) connect(sender router.PeerName, gossiper router select { case message := <-gossipChan: if message.exitChan != nil { - message.exitChan <- true + close(message.exitChan) return } From 959e5f0082b42d9919df40f80c1c6fa89d6499b2 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 25 Jun 2015 17:04:07 +0100 Subject: [PATCH 2/5] Use different object types for mock gossip messages --- ipam/allocator_test.go | 2 +- ipam/testutils_test.go | 38 ++++++++++++++++++++------------------ 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index d32e9174..2aaece78 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -168,7 +168,7 @@ func TestCancel(t *testing.T) { CIDR = "10.0.1.7/26" ) - router := TestGossipRouter{make(map[router.PeerName]chan gossipMessage), 0.0} + router := TestGossipRouter{make(map[router.PeerName]chan interface{}), 0.0} alloc1, subnet := makeAllocator("01:00:00:02:00:00", CIDR, 2) alloc1.SetInterfaces(router.connect(alloc1.ourName, alloc1)) diff --git a/ipam/testutils_test.go b/ipam/testutils_test.go index 2ffac518..fb2af1fd 100644 --- a/ipam/testutils_test.go +++ b/ipam/testutils_test.go @@ -200,23 +200,26 @@ func AssertNothingSentErr(t *testing.T, ch <-chan error) { } // Router to convey gossip from one gossiper to another, for testing -type gossipMessage struct { - isUnicast bool - sender *router.PeerName - buf []byte // for unicast - data router.GossipData // for broadcast - exitChan chan struct{} +type unicastMessage struct { + sender *router.PeerName + buf []byte +} +type broadcastMessage struct { + data router.GossipData +} +type exitMessage struct { + exitChan chan struct{} } type TestGossipRouter struct { - gossipChans map[router.PeerName]chan gossipMessage + gossipChans map[router.PeerName]chan interface{} loss float32 // 0.0 means no loss } func (grouter *TestGossipRouter) GossipBroadcast(update router.GossipData) error { for _, gossipChan := range grouter.gossipChans { select { - case gossipChan <- gossipMessage{data: update}: + case gossipChan <- broadcastMessage{data: update}: default: // drop the message if we cannot send it } } @@ -226,7 +229,7 @@ func (grouter *TestGossipRouter) GossipBroadcast(update router.GossipData) error func (grouter *TestGossipRouter) removePeer(peer router.PeerName) { gossipChan := grouter.gossipChans[peer] resultChan := make(chan struct{}) - gossipChan <- gossipMessage{exitChan: resultChan} + gossipChan <- exitMessage{exitChan: resultChan} <-resultChan delete(grouter.gossipChans, peer) } @@ -237,27 +240,26 @@ type TestGossipRouterClient struct { } func (grouter *TestGossipRouter) connect(sender router.PeerName, gossiper router.Gossiper) router.Gossip { - gossipChan := make(chan gossipMessage, 100) + gossipChan := make(chan interface{}, 100) go func() { gossipTimer := time.Tick(10 * time.Second) for { select { - case message := <-gossipChan: - if message.exitChan != nil { + case gossip := <-gossipChan: + if message, isExit := gossip.(exitMessage); isExit { close(message.exitChan) return } - if rand.Float32() > (1.0 - grouter.loss) { continue } - - if message.isUnicast { + switch message := gossip.(type) { + case unicastMessage: if err := gossiper.OnGossipUnicast(*message.sender, message.buf); err != nil { panic(fmt.Sprintf("Error doing gossip unicast to %s: %s", sender, err)) } - } else { + case broadcastMessage: for _, msg := range message.data.Encode() { if _, err := gossiper.OnGossipBroadcast(msg); err != nil { panic(fmt.Sprintf("Error doing gossip broadcast to %s: %s", sender, err)) @@ -276,7 +278,7 @@ func (grouter *TestGossipRouter) connect(sender router.PeerName, gossiper router func (client TestGossipRouterClient) GossipUnicast(dstPeerName router.PeerName, buf []byte) error { select { - case client.router.gossipChans[dstPeerName] <- gossipMessage{isUnicast: true, sender: &client.sender, buf: buf}: + case client.router.gossipChans[dstPeerName] <- unicastMessage{sender: &client.sender, buf: buf}: default: // drop the message if we cannot send it common.Error.Printf("Dropping message") } @@ -289,7 +291,7 @@ func (client TestGossipRouterClient) GossipBroadcast(update router.GossipData) e func makeNetworkOfAllocators(size int, cidr string) ([]*Allocator, TestGossipRouter, address.Range) { - gossipRouter := TestGossipRouter{make(map[router.PeerName]chan gossipMessage), 0.0} + gossipRouter := TestGossipRouter{make(map[router.PeerName]chan interface{}), 0.0} allocs := make([]*Allocator, size) var subnet address.Range From c659e3448ad551996ff2a235a43f20efea3a2da4 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 25 Jun 2015 17:38:02 +0100 Subject: [PATCH 3/5] Add a flush() function to the mock gossip router, and call it where required --- ipam/allocator_test.go | 4 ++++ ipam/testutils_test.go | 16 +++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index 2aaece78..6cc9aae5 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -256,13 +256,17 @@ func TestTransfer(t *testing.T) { wt.AssertTrue(t, err == nil, "Failed to get address") router.GossipBroadcast(alloc2.Gossip()) + router.flush() router.GossipBroadcast(alloc3.Gossip()) + router.flush() router.removePeer(alloc2.ourName) router.removePeer(alloc3.ourName) alloc2.Stop() alloc3.Stop() + router.flush() wt.AssertSuccess(t, alloc1.AdminTakeoverRanges(alloc2.ourName.String())) wt.AssertSuccess(t, alloc1.AdminTakeoverRanges(alloc3.ourName.String())) + router.flush() wt.AssertEquals(t, alloc1.NumFreeAddresses(subnet), address.Offset(1022)) diff --git a/ipam/testutils_test.go b/ipam/testutils_test.go index fb2af1fd..8448c2a2 100644 --- a/ipam/testutils_test.go +++ b/ipam/testutils_test.go @@ -210,6 +210,9 @@ type broadcastMessage struct { type exitMessage struct { exitChan chan struct{} } +type flushMessage struct { + flushChan chan struct{} +} type TestGossipRouter struct { gossipChans map[router.PeerName]chan interface{} @@ -226,6 +229,14 @@ func (grouter *TestGossipRouter) GossipBroadcast(update router.GossipData) error return nil } +func (grouter *TestGossipRouter) flush() { + for _, gossipChan := range grouter.gossipChans { + flushChan := make(chan struct{}) + gossipChan <- flushMessage{flushChan: flushChan} + <-flushChan + } +} + func (grouter *TestGossipRouter) removePeer(peer router.PeerName) { gossipChan := grouter.gossipChans[peer] resultChan := make(chan struct{}) @@ -251,6 +262,9 @@ func (grouter *TestGossipRouter) connect(sender router.PeerName, gossiper router close(message.exitChan) return } + if message, isFlush := gossip.(flushMessage); isFlush { + close(message.flushChan) + } if rand.Float32() > (1.0 - grouter.loss) { continue } @@ -305,7 +319,7 @@ func makeNetworkOfAllocators(size int, cidr string) ([]*Allocator, TestGossipRou } gossipRouter.GossipBroadcast(allocs[size-1].Gossip()) - time.Sleep(1000 * time.Millisecond) + gossipRouter.flush() return allocs, gossipRouter, subnet } From 6725c8355f886e65518e0ef9c2073e05b6bb7ce0 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 25 Jun 2015 17:41:18 +0100 Subject: [PATCH 4/5] Check the result of Allocate in TestFakeRouterSimple() --- ipam/allocator_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index 6cc9aae5..33bed602 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -286,7 +286,8 @@ func TestFakeRouterSimple(t *testing.T) { alloc1 := allocs[0] //alloc2 := allocs[1] - alloc1.Allocate("foo", subnet, nil) + _, err := alloc1.Allocate("foo", subnet, nil) + wt.AssertTrue(t, err == nil, "Failed to get address") } func TestAllocatorFuzz(t *testing.T) { From c84c5dbcdfa4841b7b78519241fa721eb4cdfc89 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 25 Jun 2015 18:13:43 +0100 Subject: [PATCH 5/5] Make TestGossipRouter goroutine easier to follow --- ipam/testutils_test.go | 64 +++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/ipam/testutils_test.go b/ipam/testutils_test.go index 8448c2a2..73334e2d 100644 --- a/ipam/testutils_test.go +++ b/ipam/testutils_test.go @@ -201,7 +201,7 @@ func AssertNothingSentErr(t *testing.T, ch <-chan error) { // Router to convey gossip from one gossiper to another, for testing type unicastMessage struct { - sender *router.PeerName + sender router.PeerName buf []byte } type broadcastMessage struct { @@ -250,41 +250,47 @@ type TestGossipRouterClient struct { sender router.PeerName } -func (grouter *TestGossipRouter) connect(sender router.PeerName, gossiper router.Gossiper) router.Gossip { - gossipChan := make(chan interface{}, 100) +func (grouter *TestGossipRouter) run(gossiper router.Gossiper, gossipChan chan interface{}) { + gossipTimer := time.Tick(10 * time.Second) + for { + select { + case gossip := <-gossipChan: + switch message := gossip.(type) { + case exitMessage: + close(message.exitChan) + return - go func() { - gossipTimer := time.Tick(10 * time.Second) - for { - select { - case gossip := <-gossipChan: - if message, isExit := gossip.(exitMessage); isExit { - close(message.exitChan) - return - } - if message, isFlush := gossip.(flushMessage); isFlush { - close(message.flushChan) - } + case flushMessage: + close(message.flushChan) + + case unicastMessage: if rand.Float32() > (1.0 - grouter.loss) { continue } - switch message := gossip.(type) { - case unicastMessage: - if err := gossiper.OnGossipUnicast(*message.sender, message.buf); err != nil { - panic(fmt.Sprintf("Error doing gossip unicast to %s: %s", sender, err)) - } - case broadcastMessage: - for _, msg := range message.data.Encode() { - if _, err := gossiper.OnGossipBroadcast(msg); err != nil { - panic(fmt.Sprintf("Error doing gossip broadcast to %s: %s", sender, err)) - } + if err := gossiper.OnGossipUnicast(message.sender, message.buf); err != nil { + panic(fmt.Sprintf("Error doing gossip unicast to %s: %s", message.sender, err)) + } + + case broadcastMessage: + if rand.Float32() > (1.0 - grouter.loss) { + continue + } + for _, msg := range message.data.Encode() { + if _, err := gossiper.OnGossipBroadcast(msg); err != nil { + panic(fmt.Sprintf("Error doing gossip broadcast: %s", err)) } } - case <-gossipTimer: - grouter.GossipBroadcast(gossiper.Gossip()) } + case <-gossipTimer: + grouter.GossipBroadcast(gossiper.Gossip()) } - }() + } +} + +func (grouter *TestGossipRouter) connect(sender router.PeerName, gossiper router.Gossiper) router.Gossip { + gossipChan := make(chan interface{}, 100) + + go grouter.run(gossiper, gossipChan) grouter.gossipChans[sender] = gossipChan return TestGossipRouterClient{grouter, sender} @@ -292,7 +298,7 @@ func (grouter *TestGossipRouter) connect(sender router.PeerName, gossiper router func (client TestGossipRouterClient) GossipUnicast(dstPeerName router.PeerName, buf []byte) error { select { - case client.router.gossipChans[dstPeerName] <- unicastMessage{sender: &client.sender, buf: buf}: + case client.router.gossipChans[dstPeerName] <- unicastMessage{sender: client.sender, buf: buf}: default: // drop the message if we cannot send it common.Error.Printf("Dropping message") }