p2p: fully test PeerSet, more docs, parallelize PeerSet tests

* Full test PeerSet and check its concurrent guarantees
* Improve the doc for PeerSet.Has and remove unnecessary
defer for a path that sets a variable, make it fast anyways.
* Parallelize PeerSet tests with t.Parallel()
* Document functions in peer_set.go more.

p2p/peer_set.go

@@ -27,6 +27,7 @@ type peerSetItem struct {
 	index int
 }
 
+// NewPeerSet creates a new peerSet with a list of initial capacity of 256 items.
 func NewPeerSet() *PeerSet {
 	return &PeerSet{
 		lookup: make(map[string]*peerSetItem),
@@ -34,7 +35,8 @@ func NewPeerSet() *PeerSet {
 	}
 }
 
-// Returns false if peer with key (PubKeyEd25519) is already set
+// Add returns false if the peer's Key (PubKeyEd25519) is already memoized.
+// If the peer was already added, it returns ErrSwitchDuplicatePeer.
 func (ps *PeerSet) Add(peer *Peer) error {
 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()
@@ -50,13 +52,16 @@ func (ps *PeerSet) Add(peer *Peer) error {
 	return nil
 }
 
+// Has returns true iff the peerset contains
+// the peer referred to by this peerKey.
 func (ps *PeerSet) Has(peerKey string) bool {
 	ps.mtx.Lock()
-	defer ps.mtx.Unlock()
 	_, ok := ps.lookup[peerKey]
+	ps.mtx.Unlock()
 	return ok
 }
 
+// Get looks up a peer by the provided peerKey.
 func (ps *PeerSet) Get(peerKey string) *Peer {
 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()
@@ -68,6 +73,7 @@ func (ps *PeerSet) Get(peerKey string) *Peer {
 	}
 }
 
+// Remove discards peer by its Key, if the peer was previously memoized.
 func (ps *PeerSet) Remove(peer *Peer) {
 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()
@@ -77,8 +83,8 @@ func (ps *PeerSet) Remove(peer *Peer) {
 	}
 
 	index := item.index
-	// Copy the list but without the last element.
-	// (we must copy because we're mutating the list)
+	// Create a new copy of the list but with one less item.
+	// (we must copy because we'll be mutating the list).
 	newList := make([]*Peer, len(ps.list)-1)
 	copy(newList, ps.list)
 	// If it's the last peer, that's an easy special case.
@@ -88,7 +94,7 @@ func (ps *PeerSet) Remove(peer *Peer) {
 		return
 	}
 
-	// Move the last item from ps.list to "index" in list.
+	// Replace the popped item with the last item in the old list.
 	lastPeer := ps.list[len(ps.list)-1]
 	lastPeerKey := lastPeer.Key
 	lastPeerItem := ps.lookup[lastPeerKey]
@@ -96,16 +102,16 @@ func (ps *PeerSet) Remove(peer *Peer) {
 	lastPeerItem.index = index
 	ps.list = newList
 	delete(ps.lookup, peer.Key)
-
 }
 
+// Size returns the number of unique items in the peerSet.
 func (ps *PeerSet) Size() int {
 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()
 	return len(ps.list)
 }
 
-// threadsafe list of peers.
+// List returns the threadsafe list of peers.
 func (ps *PeerSet) List() []*Peer {
 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()

p2p/peer_set_test.go

@@ -2,8 +2,11 @@ package p2p
 
 import (
 	"math/rand"
+	"sync"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	cmn "github.com/tendermint/tmlibs/common"
 )
 
@@ -18,28 +21,47 @@ func randPeer() *Peer {
 	}
 }
 
-func TestAddRemoveOne(t *testing.T) {
+func TestPeerSetAddRemoveOne(t *testing.T) {
+	t.Parallel()
 	peerSet := NewPeerSet()
 
-	peer := randPeer()
-	err := peerSet.Add(peer)
-	if err != nil {
-		t.Errorf("Failed to add new peer")
+	var peerList []*Peer
+	for i := 0; i < 5; i++ {
+		p := randPeer()
+		peerSet.Add(p)
+		peerList = append(peerList, p)
 	}
-	if peerSet.Size() != 1 {
-		t.Errorf("Failed to add new peer and increment size")
+
+	n := len(peerList)
+	// 1. Test removing from the front
+	for i, peerAtFront := range peerList {
+		peerSet.Remove(peerAtFront)
+		wantSize := n - i - 1
+		for j := 0; j < 2; j++ {
+			assert.Equal(t, false, peerSet.Has(peerAtFront.Key), "#%d Run #%d: failed to remove peer", i, j)
+			assert.Equal(t, wantSize, peerSet.Size(), "#%d Run #%d: failed to remove peer and decrement size", i, j)
+			// Test the route of removing the now non-existent element
+			peerSet.Remove(peerAtFront)
+		}
 	}
 
-	peerSet.Remove(peer)
-	if peerSet.Has(peer.Key) {
-		t.Errorf("Failed to remove peer")
+	// 2. Next we are testing removing the peer at the end
+	// a) Replenish the peerSet
+	for _, peer := range peerList {
+		peerSet.Add(peer)
 	}
-	if peerSet.Size() != 0 {
-		t.Errorf("Failed to remove peer and decrement size")
+
+	// b) In reverse, remove each element
+	for i := n - 1; i >= 0; i-- {
+		peerAtEnd := peerList[i]
+		peerSet.Remove(peerAtEnd)
+		assert.Equal(t, false, peerSet.Has(peerAtEnd.Key), "#%d: failed to remove item at end", i)
+		assert.Equal(t, i, peerSet.Size(), "#%d: differing sizes after peerSet.Remove(atEndPeer)", i)
 	}
 }
 
-func TestAddRemoveMany(t *testing.T) {
+func TestPeerSetAddRemoveMany(t *testing.T) {
+	t.Parallel()
 	peerSet := NewPeerSet()
 
 	peers := []*Peer{}
@@ -65,3 +87,61 @@ func TestAddRemoveMany(t *testing.T) {
 		}
 	}
 }
+
+func TestPeerSetAddDuplicate(t *testing.T) {
+	t.Parallel()
+	peerSet := NewPeerSet()
+	peer := randPeer()
+
+	n := 20
+	errsChan := make(chan error)
+	// Add the same asynchronously to test the
+	// concurrent guarantees of our APIs, and
+	// our expectation in the end is that only
+	// one addition succeeded, but the rest are
+	// instances of ErrSwitchDuplicatePeer.
+	for i := 0; i < n; i++ {
+		go func() {
+			errsChan <- peerSet.Add(peer)
+		}()
+	}
+
+	// Now collect and tally the results
+	errsTally := make(map[error]int)
+	for i := 0; i < n; i++ {
+		err := <-errsChan
+		errsTally[err] += 1
+	}
+
+	// Our next procedure is to ensure that only one addition
+	// succeeded and that the rest are each ErrSwitchDuplicatePeer.
+	wantErrCount, gotErrCount := n-1, errsTally[ErrSwitchDuplicatePeer]
+	assert.Equal(t, wantErrCount, gotErrCount, "invalid ErrSwitchDuplicatePeer count")
+
+	wantNilErrCount, gotNilErrCount := 1, errsTally[nil]
+	assert.Equal(t, wantNilErrCount, gotNilErrCount, "invalid nil errCount")
+}
+
+func TestPeerSetGet(t *testing.T) {
+	t.Parallel()
+	peerSet := NewPeerSet()
+	peer := randPeer()
+	assert.Nil(t, peerSet.Get(peer.Key), "expecting a nil lookup, before .Add")
+
+	if err := peerSet.Add(peer); err != nil {
+		t.Fatalf("Failed to add new peer: %v", err)
+	}
+
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		// Add them asynchronously to test the
+		// concurrent guarantees of our APIs.
+		wg.Add(1)
+		go func(i int) {
+			defer wg.Done()
+			got, want := peerSet.Get(peer.Key), peer
+			assert.Equal(t, got, want, "#%d: got=%v want=%v", i, got, want)
+		}(i)
+	}
+	wg.Wait()
+}