=== modified file 'src/pconn.cc'
--- src/pconn.cc	2010-04-17 02:29:04 +0000
+++ src/pconn.cc	2010-10-02 16:37:57 +0000
@@ -34,98 +34,104 @@
 
 #include "squid.h"
 #include "CacheManager.h"
-#include "Store.h"
 #include "comm.h"
+#include "comm/Connection.h"
+#include "fde.h"
 #include "pconn.h"
-#include "fde.h"
+#include "Store.h"
 
 #define PCONN_FDS_SZ	8	/* pconn set size, increase for better memcache hit rate */
 
-static MemAllocator *pconn_fds_pool = NULL;
+//TODO: re-attach to MemPools. WAS: static MemAllocator *pconn_fds_pool = NULL;
 PconnModule * PconnModule::instance = NULL;
 CBDATA_CLASS_INIT(IdleConnList);
 
 /* ========== IdleConnList ============================================ */
 
-IdleConnList::IdleConnList(const char *key, PconnPool *thePool) : parent(thePool)
+IdleConnList::IdleConnList(const char *key, PconnPool *thePool) :
+        nfds_alloc(PCONN_FDS_SZ),
+        nfds(0),
+        parent(thePool)
 {
     hash.key = xstrdup(key);
-    nfds_alloc = PCONN_FDS_SZ;
-    nfds = 0;
-    fds = (int *)pconn_fds_pool->alloc();
+    theList = new Comm::ConnectionPointer[nfds_alloc];
+// TODO: re-attach to MemPools. WAS: fds = (int *)pconn_fds_pool->alloc();
 }
 
 IdleConnList::~IdleConnList()
 {
-
     parent->unlinkList(this);
 
+/* TODO: re-attach to MemPools.
     if (nfds_alloc == PCONN_FDS_SZ)
-        pconn_fds_pool->freeOne(fds);
+        pconn_fds_pool->freeOne(theList);
     else
-        xfree(fds);
+*/
+    delete[] theList;
 
     xfree(hash.key);
 }
 
 int
-IdleConnList::findFDIndex (int fd)
+IdleConnList::findIndex(const Comm::ConnectionPointer &conn)
 {
-    int index;
-
-    for (index = nfds - 1; index >= 0; --index) {
-        if (fds[index] == fd)
+    for (int index = nfds - 1; index >= 0; --index) {
+        if (conn->fd == theList[index]->fd)
             return index;
     }
 
     return -1;
 }
 
-void
-IdleConnList::removeFD(int fd)
+bool
+IdleConnList::remove(const Comm::ConnectionPointer &conn)
 {
-    int index = findFDIndex(fd);
+    int index = findIndex(conn);
     if (index < 0) {
-        debugs(48, 2, "IdleConnList::removeFD: FD " << fd << " NOT FOUND!");
-        return;
+        debugs(48, 2, HERE << conn << " NOT FOUND!");
+        return false;
     }
-    debugs(48, 3, "IdleConnList::removeFD: found FD " << fd << " at index " << index);
+    debugs(48, 3, HERE << "found " << conn << " at index " << index);
 
     for (; index < nfds - 1; index++)
-        fds[index] = fds[index + 1];
+        theList[index] = theList[index + 1];
 
     if (--nfds == 0) {
         debugs(48, 3, "IdleConnList::removeFD: deleting " << hashKeyStr(&hash));
         delete this;
     }
+    return true;
 }
 
 void
-IdleConnList::clearHandlers(int fd)
+IdleConnList::clearHandlers(const Comm::ConnectionPointer &conn)
 {
-    comm_read_cancel(fd, IdleConnList::read, this);
-    commSetTimeout(fd, -1, NULL, NULL);
+    comm_read_cancel(conn->fd, IdleConnList::read, this);
+    commSetTimeout(conn->fd, -1, NULL, NULL);
 }
 
 void
-IdleConnList::push(int fd)
+IdleConnList::push(const Comm::ConnectionPointer &conn)
 {
     if (nfds == nfds_alloc) {
         debugs(48, 3, "IdleConnList::push: growing FD array");
         nfds_alloc <<= 1;
-        int *old = fds;
-        fds = (int *)xmalloc(nfds_alloc * sizeof(int));
-        xmemcpy(fds, old, nfds * sizeof(int));
+        const Comm::ConnectionPointer *oldList = theList;
+        theList = new Comm::ConnectionPointer[nfds_alloc];
+        for (int index = 0; index < nfds; index++)
+            theList[index] = oldList[index];
 
+/* TODO: re-attach to MemPools.
         if (nfds == PCONN_FDS_SZ)
-            pconn_fds_pool->freeOne(old);
+            pconn_fds_pool->freeOne(oldList);
         else
-            xfree(old);
+*/
+        delete[] oldList;
     }
 
-    fds[nfds++] = fd;
-    comm_read(fd, fakeReadBuf, sizeof(fakeReadBuf), IdleConnList::read, this);
-    commSetTimeout(fd, Config.Timeout.pconn, IdleConnList::timeout, this);
+    theList[nfds++] = conn;
+    comm_read(conn, fakeReadBuf, sizeof(fakeReadBuf), IdleConnList::read, this);
+    commSetTimeout(conn->fd, Config.Timeout.pconn, IdleConnList::timeout, this);
 }
 
 /*
@@ -136,24 +142,36 @@
  * of requests JUST as they timeout (say, it shuts down) we'll be wasting
  * quite a bit of CPU. Just keep it in mind.
  */
-int
-IdleConnList::findUseableFD()
+Comm::ConnectionPointer
+IdleConnList::findUseable(const Comm::ConnectionPointer &key)
 {
     assert(nfds);
 
     for (int i=nfds-1; i>=0; i--) {
-        if (!comm_has_pending_read_callback(fds[i])) {
-            return fds[i];
-        }
+
+        // callback pending indicates that remote end of the conn has just closed.
+        if (comm_has_pending_read_callback(theList[i]->fd))
+            continue;
+
+        // local end port is required, but dont match.
+        if (key->local.GetPort() > 0 && key->local.GetPort() != theList[i]->local.GetPort())
+            continue;
+
+        // local address is required, but does not match.
+        if (!key->local.IsAnyAddr() && key->local.matchIPAddr(theList[i]->local) != 0)
+            continue;
+
+        // finally, a match
+        return theList[i];
     }
 
-    return -1;
+    return key;
 }
 
 void
-IdleConnList::read(int fd, char *buf, size_t len, comm_err_t flag, int xerrno, void *data)
+IdleConnList::read(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm_err_t flag, int xerrno, void *data)
 {
-    debugs(48, 3, "IdleConnList::read: " << len << " bytes from FD " << fd);
+    debugs(48, 3, HERE << len << " bytes from " << conn);
 
     if (flag == COMM_ERR_CLOSING) {
         /* Bail out early on COMM_ERR_CLOSING - close handlers will tidy up for us */
@@ -161,8 +179,11 @@
     }
 
     IdleConnList *list = (IdleConnList *) data;
-    list->removeFD(fd);	/* might delete list */
-    comm_close(fd);
+    /* might delete list */
+    if (list && list->remove(conn)) {
+        Comm::ConnectionPointer nonConst = conn;
+        nonConst->close();
+    }
 }
 
 void
@@ -170,35 +191,34 @@
 {
     debugs(48, 3, "IdleConnList::timeout: FD " << fd);
     IdleConnList *list = (IdleConnList *) data;
-    list->removeFD(fd);	/* might delete list */
-    comm_close(fd);
+    Comm::ConnectionPointer temp = new Comm::Connection; // XXX: transition. make timeouts pass conn in
+    temp->fd = fd;
+    if (list->remove(temp)) {
+        temp->close();
+    } else
+        temp->fd = -1; // XXX: transition. prevent temp erasure double-closing FD until timeout CB passess conn in.
 }
 
 /* ========== PconnPool PRIVATE FUNCTIONS ============================================ */
 
 const char *
-PconnPool::key(const char *host, u_short port, const char *domain, Ip::Address &client_address)
+PconnPool::key(const Comm::ConnectionPointer &destLink, const char *domain)
 {
     LOCAL_ARRAY(char, buf, SQUIDHOSTNAMELEN * 3 + 10);
-    char ntoabuf[MAX_IPSTRLEN];
-
-    if (domain && !client_address.IsAnyAddr())
-        snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d-%s/%s", host, (int) port, client_address.NtoA(ntoabuf,MAX_IPSTRLEN), domain);
-    else if (domain && client_address.IsAnyAddr())
-        snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d/%s", host, (int) port, domain);
-    else if ((!domain) && !client_address.IsAnyAddr())
-        snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d-%s", host, (int) port, client_address.NtoA(ntoabuf,MAX_IPSTRLEN));
-    else
-        snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d", host, (int) port);
-
-    debugs(48,6,"PconnPool::key(" << (host?host:"(no host!)") << "," << port << "," << (domain?domain:"(no domain)") << "," << client_address << "is {" << buf << "}" );
+
+    destLink->remote.ToURL(buf, SQUIDHOSTNAMELEN * 3 + 10);
+    if (domain) {
+        int used = strlen(buf);
+        snprintf(buf+used, SQUIDHOSTNAMELEN * 3 + 10-used, "/%s", domain);
+    }
+
+    debugs(48,6,"PconnPool::key(" << destLink << ", " << (domain?domain:"[no domain]") << ") is {" << buf << "}" );
     return buf;
 }
 
 void
-PconnPool::dumpHist(StoreEntry * e)
+PconnPool::dumpHist(StoreEntry * e) const
 {
-    int i;
     storeAppendPrintf(e,
                       "%s persistent connection counts:\n"
                       "\n"
@@ -207,7 +227,7 @@
                       "\t----  ---------\n",
                       descr);
 
-    for (i = 0; i < PCONN_HIST_SZ; i++) {
+    for (int i = 0; i < PCONN_HIST_SZ; i++) {
         if (hist[i] == 0)
             continue;
 
@@ -216,14 +236,13 @@
 }
 
 void
-PconnPool::dumpHash(StoreEntry *e)
+PconnPool::dumpHash(StoreEntry *e) const
 {
-    int i;
-    hash_link *walker = NULL;
     hash_table *hid = table;
     hash_first(hid);
 
-    for (i = 0, walker = hid->next; walker; walker = hash_next(hid)) {
+    int i = 0;
+    for (hash_link *walker = hid->next; walker; walker = hash_next(hid)) {
         storeAppendPrintf(e, "\t item %5d: %s\n", i++, (char *)(walker->key));
     }
 }
@@ -232,10 +251,9 @@
 
 PconnPool::PconnPool(const char *aDescr) : table(NULL), descr(aDescr)
 {
-    int i;
     table = hash_create((HASHCMP *) strcmp, 229, hash_string);
 
-    for (i = 0; i < PCONN_HIST_SZ; i++)
+    for (int i = 0; i < PCONN_HIST_SZ; i++)
         hist[i] = 0;
 
     PconnModule::GetInstance()->add(this);
@@ -248,25 +266,22 @@
 }
 
 void
-PconnPool::push(int fd, const char *host, u_short port, const char *domain, Ip::Address &client_address)
+PconnPool::push(const Comm::ConnectionPointer &conn, const char *domain)
 {
-    IdleConnList *list;
-    const char *aKey;
-    LOCAL_ARRAY(char, desc, FD_DESC_SZ);
-
     if (fdUsageHigh()) {
         debugs(48, 3, "PconnPool::push: Not many unused FDs");
-        comm_close(fd);
+        Comm::ConnectionPointer nonConst = conn;
+        nonConst->close();
         return;
     } else if (shutting_down) {
-        comm_close(fd);
+        Comm::ConnectionPointer nonConst = conn;
+        nonConst->close();
         debugs(48, 3, "PconnPool::push: Squid is shutting down. Refusing to do anything");
         return;
     }
 
-    aKey = key(host, port, domain, client_address);
-
-    list = (IdleConnList *) hash_lookup(table, aKey);
+    const char *aKey = key(conn, domain);
+    IdleConnList *list = (IdleConnList *) hash_lookup(table, aKey);
 
     if (list == NULL) {
         list = new IdleConnList(aKey, this);
@@ -276,48 +291,41 @@
         debugs(48, 3, "PconnPool::push: found IdleConnList for {" << hashKeyStr(&list->hash) << "}" );
     }
 
-    list->push(fd);
+    list->push(conn);
+    assert(!comm_has_incomplete_write(conn->fd));
 
-    assert(!comm_has_incomplete_write(fd));
-    snprintf(desc, FD_DESC_SZ, "%s idle connection", host);
-    fd_note(fd, desc);
-    debugs(48, 3, "PconnPool::push: pushed FD " << fd << " for " << aKey);
+    LOCAL_ARRAY(char, desc, FD_DESC_SZ);
+    snprintf(desc, FD_DESC_SZ, "Idle: %s", aKey);
+    fd_note(conn->fd, desc);
+    debugs(48, 3, HERE << "pushed " << conn << " for " << aKey);
 }
 
-/**
- * Return a pconn fd for host:port if available and retriable.
- * Otherwise, return -1.
- *
- * We close available persistent connection if the caller transaction is not
- * retriable to avoid having a growing number of open connections when many
- * transactions create persistent connections but are not retriable.
- */
-int
-PconnPool::pop(const char *host, u_short port, const char *domain, Ip::Address &client_address, bool isRetriable)
+bool
+PconnPool::pop(Comm::ConnectionPointer &destLink, const char *domain, bool isRetriable)
 {
-    const char * aKey = key(host, port, domain, client_address);
+    const char * aKey = key(destLink, domain);
 
     IdleConnList *list = (IdleConnList *)hash_lookup(table, aKey);
     if (list == NULL) {
         debugs(48, 3, "PconnPool::pop: lookup for key {" << aKey << "} failed.");
-        return -1;
+        return false;
     } else {
         debugs(48, 3, "PconnPool::pop: found " << hashKeyStr(&list->hash) << (isRetriable?"(to use)":"(to kill)") );
     }
 
-    int fd = list->findUseableFD(); // search from the end. skip pending reads.
-
-    if (fd >= 0) {
-        list->clearHandlers(fd);
-        list->removeFD(fd);	/* might delete list */
-
-        if (!isRetriable) {
-            comm_close(fd);
-            return -1;
-        }
+    Comm::ConnectionPointer temp = list->findUseable(destLink);
+
+    if (Comm::IsConnOpen(temp)) {
+        list->clearHandlers(temp);
+
+        /* might delete list */
+        if (list->remove(temp) && !isRetriable)
+            temp->close();
+        else
+            destLink = temp;
     }
 
-    return fd;
+    return true;
 }
 
 void
@@ -344,7 +352,7 @@
 PconnModule::PconnModule() : pools(NULL), poolCount(0)
 {
     pools = (PconnPool **) xcalloc(MAX_NUM_PCONN_POOLS, sizeof(*pools));
-    pconn_fds_pool = memPoolCreate("pconn_fds", PCONN_FDS_SZ * sizeof(int));
+//TODO: re-link to MemPools. WAS:    pconn_fds_pool = memPoolCreate("pconn_fds", PCONN_FDS_SZ * sizeof(int));
     debugs(48, 0, "persistent connection module initialized");
     registerWithCacheManager();
 }
@@ -369,8 +377,7 @@
 
 void
 
-PconnModule::add
-(PconnPool *aPool)
+PconnModule::add(PconnPool *aPool)
 {
     assert(poolCount < MAX_NUM_PCONN_POOLS);
     *(pools+poolCount) = aPool;
@@ -380,9 +387,7 @@
 void
 PconnModule::dump(StoreEntry *e)
 {
-    int i;
-
-    for (i = 0; i < poolCount; i++) {
+    for (int i = 0; i < poolCount; i++) {
         storeAppendPrintf(e, "\n Pool %d Stats\n", i);
         (*(pools+i))->dumpHist(e);
         storeAppendPrintf(e, "\n Pool %d Hash Table\n",i);

=== modified file 'src/pconn.h'
--- src/pconn.h	2010-05-02 18:52:45 +0000
+++ src/pconn.h	2010-10-02 16:33:56 +0000
@@ -26,7 +26,10 @@
 /// \ingroup PConnAPI
 #define PCONN_HIST_SZ (1<<16)
 
-/// \ingroup PConnAPI
+/** \ingroup PConnAPI
+ * A list of connections currently open to a particular destination end-point.
+ * We currently define the end-point by the FQDN it is serving.
+ */
 class IdleConnList
 {
 public:
@@ -34,11 +37,26 @@
     ~IdleConnList();
     int numIdle() { return nfds; }
 
-    int findFDIndex(int fd); ///< search from the end of array
-    void removeFD(int fd);
-    void push(int fd);
-    int findUseableFD();     ///< find first from the end not pending read fd.
-    void clearHandlers(int fd);
+    int findIndex(const Comm::ConnectionPointer &conn); ///< search from the end of array
+
+    /**
+     * Search the list for an existing connection. Matches by FD.
+     *
+     * \retval false  The connection does not currently exist in the list.
+     *                We seem to have hit and lost a race condition.
+     *                Nevermind, but MUST NOT do anything with the raw FD.
+     */
+    bool remove(const Comm::ConnectionPointer &conn);
+
+    void push(const Comm::ConnectionPointer &conn);
+
+    /** Search the list for a connection which matches the 'key' details.
+     * The list is created based on remote IP:port hash. This further filters
+     * the choices based on specific local-end details requested.
+     * If nothing usable is found the key is returned unchanged.
+     */
+    Comm::ConnectionPointer findUseable(const Comm::ConnectionPointer &key);
+    void clearHandlers(const Comm::ConnectionPointer &conn);
 
 private:
     static IOCB read;
@@ -48,11 +66,11 @@
     hash_link hash;             /** must be first */
 
 private:
-    int *fds;
+    Comm::ConnectionPointer *theList;
     int nfds_alloc;
     int nfds;
     PconnPool *parent;
-    char fakeReadBuf[4096];
+    char fakeReadBuf[4096]; // TODO: kill magic number.
     CBDATA_CLASS2(IdleConnList);
 };
 
@@ -65,7 +83,10 @@
 /* for hash_table */
 #include "hash.h"
 
-/// \ingroup PConnAPI
+/** \ingroup PConnAPI
+ * A pool of persistent connections for a particular service type.
+ * HTTP servers being one such pool type, ICAP services another etc.
+ */
 class PconnPool
 {
 
@@ -74,28 +95,38 @@
     ~PconnPool();
 
     void moduleInit();
-    void push(int fd, const char *host, u_short port, const char *domain, Ip::Address &client_address);
-    int pop(const char *host, u_short port, const char *domain, Ip::Address &client_address, bool retriable);
+    void push(const Comm::ConnectionPointer &serverConn, const char *domain);
+
+    /**
+     * Updates destLink to point at an existing open connection if available and retriable.
+     * Otherwise, return false.
+     *
+     * We close available persistent connection if the caller transaction is not
+     * retriable to avoid having a growing number of open connections when many
+     * transactions create persistent connections but are not retriable.
+     */
+    bool pop(Comm::ConnectionPointer &serverConn, const char *domain, bool retriable);
     void count(int uses);
-    void dumpHist(StoreEntry *e);
-    void dumpHash(StoreEntry *e);
+    void dumpHist(StoreEntry *e) const;
+    void dumpHash(StoreEntry *e) const;
     void unlinkList(IdleConnList *list) const;
 
 private:
 
-    static const char *key(const char *host, u_short port, const char *domain, Ip::Address &client_address);
+    static const char *key(const Comm::ConnectionPointer &destLink, const char *domain);
 
     int hist[PCONN_HIST_SZ];
     hash_table *table;
     const char *descr;
-
 };
 
 
 class StoreEntry;
 class PconnPool;
 
-/// \ingroup PConnAPI
+/** \ingroup PConnAPI
+ * The global registry of persistent connection pools.
+ */
 class PconnModule
 {