From: Alex Rousskov Date: Thu, 28 Dec 2017 16:35:32 +0000 (-0700) Subject: Bug 2378: Duplicates in selected peer destinations (#112) X-Git-Tag: SQUID_4_0_23~13 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=6b6b34eb4d936b8b22d593e0c55bdb528950b4c9;p=thirdparty%2Fsquid.git 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 6de1fea3f9..9e0b861f82 100644 --- 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; }