fix bug in peer eviction (#87)

* This is the fix for the bug wherein we don't fire PeerRemoved notifications when we replace "less useful" peers in a bucket.

table.go

@@ -201,7 +201,7 @@ func (rt *RoutingTable) addPeer(p peer.ID, queryPeer bool) (bool, error) {
 
 	if time.Since(minLast.LastUsefulAt) > rt.usefulnessGracePeriod {
 		// let's evict it and add the new peer
-		if bucket.remove(minLast.Id) {
+		if rt.removePeer(minLast.Id) {
 			bucket.pushFront(&PeerInfo{
 				Id:                            p,
 				LastUsefulAt:                  lastUsefulAt,
@@ -273,7 +273,7 @@ func (rt *RoutingTable) RemovePeer(p peer.ID) {
 }
 
 // locking is the responsibility of the caller
-func (rt *RoutingTable) removePeer(p peer.ID) {
+func (rt *RoutingTable) removePeer(p peer.ID) bool {
 	bucketID := rt.bucketIdForPeer(p)
 	bucket := rt.buckets[bucketID]
 	if bucket.remove(p) {
@@ -296,7 +296,9 @@ func (rt *RoutingTable) removePeer(p peer.ID) {
 
 		// peer removed callback
 		rt.PeerRemoved(p)
+		return true
 	}
+	return false
 }
 
 func (rt *RoutingTable) nextBucket() {

table_test.go

@@ -593,6 +593,48 @@ func TestGetPeerInfos(t *testing.T) {
 	require.False(t, ms[p2].LastUsefulAt.IsZero())
 }
 
+func TestPeerRemovedNotificationWhenPeerIsEvicted(t *testing.T) {
+	t.Parallel()
+
+	local := test.RandPeerIDFatal(t)
+	m := pstore.NewMetrics()
+	rt, err := NewRoutingTable(1, ConvertPeerID(local), time.Hour, m, 1*time.Hour)
+	require.NoError(t, err)
+	pset := make(map[peer.ID]struct{})
+	rt.PeerAdded = func(p peer.ID) {
+		pset[p] = struct{}{}
+	}
+	rt.PeerRemoved = func(p peer.ID) {
+		delete(pset, p)
+	}
+
+	p1, _ := rt.GenRandPeerID(0)
+	p2, _ := rt.GenRandPeerID(0)
+
+	// first peer works
+	b, err := rt.TryAddPeer(p1, true)
+	require.NoError(t, err)
+	require.True(t, b)
+
+	// second is rejected because of capacity
+	b, err = rt.TryAddPeer(p2, true)
+	require.False(t, b)
+	require.Error(t, err)
+
+	// pset has first peer
+	require.Contains(t, pset, p1)
+	require.NotContains(t, pset, p2)
+
+	// update last useful at so it's ripe for eviction.
+	require.True(t, rt.UpdateLastUsefulAt(p1, time.Now().AddDate(-1, 0, 0)))
+
+	b, err = rt.TryAddPeer(p2, true)
+	require.NoError(t, err)
+	require.True(t, b)
+	require.Contains(t, pset, p2)
+	require.NotContains(t, pset, p1)
+}
+
 func BenchmarkAddPeer(b *testing.B) {
 	b.StopTimer()
 	local := ConvertKey("localKey")