Bug 2378: Duplicates in selected peer destinations (#112)

Duplicates in FwdServers lead to excessive peer connection retries, skew
in round-robin peer selection, and probably other problems.

This bug was fixed in 2008 but that v2 fix was never ported to v3. This
fix includes a bug 2408 fix for the original (bug 2378) fix, although I
adjusted bug 2408 logic to explicitly reject duplicate PINNED
destinations and to clarify why PINNED connection handling is "special".

I also centralized and improved peerAddFwdServer-related debugging,
removing duplicated and slightly inconsistent code.

diff --git a/src/peer_select.cc b/src/peer_select.cc
index 6de1fea3f93399f5bdc601dfb26cff6db73839e2..9e0b861f826ece2ba7d182f99fb9f051162e555b 100644 (file)
--- a/src/peer_select.cc
+++ b/src/peer_select.cc
@@ -46,6 +46,18 @@ static const char *DirectStr[] = {
     "DIRECT_YES"
 };
 
+/// a helper class to report a selected destination (for debugging)
+class PeerSelectionDumper
+{
+public:
+    PeerSelectionDumper(const ps_state * const aPs, const CachePeer * const aPeer, const hier_code aCode):
+        ps(aPs), peer(aPeer), code(aCode) {}
+
+    const ps_state * const ps; ///< selection parameters
+    const CachePeer * const peer; ///< successful selection info
+    const hier_code code; ///< selection algorithm
+};
+
 static void peerSelectFoo(ps_state *);
 static void peerPingTimeout(void *data);
 static IRCB peerHandlePingReply;
@@ -60,12 +72,26 @@ static void peerGetSomeNeighborReplies(ps_state *);
 static void peerGetSomeDirect(ps_state *);
 static void peerGetSomeParent(ps_state *);
 static void peerGetAllParents(ps_state *);
-static void peerAddFwdServer(FwdServer **, CachePeer *, hier_code);
+static void peerAddFwdServer(ps_state*, CachePeer*, const hier_code);
 static void peerSelectPinned(ps_state * ps);
 static void peerSelectDnsResults(const ipcache_addrs *ia, const Dns::LookupDetails &details, void *data);
 
 CBDATA_CLASS_INIT(ps_state);
 
+/// prints PeerSelectionDumper (for debugging)
+static std::ostream &
+operator <<(std::ostream &os, const PeerSelectionDumper &fsd)
+{
+    os << hier_code_str[fsd.code];
+
+    if (fsd.peer)
+        os << '/' << fsd.peer->host;
+    else if (fsd.ps) // useful for DIRECT and gone PINNED destinations
+        os << '#' << fsd.ps->request->url.host();
+
+    return os;
+}
+
 ps_state::~ps_state()
 {
     while (servers) {
@@ -530,11 +556,11 @@ peerSelectPinned(ps_state * ps)
     CachePeer *pear = request->pinnedConnection()->pinnedPeer();
     if (Comm::IsConnOpen(request->pinnedConnection()->validatePinnedConnection(request, pear))) {
         if (pear && peerAllowedToUse(pear, request)) {
-            peerAddFwdServer(&ps->servers, pear, PINNED);
+            peerAddFwdServer(ps, pear, PINNED);
             if (ps->entry)
                 ps->entry->ping_status = PING_DONE;     /* Skip ICP */
         } else if (!pear && ps->direct != DIRECT_NO) {
-            peerAddFwdServer(&ps->servers, NULL, PINNED);
+            peerAddFwdServer(ps, nullptr, PINNED);
             if (ps->entry)
                 ps->entry->ping_status = PING_DONE;     /* Skip ICP */
         }
@@ -604,8 +630,7 @@ peerGetSomeNeighbor(ps_state * ps)
 
     if (code != HIER_NONE) {
         assert(p);
-        debugs(44, 3, "peerSelect: " << hier_code_str[code] << "/" << p->host);
-        peerAddFwdServer(&ps->servers, p, code);
+        peerAddFwdServer(ps, p, code);
     }
 
     entry->ping_status = PING_DONE;
@@ -619,7 +644,6 @@ peerGetSomeNeighbor(ps_state * ps)
 static void
 peerGetSomeNeighborReplies(ps_state * ps)
 {
-    HttpRequest *request = ps->request;
     CachePeer *p = NULL;
     hier_code code = HIER_NONE;
     assert(ps->entry->ping_status == PING_WAITING);
@@ -627,8 +651,7 @@ peerGetSomeNeighborReplies(ps_state * ps)
 
     if (peerCheckNetdbDirect(ps)) {
         code = CLOSEST_DIRECT;
-        debugs(44, 3, hier_code_str[code] << "/" << request->url.host());
-        peerAddFwdServer(&ps->servers, NULL, code);
+        peerAddFwdServer(ps, nullptr, code);
         return;
     }
 
@@ -644,8 +667,7 @@ peerGetSomeNeighborReplies(ps_state * ps)
         }
     }
     if (p && code != HIER_NONE) {
-        debugs(44, 3, hier_code_str[code] << "/" << p->host);
-        peerAddFwdServer(&ps->servers, p, code);
+        peerAddFwdServer(ps, p, code);
     }
 }
 
@@ -665,7 +687,7 @@ peerGetSomeDirect(ps_state * ps)
     if (ps->request->url.getScheme() == AnyP::PROTO_WAIS)
         return;
 
-    peerAddFwdServer(&ps->servers, NULL, HIER_DIRECT);
+    peerAddFwdServer(ps, nullptr, HIER_DIRECT);
 }
 
 static void
@@ -698,8 +720,7 @@ peerGetSomeParent(ps_state * ps)
     }
 
     if (code != HIER_NONE) {
-        debugs(44, 3, "peerSelect: " << hier_code_str[code] << "/" << p->host);
-        peerAddFwdServer(&ps->servers, p, code);
+        peerAddFwdServer(ps, p, code);
     }
 }
 
@@ -723,9 +744,7 @@ peerGetAllParents(ps_state * ps)
         if (!peerHTTPOkay(p, request))
             continue;
 
-        debugs(15, 3, "peerGetAllParents: adding alive parent " << p->host);
-
-        peerAddFwdServer(&ps->servers, p, ANY_OLD_PARENT);
+        peerAddFwdServer(ps, p, ANY_OLD_PARENT);
     }
 
     /* XXX: should add dead parents here, but it is currently
@@ -734,7 +753,7 @@ peerGetAllParents(ps_state * ps)
      */
     /* Add default parent as a last resort */
     if ((p = getDefaultParent(request))) {
-        peerAddFwdServer(&ps->servers, p, DEFAULT_PARENT);
+        peerAddFwdServer(ps, p, DEFAULT_PARENT);
     }
 }
 
@@ -924,16 +943,27 @@ peerHandlePingReply(CachePeer * p, peer_t type, AnyP::ProtocolType proto, void *
 }
 
 static void
-peerAddFwdServer(FwdServer ** FSVR, CachePeer * p, hier_code code)
+peerAddFwdServer(ps_state *ps, CachePeer *peer, const hier_code code)
 {
-    debugs(44, 5, "peerAddFwdServer: adding " <<
-           (p ? p->host : "DIRECT")  << " " <<
-           hier_code_str[code]  );
-    FwdServer *fs = new FwdServer(p, code);
-
-    while (*FSVR)
-        FSVR = &(*FSVR)->next;
+    // Find the end of the servers list. Bail on a duplicate destination.
+    assert(ps);
+    FwdServer **FSVR = &ps->servers;
+    while (const auto server = *FSVR) {
+        // There can be at most one PINNED destination.
+        // Non-PINNED destinations are uniquely identified by their CachePeer
+        // (even though a DIRECT destination might match a cache_peer address).
+        const bool duplicate = (server->code == PINNED) ?
+            (code == PINNED) : (server->_peer == peer);
+        if (duplicate) {
+            debugs(44, 3, "skipping " << PeerSelectionDumper(ps, peer, code) <<
+                   "; have " << PeerSelectionDumper(ps, server->_peer.get(), server->code));
+            return;
+        }
+        FSVR = &server->next;
+    }
 
+    debugs(44, 3, "adding " << PeerSelectionDumper(ps, peer, code));
+    FwdServer *fs = new FwdServer(peer, code);
     *FSVR = fs;
 }