=== modified file 'src/adaptation/icap/ServiceRep.cc'
--- src/adaptation/icap/ServiceRep.cc	2011-05-13 10:38:28 +0000
+++ src/adaptation/icap/ServiceRep.cc	2011-06-16 15:01:27 +0000
@@ -24,7 +24,7 @@
         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 @@
 {
     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 @@
         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::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 @@
     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();

=== modified file 'src/adaptation/icap/ServiceRep.h'
--- src/adaptation/icap/ServiceRep.h	2011-05-13 10:38:28 +0000
+++ src/adaptation/icap/ServiceRep.h	2011-06-14 22:55:02 +0000
@@ -160,7 +160,7 @@
     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

=== modified file 'src/pconn.cc'
--- src/pconn.cc	2011-05-13 10:38:28 +0000
+++ src/pconn.cc	2011-06-15 02:26:58 +0000
@@ -57,8 +57,8 @@
 
 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 @@
     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;
     }

=== modified file 'src/pconn.h'
--- src/pconn.h	2011-05-13 10:38:28 +0000
+++ src/pconn.h	2011-06-15 00:07:46 +0000
@@ -35,6 +35,7 @@
 
     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);