Do not use invasive lists to store CachePeers (#1424)

Using invasive lists for CachePeer objects gives no advantages, but
requires maintaining non-standard error-prone code that leads to
excessive locking and associated memory overheads.

Also fixed a neighbors_init() bug that resulted in accessing an already
deleted "looks like this host" CachePeer object.

src/CachePeer.cc

@@ -47,8 +47,6 @@ CachePeer::~CachePeer()
     xfree(digest_url);
 #endif
 
-    delete next;
-
     xfree(login);
 
     delete standby.pool;

src/CachePeer.h

@@ -48,6 +48,7 @@ class CachePeer
     /// \returns the effective connect timeout for the given peer
     time_t connectTimeout() const;
 
+    /// n-th cache_peer directive, starting with 1
     u_int index = 0;
 
     /// cache_peer name (if explicitly configured) or hostname (otherwise).
@@ -179,7 +180,6 @@ class CachePeer
     Ip::Address addresses[10];
     int n_addresses = 0;
     int rr_count = 0;
-    CachePeer *next = nullptr;
     int testing_now = 0;
 
     struct {

src/CachePeers.cc

@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#include "squid.h"
+#include "CachePeers.h"
+#include "SquidConfig.h"
+
+CachePeer &
+CachePeers::nextPeerToPing(const size_t pollIndex)
+{
+    Assure(size());
+
+    // Remember the number of polls to keep shifting each poll starting point,
+    // to avoid always polling the same group of peers before other peers and
+    // risk overloading that first group with requests.
+    if (!pollIndex)
+        ++peerPolls_;
+
+    // subtract 1 to set the very first pos to zero
+    const auto pos = (peerPolls_ - 1 + pollIndex) % size();
+
+    return *storage[pos];
+}
+
+void
+CachePeers::remove(CachePeer * const peer)
+{
+    const auto pos = std::find_if(storage.begin(), storage.end(), [&](const auto &storePeer) {
+        return storePeer.get() == peer;
+    });
+    Assure(pos != storage.end());
+    storage.erase(pos);
+}
+
+const CachePeers &
+CurrentCachePeers()
+{
+    if (Config.peers)
+        return *Config.peers;
+
+    static const CachePeers empty;
+    return empty;
+}
+
+void
+DeleteConfigured(CachePeer * const peer)
+{
+    Assure(Config.peers);
+    Config.peers->remove(peer);
+}
+

src/CachePeers.h

@@ -13,8 +13,51 @@
 #include "CachePeer.h"
 #include "mem/PoolingAllocator.h"
 
+#include <memory>
 #include <vector>
 
+/// cache_peer configuration storage
+class CachePeers
+{
+public:
+    /// owns stored CachePeer objects
+    using Storage = std::vector< std::unique_ptr<CachePeer>, PoolingAllocator< std::unique_ptr<CachePeer> > >;
+
+    /// stores a being-configured cache_peer
+    void add(CachePeer *p) { storage.emplace_back(p); }
+
+    /// deletes a previously add()ed CachePeer object
+    void remove(CachePeer *);
+
+    /// the number of currently stored (i.e. added and not removed) cache_peers
+    auto size() const { return storage.size(); }
+
+    /* peer iterators forming a sequence for C++ range-based for loop API */
+    using const_iterator = Storage::const_iterator;
+    auto begin() const { return storage.cbegin(); }
+    auto end() const { return storage.cend(); }
+
+    /// A CachePeer to query next when scanning all peer caches in hope to fetch
+    /// a remote cache hit. \sa neighborsUdpPing()
+    /// \param iteration a 0-based index of a loop scanning all peers
+    CachePeer &nextPeerToPing(size_t iteration);
+
+private:
+    /// cache_peers in configuration/parsing order
+    Storage storage;
+
+    /// total number of completed peer scans by nextPeerToPing()-calling code
+    uint64_t peerPolls_ = 0;
+};
+
+/// All configured cache_peers that are still available/relevant.
+/// \returns an empty container if no cache_peers were configured or all
+/// configured cache_peers were removed (e.g., by DeleteConfigured()).
+const CachePeers &CurrentCachePeers();
+
+/// destroys the given peer after removing it from the set of configured peers
+void DeleteConfigured(CachePeer *);
+
 /// Weak pointers to zero or more Config.peers.
 /// Users must specify the selection algorithm and the order of entries.
 using SelectedCachePeers = std::vector< CbcPointer<CachePeer>, PoolingAllocator< CbcPointer<CachePeer> > >;

src/Makefile.am

@@ -199,6 +199,7 @@ squid_SOURCES = \
 	CacheManager.h \
 	CachePeer.cc \
 	CachePeer.h \
+	CachePeers.cc \
 	CachePeers.h \
 	ClientInfo.h \
 	ClientRequestContext.h \
@@ -1741,6 +1742,8 @@ tests_test_http_range_SOURCES = \
 	CacheDigest.h \
 	CachePeer.cc \
 	CachePeer.h \
+	CachePeers.cc \
+	CachePeers.h \
 	ClientInfo.h \
 	tests/stub_CollapsedForwarding.cc \
 	ConfigOption.cc \
@@ -2124,6 +2127,8 @@ tests_testHttpRequest_SOURCES = \
 	CacheDigest.h \
 	CachePeer.cc \
 	CachePeer.h \
+	CachePeers.cc \
+	CachePeers.h \
 	ClientInfo.h \
 	tests/stub_CollapsedForwarding.cc \
 	ConfigOption.cc \
@@ -2420,6 +2425,8 @@ tests_testCacheManager_SOURCES = \
 	tests/testCacheManager.cc \
 	CachePeer.cc \
 	CachePeer.h \
+	CachePeers.cc \
+	CachePeers.h \
 	ClientInfo.h \
 	tests/stub_CollapsedForwarding.cc \
 	ConfigOption.cc \

src/PeerPoolMgr.cc

@@ -11,6 +11,7 @@
 #include "base/AsyncCallbacks.h"
 #include "base/RunnersRegistry.h"
 #include "CachePeer.h"
+#include "CachePeers.h"
 #include "comm/Connection.h"
 #include "comm/ConnOpener.h"
 #include "debug/Stream.h"
@@ -244,7 +245,8 @@ DefineRunnerRegistrator(PeerPoolMgrsRr);
 void
 PeerPoolMgrsRr::syncConfig()
 {
-    for (CachePeer *p = Config.peers; p; p = p->next) {
+    for (const auto &peer: CurrentCachePeers()) {
+        const auto p = peer.get();
         // On reconfigure, Squid deletes the old config (and old peers in it),
         // so should always be dealing with a brand new configuration.
         assert(!p->standby.mgr);

src/SquidConfig.h

@@ -42,7 +42,8 @@ namespace Mgr
 {
 class ActionPasswordList;
 } // namespace Mgr
-class CachePeer;
+
+class CachePeers;
 class CustomLog;
 class CpuAffinityMap;
 class DebugMessages;
@@ -242,7 +243,7 @@ class SquidConfig
     size_t tcpRcvBufsz;
     size_t udpMaxHitObjsz;
     wordlist *mcast_group_list;
-    CachePeer *peers;
+    CachePeers *peers;
     int npeers;
 
     struct {

src/cache_cf.cc

@@ -25,6 +25,7 @@
 #include "base/RunnersRegistry.h"
 #include "cache_cf.h"
 #include "CachePeer.h"
+#include "CachePeers.h"
 #include "ConfigOption.h"
 #include "ConfigParser.h"
 #include "CpuAffinityMap.h"
@@ -975,7 +976,7 @@ configDoConfigure(void)
 #endif
     }
 
-    for (CachePeer *p = Config.peers; p != nullptr; p = p->next) {
+    for (const auto &p: CurrentCachePeers()) {
 
         // default value for ssldomain= is the peer host/IP
         if (p->secure.sslDomain.isEmpty())
@@ -2068,12 +2069,16 @@ peer_type_str(const peer_t type)
 }
 
 static void
-dump_peer(StoreEntry * entry, const char *name, CachePeer * p)
+dump_peer(StoreEntry * entry, const char *name, const CachePeers *peers)
 {
+    if (!peers)
+        return;
+
     NeighborTypeDomainList *t;
     LOCAL_ARRAY(char, xname, 128);
 
-    while (p != nullptr) {
+    for (const auto &peer: *peers) {
+        const auto p = peer.get();
         storeAppendPrintf(entry, "%s %s %s %d %d name=%s",
                           name,
                           p->host,
@@ -2094,8 +2099,6 @@ dump_peer(StoreEntry * entry, const char *name, CachePeer * p)
                               peer_type_str(t->type),
                               t->domain);
         }
-
-        p = p->next;
     }
 }
 
@@ -2160,7 +2163,7 @@ GetUdpService(void)
 }
 
 static void
-parse_peer(CachePeer ** head)
+parse_peer(CachePeers **peers)
 {
     char *host_str = ConfigParser::NextToken();
     if (!host_str) {
@@ -2397,22 +2400,21 @@ parse_peer(CachePeer ** head)
     if (p->secure.encryptTransport)
         p->secure.parseOptions();
 
-    p->index =  ++Config.npeers;
+    if (!*peers)
+        *peers = new CachePeers;
 
-    while (*head != nullptr)
-        head = &(*head)->next;
+    (*peers)->add(p);
 
-    *head = p;
+    p->index = (*peers)->size();
 
     peerClearRRStart();
 }
 
 static void
-free_peer(CachePeer ** P)
+free_peer(CachePeers ** const peers)
 {
-    delete *P;
-    *P = nullptr;
-    Config.npeers = 0;
+    delete *peers;
+    *peers = nullptr;
 }
 
 static void

src/carp.cc

@@ -58,7 +58,9 @@ carpInit(void)
     /* find out which peers we have */
 
     RawCachePeers rawCarpPeers;
-    for (auto p = Config.peers; p; p = p->next) {
+    for (const auto &peer: CurrentCachePeers()) {
+        const auto p = peer.get();
+
         if (!p->options.carp)
             continue;
 

src/htcp.cc

@@ -14,6 +14,7 @@
 #include "acl/FilledChecklist.h"
 #include "base/AsyncCallbacks.h"
 #include "CachePeer.h"
+#include "CachePeers.h"
 #include "comm.h"
 #include "comm/Connection.h"
 #include "comm/Loops.h"
@@ -1272,9 +1273,7 @@ htcpHandleClr(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from)
 static void
 htcpForwardClr(char *buf, int sz)
 {
-    CachePeer *p;
-
-    for (p = Config.peers; p; p = p->next) {
+    for (const auto &p: CurrentCachePeers()) {
         if (!p->options.htcp) {
             continue;
         }

src/icmp/net_db.cc

@@ -18,6 +18,7 @@
 
 #include "squid.h"
 #include "CachePeer.h"
+#include "CachePeers.h"
 #include "cbdata.h"
 #include "event.h"
 #include "fde.h"
@@ -1233,7 +1234,8 @@ netdbExchangeStart(void *data)
 static CachePeer *
 findUsableParentAtHostname(PeerSelector *ps, const char * const hostname, const HttpRequest &request)
 {
-    for (auto p = Config.peers; p; p = p->next) {
+    for (const auto &peer: CurrentCachePeers()) {
+        const auto p = peer.get();
         // Both fields should be lowercase, but no code ensures that invariant.
         // TODO: net_db_peer should point to CachePeer instead of its hostname!
         if (strcasecmp(p->host, hostname) != 0)