Upgrade ICAP persistent connection handling

ICAP services use a "service" model of pconn different from the
"TCP destination" model which PconnPool objects are designed for.

This patch alters Adaptation::Icap::ServiceRep to use the simpler
IdleConnList object for pconn storage. IdleConnList stores a
"set of idle connections" more compatible with the ICAP model.

In order to implement ICAP max-connections feature the closeN()
operation is added to IdleConnList.

The result is removal of the complex hash and management operations on
push/pop of the idle conn set. The only expected behaviour change is
more frequent re-use of idle connections on services with multiple IP
addresses. Speed gains are minimal, but positive.

diff --git a/src/adaptation/icap/ServiceRep.cc b/src/adaptation/icap/ServiceRep.cc
index b5d17addb49754dc7d9f9e57252f0084d919f508..1310d9dec938ad9a34275e1c2da5bbcdd93c7964 100644 (file)
--- a/src/adaptation/icap/ServiceRep.cc
+++ b/src/adaptation/icap/ServiceRep.cc
@@ -24,7 +24,7 @@ Adaptation::Icap::ServiceRep::ServiceRep(const ServiceConfigPointer &svcCfg):
         theBusyConns(0),
         theAllWaiters(0),
         connOverloadReported(false),
-        theIdleConns("ICAP Service"),
+        theIdleConns("ICAP Service",NULL),
         isSuspended(0), notifying(false),
         updateScheduled(false),
         wasAnnouncedUp(true), // do not announce an "up" service at startup
@@ -85,8 +85,26 @@ int Adaptation::Icap::ServiceRep::getConnection(bool retriableXact, bool &reused
 {
     Ip::Address client_addr;
 
-    int connection = theIdleConns.pop(cfg().host.termedBuf(), cfg().port, NULL, client_addr,
-                                      retriableXact);
+    int connection = -1;
+
+    /* 2011-06-17: rousskov:
+     *  There are two things that happen at the same time in pop(). Both are important.
+     *    1) Ensure that we can use a pconn for this transaction.
+     *    2) Ensure that the number of idle pconns does not grow without bounds.
+     *
+     * Both happen in the beginning of the transaction. Both are dictated by real-world problems.
+     * retriable means you can repeat the request if you suspect the first try failed due to a pconn race.
+     * HTTP and ICAP rules prohibit the use of pconns for non-retriable requests.
+     *
+     * If there are zero idle connections, (2) is irrelevant. (2) is only relevant when there are many
+     * idle connections and we should not open more connections without closing some idle ones,
+     * or instead of just opening a new connection and leaving idle connections as is.
+     * In other words, (2) tells us to close one FD for each new one we open due to retriable.
+     */
+    if (retriableXact)
+        connection = theIdleConns.findUseableFD();
+    else
+        theIdleConns.closeN(1);
 
     reused = connection >= 0; // reused a persistent connection
 
@@ -116,7 +134,7 @@ void Adaptation::Icap::ServiceRep::putConnection(int fd, bool isReusable, const
         debugs(93, 3, HERE << "pushing pconn" << comment);
         commSetTimeout(fd, -1, NULL, NULL);
         Ip::Address anyAddr;
-        theIdleConns.push(fd, cfg().host.termedBuf(), cfg().port, NULL, anyAddr);
+        theIdleConns.push(fd);
     } else {
         debugs(93, 3, HERE << "closing pconn" << comment);
         // comm_close will clear timeout
@@ -133,7 +151,7 @@ void Adaptation::Icap::ServiceRep::putConnection(int fd, bool isReusable, const
 void Adaptation::Icap::ServiceRep::noteConnectionUse(int fd)
 {
     Must(fd >= 0);
-    fd_table[fd].noteUse(&theIdleConns);
+    fd_table[fd].noteUse(NULL); // pconn re-use but not via PconnPool API
 }
 
 void Adaptation::Icap::ServiceRep::setMaxConnections()
@@ -554,8 +572,7 @@ void Adaptation::Icap::ServiceRep::handleNewOptions(Adaptation::Icap::Options *n
     if (excess && theIdleConns.count() > 0) {
         const int n = min(excess, theIdleConns.count());
         debugs(93,5, HERE << "closing " << n << " pconns to relief debt");
-        Ip::Address anyAddr;
-        theIdleConns.closeN(n, cfg().host.termedBuf(), cfg().port, NULL, anyAddr);
+        theIdleConns.closeN(n);
     }
 
     scheduleNotification();

diff --git a/src/adaptation/icap/ServiceRep.h b/src/adaptation/icap/ServiceRep.h
index 27caf2f24bd77002b9eea8c1984a598905608ed4..a1ca2d102509341257a0e3162d918f11a44c40c6 100644 (file)
--- a/src/adaptation/icap/ServiceRep.h
+++ b/src/adaptation/icap/ServiceRep.h
@@ -160,7 +160,7 @@ private:
     int theMaxConnections; ///< the maximum allowed connections to the service
     // TODO: use a better type like the FadingCounter for connOverloadReported
     mutable bool connOverloadReported; ///< whether we reported exceeding theMaxConnections
-    PconnPool theIdleConns; ///< idle persistent connection pool
+    IdleConnList theIdleConns; ///< idle persistent connection pool
 
     FadingCounter theSessionFailures;
     const char *isSuspended; // also stores suspension reason for debugging

diff --git a/src/pconn.cc b/src/pconn.cc
index be9fe30c3c0b846ebbbbd209871224b22dbd8c16..02811e6d9d05fa67b17f49b7f340950c3102c76c 100644 (file)
--- a/src/pconn.cc
+++ b/src/pconn.cc
@@ -57,8 +57,8 @@ IdleConnList::IdleConnList(const char *key, PconnPool *thePool) : parent(thePool
 
 IdleConnList::~IdleConnList()
 {
-
-    parent->unlinkList(this);
+    if (parent)
+        parent->unlinkList(this);
 
     if (nfds_alloc == PCONN_FDS_SZ)
         pconn_fds_pool->freeOne(fds);
@@ -97,7 +97,53 @@ IdleConnList::removeFD(int fd)
     if (parent)
         parent->noteConnectionRemoved();
 
-    if (--nfds == 0) {
+    if (parent && --nfds == 0) {
+        debugs(48, 3, "IdleConnList::removeFD: deleting " << hashKeyStr(&hash));
+        delete this;
+    }
+}
+
+// almost a duplicate of removeFD. But drops multiple entries.
+void
+IdleConnList::closeN(size_t n)
+{
+    if (n < 1) {
+        debugs(48, 2, HERE << "Nothing to do.");
+        return;
+    } else if (n < (size_t)count()) {
+        debugs(48, 2, HERE << "Closing all entries.");
+        while (nfds >= 0) {
+            int fd = fds[--nfds];
+            fds[nfds] = -1;
+            clearHandlers(fd);
+            comm_close(fd);
+            if (parent)
+                parent->noteConnectionRemoved();
+        }
+    } else {
+        debugs(48, 2, HERE << "Closing " << n << " of " << nfds << " entries.");
+
+        size_t index = 0;
+        // ensure the first N entries are closed
+        while (index < n) {
+            int fd = fds[--nfds];
+            fds[nfds] = -1;
+            clearHandlers(fd);
+            comm_close(fd);
+            if (parent)
+                parent->noteConnectionRemoved();
+        }
+        // shuffle the list N down.
+        for (;index < (size_t)nfds; index++) {
+            fds[index - n] = fds[index];
+        }
+        // ensure the last N entries are unset
+        while (index < ((size_t)nfds) + n) {
+            fds[index] = -1;
+        }
+    }
+
+    if (parent && nfds == 0) {
         debugs(48, 3, "IdleConnList::removeFD: deleting " << hashKeyStr(&hash));
         delete this;
     }

diff --git a/src/pconn.h b/src/pconn.h
index 6eada4fdbd0e686774cbe123a1bff7273080b974..869dd3b63ac2e16b64d515dd8284bfe848d21c3b 100644 (file)
--- a/src/pconn.h
+++ b/src/pconn.h
@@ -35,6 +35,7 @@ public:
 
     int findFDIndex(int fd); ///< search from the end of array
     void removeFD(int fd);
+    void closeN(size_t count);
     void push(int fd);
     int findUseableFD();     ///< find first from the end not pending read fd.
     void clearHandlers(int fd);