Do not use raw pointers to index sourcehash CachePeers (#1474)

Simplified and improved code safety by using CbcPointers for sourcehash
cache_peers, as we have done for CARP peers in recent commit e7959b5.

Also fixed mgr:sourcehash Cache Manager reports to detail relevant
cache_peers instead of all cache_peers. This problem existed since
inception (2008 commit f7e1d9c) as detailed in recent commit e7959b5.

Also applied the new "no new globals" policy to CARP peering code, to
keep improved CARP and sourcehash peering code in sync.

diff --git a/src/carp.cc b/src/carp.cc
index d8f1738945d3d642f0bc1ed0387e7a97490e1f9b..7327f20dc48ded2f6c765f347b14f241ff4b4585 100644 (file)
--- a/src/carp.cc
+++ b/src/carp.cc
@@ -24,7 +24,12 @@
 #define ROTATE_LEFT(x, n) (((x) << (n)) | ((x) >> (32-(n))))
 
 /// CARP cache_peers ordered by their CARP weight
-static SelectedCachePeers TheCarpPeers;
+static auto &
+CarpPeers()
+{
+    static const auto carpPeers = new SelectedCachePeers();
+    return *carpPeers;
+}
 
 static OBJH carpCachemgr;
 
@@ -50,7 +55,7 @@ carpInit(void)
     char *t;
     /* Clean up */
 
-    TheCarpPeers.clear();
+    CarpPeers().clear();
 
     /* initialize cache manager before we have a chance to leave the execution path */
     carpRegisterWithCacheManager();
@@ -126,7 +131,7 @@ carpInit(void)
         P_last = p->carp.load_factor;
     }
 
-    TheCarpPeers.assign(rawCarpPeers.begin(), rawCarpPeers.end());
+    CarpPeers().assign(rawCarpPeers.begin(), rawCarpPeers.end());
 }
 
 CachePeer *
@@ -141,14 +146,14 @@ carpSelectParent(PeerSelector *ps)
     double score;
     double high_score = 0;
 
-    if (TheCarpPeers.empty())
+    if (CarpPeers().empty())
         return nullptr;
 
     /* calculate hash key */
     debugs(39, 2, "carpSelectParent: Calculating hash for " << request->effectiveRequestUri());
 
     /* select CachePeer */
-    for (const auto &tp: TheCarpPeers) {
+    for (const auto &tp: CarpPeers()) {
         if (!tp)
             continue; // peer gone
 
@@ -217,13 +222,13 @@ carpCachemgr(StoreEntry * sentry)
                       "Factor",
                       "Actual");
 
-    for (const auto &p: TheCarpPeers) {
+    for (const auto &p: CarpPeers()) {
         if (!p)
             continue;
         sumfetches += p->stats.fetches;
     }
 
-    for (const auto &p: TheCarpPeers) {
+    for (const auto &p: CarpPeers()) {
         if (!p)
             continue;
         storeAppendPrintf(sentry, "%24s %10x %10f %10f %10f\n",

diff --git a/src/peer_sourcehash.cc b/src/peer_sourcehash.cc
index 2cd54c02f4c9361f616aa0db1eed85edaaab8d2f..a47cf06caf163910242e58c57913289b145f2a60 100644 (file)
--- a/src/peer_sourcehash.cc
+++ b/src/peer_sourcehash.cc
@@ -23,8 +23,14 @@
 
 #define ROTATE_LEFT(x, n) (((x) << (n)) | ((x) >> (32-(n))))
 
-static int n_sourcehash_peers = 0;
-static CachePeer **sourcehash_peers = nullptr;
+/// sourcehash peers ordered by their sourcehash weight
+static auto &
+SourceHashPeers()
+{
+    static const auto hashPeers = new SelectedCachePeers();
+    return *hashPeers;
+}
+
 static OBJH peerSourceHashCachemgr;
 static void peerSourceHashRegisterWithCacheManager(void);
 
@@ -40,21 +46,17 @@ void
 peerSourceHashInit(void)
 {
     int W = 0;
-    int K;
-    int k;
     double P_last, X_last, Xn;
     char *t;
     /* Clean up */
 
-    for (k = 0; k < n_sourcehash_peers; ++k) {
-        cbdataReferenceDone(sourcehash_peers[k]);
-    }
-
-    safe_free(sourcehash_peers);
-    n_sourcehash_peers = 0;
+    SourceHashPeers().clear();
     /* find out which peers we have */
 
+    RawCachePeers rawSourceHashPeers;
     for (const auto &p: CurrentCachePeers()) {
+        const auto peer = p.get();
+
         if (!p->options.sourcehash)
             continue;
 
@@ -63,29 +65,18 @@ peerSourceHashInit(void)
         if (p->weight == 0)
             continue;
 
-        ++n_sourcehash_peers;
+        rawSourceHashPeers.push_back(peer);
 
         W += p->weight;
     }
 
     peerSourceHashRegisterWithCacheManager();
 
-    if (n_sourcehash_peers == 0)
+    if (rawSourceHashPeers.empty())
         return;
 
-    sourcehash_peers = (CachePeer **)xcalloc(n_sourcehash_peers, sizeof(*sourcehash_peers));
-
-    auto P = sourcehash_peers;
-    /* Build a list of the found peers and calculate hashes and load factors */
-    for (const auto &peer: CurrentCachePeers()) {
-        const auto p = peer.get();
-
-        if (!p->options.sourcehash)
-            continue;
-
-        if (p->weight == 0)
-            continue;
-
+    /* calculate hashes and load factors */
+    for (const auto &p: rawSourceHashPeers) {
         /* calculate this peers hash */
         p->sourcehash.hash = 0;
 
@@ -101,13 +92,10 @@ peerSourceHashInit(void)
 
         if (floor(p->sourcehash.load_factor * 1000.0) == 0.0)
             p->sourcehash.load_factor = 0.0;
-
-        /* add it to our list of peers */
-        *P++ = cbdataReference(p);
     }
 
     /* Sort our list on weight */
-    qsort(sourcehash_peers, n_sourcehash_peers, sizeof(*sourcehash_peers), peerSortWeight);
+    qsort(rawSourceHashPeers.data(), rawSourceHashPeers.size(), sizeof(decltype(rawSourceHashPeers)::value_type), peerSortWeight);
 
     /* Calculate the load factor multipliers X_k
      *
@@ -117,7 +105,7 @@ peerSourceHashInit(void)
      * X_k = pow (X_k, {1/(K-k+1)})
      * simplified to have X_1 part of the loop
      */
-    K = n_sourcehash_peers;
+    const auto K = rawSourceHashPeers.size();
 
     P_last = 0.0;       /* Empty P_0 */
 
@@ -125,9 +113,9 @@ peerSourceHashInit(void)
 
     X_last = 0.0;       /* Empty X_0, nullifies the first pow statement */
 
-    for (k = 1; k <= K; ++k) {
+    for (size_t k = 1; k <= K; ++k) {
         double Kk1 = (double) (K - k + 1);
-        const auto p = sourcehash_peers[k - 1];
+        const auto p = rawSourceHashPeers[k - 1];
         p->sourcehash.load_multiplier = (Kk1 * (p->sourcehash.load_factor - P_last)) / Xn;
         p->sourcehash.load_multiplier += pow(X_last, Kk1);
         p->sourcehash.load_multiplier = pow(p->sourcehash.load_multiplier, 1.0 / Kk1);
@@ -135,6 +123,8 @@ peerSourceHashInit(void)
         X_last = p->sourcehash.load_multiplier;
         P_last = p->sourcehash.load_factor;
     }
+
+    SourceHashPeers().assign(rawSourceHashPeers.begin(), rawSourceHashPeers.end());
 }
 
 static void
@@ -147,10 +137,8 @@ peerSourceHashRegisterWithCacheManager(void)
 CachePeer *
 peerSourceHashSelectParent(PeerSelector *ps)
 {
-    int k;
     const char *c;
     CachePeer *p = nullptr;
-    CachePeer *tp;
     unsigned int user_hash = 0;
     unsigned int combined_hash;
     double score;
@@ -158,7 +146,7 @@ peerSourceHashSelectParent(PeerSelector *ps)
     const char *key = nullptr;
     char ntoabuf[MAX_IPSTRLEN];
 
-    if (n_sourcehash_peers == 0)
+    if (SourceHashPeers().empty())
         return nullptr;
 
     assert(ps);
@@ -173,8 +161,10 @@ peerSourceHashSelectParent(PeerSelector *ps)
         user_hash += ROTATE_LEFT(user_hash, 19) + *c;
 
     /* select CachePeer */
-    for (k = 0; k < n_sourcehash_peers; ++k) {
-        tp = sourcehash_peers[k];
+    for (const auto &tp: SourceHashPeers()) {
+        if (!tp)
+            continue; // peer gone
+
         combined_hash = (user_hash ^ tp->sourcehash.hash);
         combined_hash += combined_hash * 0x62531965;
         combined_hash = ROTATE_LEFT(combined_hash, 21);
@@ -182,8 +172,8 @@ peerSourceHashSelectParent(PeerSelector *ps)
         debugs(39, 3, *tp << " combined_hash " << combined_hash  <<
                " score " << std::setprecision(0) << score);
 
-        if ((score > high_score) && peerHTTPOkay(tp, ps)) {
-            p = tp;
+        if ((score > high_score) && peerHTTPOkay(tp.get(), ps)) {
+            p = tp.get();
             high_score = score;
         }
     }
@@ -205,10 +195,15 @@ peerSourceHashCachemgr(StoreEntry * sentry)
                       "Factor",
                       "Actual");
 
-    for (const auto &p: CurrentCachePeers())
+    for (const auto &p: SourceHashPeers()) {
+        if (!p)
+            continue;
         sumfetches += p->stats.fetches;
+    }
 
-    for (const auto &p: CurrentCachePeers()) {
+    for (const auto &p: SourceHashPeers()) {
+        if (!p)
+            continue;
         storeAppendPrintf(sentry, "%24s %10x %10f %10f %10f\n",
                           p->name, p->sourcehash.hash,
                           p->sourcehash.load_multiplier,