From 1fdd87ee2554003f6165fd8046947540369db4d6 Mon Sep 17 00:00:00 2001
From: Manohar HT <manohar.ht@vimeo.com>
Date: Fri, 16 Dec 2022 22:08:08 +0530
Subject: [PATCH] * deep copy of fetchers map * use read lock * add checks
 inside incremental peers test for listPeers method * fix doc comments for
 ListPeers

---
 galaxycache.go |  3 ++-
 peers.go       | 12 +++++++++---
 peers_test.go  | 27 +++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/galaxycache.go b/galaxycache.go
index b51965ba..ca89c913 100644
--- a/galaxycache.go
+++ b/galaxycache.go
@@ -232,7 +232,8 @@ func (universe *Universe) RemovePeers(ids ...string) error {
 	return universe.peerPicker.remove(ids...)
 }
 
-// ListPeers returns Universe's remote fetchers map keyed by peer IDs
+// ListPeers returns a map of remote fetchers keyed by Peer ID,
+// useful for testing incremental changes to galaxycache peers.
 func (universe *Universe) ListPeers() map[string]RemoteFetcher {
 	return universe.peerPicker.listPeers()
 }
diff --git a/peers.go b/peers.go
index 60c315ae..0a127599 100644
--- a/peers.go
+++ b/peers.go
@@ -396,9 +396,15 @@ func (pp *PeerPicker) remove(ids ...string) error {
 }
 
 func (pp *PeerPicker) listPeers() map[string]RemoteFetcher {
-	pp.mu.Lock()
-	defer pp.mu.Unlock()
-	fetchers := pp.fetchers
+	pp.mu.RLock()
+	defer pp.mu.RUnlock()
+
+	// deep copy of pp.fetchers map.
+	fetchers := make(map[string]RemoteFetcher)
+	for p, f := range pp.fetchers {
+		fetchers[p] = f
+	}
+
 	return fetchers
 }
 
diff --git a/peers_test.go b/peers_test.go
index 5c709a30..c3636e70 100644
--- a/peers_test.go
+++ b/peers_test.go
@@ -244,6 +244,14 @@ func TestPeersIncremental(t *testing.T) {
 					fetcherURIs[f.(*TestFetcher).uri] = struct{}{}
 				}
 
+				allFetchers := u.peerPicker.listPeers()
+				allPeerIDs := make(map[string]struct{}, len(allFetchers))
+				allFetcherURIs := make(map[string]struct{}, len(allFetchers))
+				for peerID, fetcher := range allFetchers {
+					allPeerIDs[peerID] = struct{}{}
+					allFetcherURIs[fetcher.(*TestFetcher).uri] = struct{}{}
+				}
+
 				for _, expPeer := range step.expectedPeers {
 					if _, ok := allPeers[expPeer.ID]; !ok {
 						t.Errorf("missing peer %q from hashring at step %d", expPeer.ID, si)
@@ -258,6 +266,17 @@ func TestPeersIncremental(t *testing.T) {
 							expPeer.ID, expPeer.URI, si)
 					}
 					delete(fetcherURIs, expPeer.URI)
+					// Checks for peer IDs and fetchers from listPeers method.
+					if _, ok := allPeerIDs[expPeer.ID]; !ok {
+						t.Errorf("missing peer %q from copy of fetchers map keys at step %d",
+							expPeer.ID, si)
+					}
+					delete(allPeerIDs, expPeer.ID)
+					if _, ok := allFetcherURIs[expPeer.URI]; !ok {
+						t.Errorf("missing peer %q (URI %q) from copy of fetchers values at step %d",
+							expPeer.ID, expPeer.URI, si)
+					}
+					delete(allFetcherURIs, expPeer.URI)
 				}
 				if step.includeSelf {
 					if _, ok := allPeers[selfID]; !ok {
@@ -274,6 +293,14 @@ func TestPeersIncremental(t *testing.T) {
 				if len(fetcherURIs) > 0 {
 					t.Errorf("unexpected peer(s)' URI(s) in fetcher-map at step %d: %v", si, fetcherURIs)
 				}
+				// Checks for peer IDs and fetchers from listPeers method.
+				if len(allPeerIDs) > 0 {
+					t.Errorf("unexpected peer(s) in copy of fetcher-map at step %d: %v", si, allPeerIDs)
+				}
+				if len(allFetcherURIs) > 0 {
+					t.Errorf("unexpected peer(s)' URI(s) in copy of fetcher-map at step %d: %v",
+						si, allFetcherURIs)
+				}
 			}
 		})
 	}