From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Mon, 8 May 2017 07:04:05 +0000 (+1200)
Subject: Do not forward HTTP requests to dead idle peers.
X-Git-Tag: SQUID_4_0_20~17
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c9d2d8f1ee1691ca1be81ca8c24e90b833b6179b;p=thirdparty%2Fsquid.git

Do not forward HTTP requests to dead idle peers.

Squid does not forward HTTP transactions to dead peers except when a
dead peer was idle for some time (ten peer connect timeouts or longer).
When the idle peer is still dead, this exception leads to transaction
delays (at best) or client disconnects/errors (at worst), depending on
Squid and client configurations/state. I am removing this exception.

The "use dead idle peer" heuristic was introduced as a small part of a
much bigger bug #14 fix (trunk r6631). AFAICT, the stated goal of the
feature was speeding up failure recovery: The heuristic may result in
HTTP transactions sent to a previously dead (but now alive) idle peer
earlier, before the peer is proven to be alive (using peer revival
mechanisms such as TCP probes). However, the negative side effects of
this heuristic outweigh its accidental benefits. If somebody needs Squid
to detect revived idle peers earlier, they need to add a different
probing mechanism that does not jeopardize HTTP transactions.

Nobody has spoken in defense of this feature on Squid mailing lists:
http://lists.squid-cache.org/pipermail/squid-users/2017-March/014785.html
http://lists.squid-cache.org/pipermail/squid-dev/2017-March/008308.html

The removed functionality was not used to detect revived peers. All peer
revival mechanisms (such as TCP probes) remain intact.
---

diff --git a/src/neighbors.cc b/src/neighbors.cc
index c72c3d5812..256ff9a561 100644
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -59,7 +59,7 @@ static void neighborAliveHtcp(CachePeer *, const MemObject *, const HtcpReplyDat
 static void neighborCountIgnored(CachePeer *);
 static void peerRefreshDNS(void *);
 static IPH peerDNSConfigure;
-static bool peerProbeConnect(CachePeer *);
+static void peerProbeConnect(CachePeer *);
 static CNCB peerProbeConnectDone;
 static void peerCountMcastPeersDone(void *data);
 static void peerCountMcastPeersStart(void *data);
@@ -1123,10 +1123,8 @@ int
 neighborUp(const CachePeer * p)
 {
     if (!p->tcp_up) {
-        if (!peerProbeConnect((CachePeer *) p)) {
-            debugs(15, 8, "neighborUp: DOWN (probed): " << p->host << " (" << p->in_addr << ")");
-            return 0;
-        }
+        peerProbeConnect(const_cast<CachePeer*>(p));
+        return 0;
     }
 
     /*
@@ -1292,18 +1290,20 @@ peerConnectSucceded(CachePeer * p)
 /*
 * peerProbeConnect will be called on dead peers by neighborUp
 */
-static bool
+static void
 peerProbeConnect(CachePeer * p)
 {
-    const time_t ctimeout = peerConnectTimeout(p);
-    bool ret = (squid_curtime - p->stats.last_connect_failure) > (ctimeout * 10);
-
-    if (p->testing_now > 0)
-        return ret;/* probe already running */
+    if (p->testing_now > 0) {
+        debugs(15, 8, "already probing " << p);
+        return;
+    }
 
-    if (squid_curtime - p->stats.last_connect_probe == 0)
-        return ret;/* don't probe to often */
+    if (squid_curtime - p->stats.last_connect_probe == 0) {
+        debugs(15, 8, "just probed " << p);
+        return;
+    }
 
+    const time_t ctimeout = peerConnectTimeout(p);
     /* for each IP address of this CachePeer. find one that we can connect to and probe it. */
     for (int i = 0; i < p->n_addresses; ++i) {
         Comm::ConnectionPointer conn = new Comm::Connection;
@@ -1321,8 +1321,6 @@ peerProbeConnect(CachePeer * p)
     }
 
     p->stats.last_connect_probe = squid_curtime;
-
-    return ret;
 }
 
 static void