Author: Francesco Chemolli + Amos Jeffries

Pconn not being used when they should.

A slight misalignment between the keys generated for push and pop of
connections to the waiting pool caused new connections never to match
any of the existing connections.

This patch makes several alterations to achieve a fix:
 - reduces the FwdState push logics down into a simple selection in
   pconnPush function which previously was a dumb wrapper.
 - adds a dump of current hash keys to the cacheManager pconn report
 - adds much better debugging to the pconn process at level 48,3 and 48,6
 - adds some additional documentation of code to the related call tree

  Pconn API after this patch :

The Pconn KEY takes several parameters (host, port, domain, client-ip).

For HTTP requests this is normally generated from the request data of
same name with domain being optional since it may be identical to host.

However for peer-sourced requests this alters slightly and the host:port
fields become the peer NAME and HTTP-PORT.

This means the pconn key in abstract becomes a key to the TCP remote-end of
the link with an optional anchor on the domain being requested.

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 4685e3fd042979600bec4a6e33d52429908fd1b4..f921a854e843e92bb64a39c8535295bbce3d41c3 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -326,7 +326,7 @@ HttpRequest::prefixLen()
            header.len + 2;
 }
 
-/*
+/**
  * Returns true if HTTP allows us to pass this header on.  Does not
  * check anonymizer (aka header_access) configuration.
  */

diff --git a/src/forward.cc b/src/forward.cc
index 1b0341812e62cd9fda90f2ea9ee94e031ccca467..0c5ebd4e219dd52405c99d67fe565a6f72126f0b 100644 (file)
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -767,7 +767,6 @@ FwdState::connectStart()
     FwdServer *fs = servers;
     const char *host;
     unsigned short port;
-    const char *domain = NULL;
     int ctimeout;
     int ftimeout = Config.Timeout.forward - (squid_curtime - start_t);
 
@@ -780,16 +779,9 @@ FwdState::connectStart()
     debugs(17, 3, "fwdConnectStart: " << url);
 
     if (fs->_peer) {
-        host = fs->_peer->host;
-        port = fs->_peer->http_port;
         ctimeout = fs->_peer->connect_timeout > 0 ? fs->_peer->connect_timeout
                    : Config.Timeout.peer_connect;
-
-        if (fs->_peer->options.originserver)
-            domain = request->GetHost();
     } else {
-        host = request->GetHost();
-        port = request->port;
         ctimeout = Config.Timeout.connect;
     }
 
@@ -831,7 +823,16 @@ FwdState::connectStart()
         return;
     }
 
-    fd = fwdPconnPool->pop(host, port, domain, client_addr, checkRetriable());
+    if(fs->_peer) {
+        host = fs->_peer->host;
+        port = fs->_peer->http_port;
+        fd = fwdPconnPool->pop(fs->_peer->name, fs->_peer->http_port, request->GetHost(), client_addr, checkRetriable());
+    }
+    else {
+        host = request->GetHost();
+        port = request->port;
+        fd = fwdPconnPool->pop(host, port, NULL, client_addr, checkRetriable());
+    }
     if (fd >= 0) {
         debugs(17, 3, "fwdConnectStart: reusing pconn FD " << fd);
         server_fd = fd;
@@ -1175,11 +1176,24 @@ FwdState::reforwardableStatus(http_status s)
     /* NOTREACHED */
 }
 
+/**
+ * Decide where details need to be gathered to correctly describe a persistent connection.
+ * What is needed:
+ * \item  host name of server at other end of this link (either peer or requested host)
+ * \item  port to which we connected the other end of this link (for peer or request)
+ * \item  domain for which the connection is supposed to be used
+ * \item  address of the client for which we made the connection
+ */
 void
-
-FwdState::pconnPush(int fd, const char *host, int port, const char *domain, IPAddress &client_addr)
+FwdState::pconnPush(int fd, const peer *_peer, const HttpRequest *req, const char *domain, IPAddress &client_addr)
 {
-    fwdPconnPool->push(fd, host, port, domain, client_addr);
+    if (_peer) {
+        fwdPconnPool->push(fd, _peer->name, _peer->http_port, domain, client_addr);
+    } else {
+        /* small performance improvement, using NULL for domain instead of listing it twice */
+        /* although this will leave a gap open for url-rewritten domains to share a link */
+        fwdPconnPool->push(fd, req->GetHost(), req->port, NULL, client_addr);
+    }
 }
 
 void

diff --git a/src/forward.h b/src/forward.h
index dbfffe391f56a4350b3d55655cdf7b773df82048..d82dc188642fdff5cf6e339e36ae8efd05fbd8cd 100644 (file)
--- a/src/forward.h
+++ b/src/forward.h
@@ -4,6 +4,7 @@
 /* forward decls */
 
 class ErrorState;
+class HttpRequest;
 
 #include "comm.h"
 #include "IPAddress.h"
@@ -43,7 +44,7 @@ public:
     bool checkRetry();
     bool checkRetriable();
     void dispatch();
-    void pconnPush(int fd, const char *host, int port, const char *domain, IPAddress &client_addr);
+    void pconnPush(int fd, const peer *_peer, const HttpRequest *req, const char *domain, IPAddress &client_addr);
 
     bool dontRetry() { return flags.dont_retry; }
 

diff --git a/src/http.cc b/src/http.cc
index 83199ba03e9998f1fae6d670d36e1ffcba38f593..60bbc74c3c02b69c4efcb49e10f418e58c47a422 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1283,13 +1283,8 @@ HttpStateData::processReplyBody()
             if (orig_request->pinnedConnection() && ispinned) {
                 orig_request->pinnedConnection()->pinConnection(fd, orig_request, _peer,
                         (request->flags.connection_auth != 0));
-            } else if (_peer) {
-                if (_peer->options.originserver)
-                    fwd->pconnPush(fd, _peer->name, orig_request->port, orig_request->GetHost(), client_addr);
-                else
-                    fwd->pconnPush(fd, _peer->name, _peer->http_port, NULL, client_addr);
             } else {
-                fwd->pconnPush(fd, request->GetHost(), request->port, NULL, client_addr);
+                fwd->pconnPush(fd, _peer, request, orig_request->GetHost(), client_addr);
             }
 
             fd = -1;

diff --git a/src/pconn.cc b/src/pconn.cc
index a5252a94ce8b93960cdb743e40b98dc7668e59f2..6d1790e2a0d1a0147970199eeb072ab682880bb2 100644 (file)
--- a/src/pconn.cc
+++ b/src/pconn.cc
@@ -1,6 +1,5 @@
-
 /*
- * $Id: pconn.cc,v 1.55 2007/12/27 01:03:13 hno Exp $
+ * $Id$
  *
  * DEBUG: section 48    Persistent Connections
  * AUTHOR: Duane Wessels
@@ -180,18 +179,19 @@ IdleConnList::timeout(int fd, void *data)
 const char *
 PconnPool::key(const char *host, u_short port, const char *domain, IPAddress &client_address)
 {
-    LOCAL_ARRAY(char, buf, SQUIDHOSTNAMELEN * 2 + 10);
+    LOCAL_ARRAY(char, buf, SQUIDHOSTNAMELEN * 3 + 10);
     char ntoabuf[MAX_IPSTRLEN];
 
     if (domain && !client_address.IsAnyAddr())
-        snprintf(buf, SQUIDHOSTNAMELEN * 2 + 10, "%s:%d-%s/%s", host, (int) port, client_address.NtoA(ntoabuf,MAX_IPSTRLEN), domain);
+        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 * 2 + 10, "%s:%d/%s", host, (int) port, domain);
+        snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d/%s", host, (int) port, domain);
     else if ((!domain) && !client_address.IsAnyAddr())
-        snprintf(buf, SQUIDHOSTNAMELEN * 2 + 10, "%s:%d-%s", host, (int) port, client_address.NtoA(ntoabuf,MAX_IPSTRLEN));
+        snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d-%s", host, (int) port, client_address.NtoA(ntoabuf,MAX_IPSTRLEN));
     else
-        snprintf(buf, SQUIDHOSTNAMELEN * 2 + 10, "%s:%d", host, (int) port);
+        snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d", host, (int) port);
 
+    debugs(48,6,"PconnPool::key(" << host << "," << port << "," << domain << "," << client_address << "is {" << buf << "}" );
     return buf;
 }
 
@@ -215,6 +215,19 @@ PconnPool::dumpHist(StoreEntry * e)
     }
 }
 
+void
+PconnPool::dumpHash(StoreEntry *e)
+{
+    int i;
+    hash_link *walker = NULL;
+    hash_table *hid = table;
+    hash_first(hid);
+
+    for (i = 0, walker = hid->next; walker; walker = hash_next(hid)) {
+        storeAppendPrintf(e, "\t item %5d: %s\n", i++, (char *)(walker->key));
+    }
+}
+
 /* ========== PconnPool PUBLIC FUNCTIONS ============================================ */
 
 PconnPool::PconnPool(const char *aDescr) : table(NULL), descr(aDescr)
@@ -243,6 +256,7 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, IPAd
         return;
     } else if (shutting_down) {
         comm_close(fd);
+        debugs(48, 3, "PconnPool::push: Squid is shutting down. Refusing to do anything");
         return;
     }
 
@@ -252,8 +266,10 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, IPAd
 
     if (list == NULL) {
         list = new IdleConnList(aKey, this);
-        debugs(48, 3, "pconnNew: adding " << hashKeyStr(&list->hash));
+        debugs(48, 3, "PconnPool::push: new IdleConnList for {" << hashKeyStr(&list->hash) << "}" );
         hash_join(table, &list->hash);
+    } else {
+        debugs(48, 3, "PconnPool::push: found IdleConnList for {" << hashKeyStr(&list->hash) << "}" );
     }
 
     list->push(fd);
@@ -264,7 +280,7 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, IPAd
     debugs(48, 3, "PconnPool::push: pushed FD " << fd << " for " << aKey);
 }
 
-/*
+/**
  * Return a pconn fd for host:port if available and retriable.
  * Otherwise, return -1.
  *
@@ -273,15 +289,17 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, IPAd
  * transactions create persistent connections but are not retriable.
  */
 int
-
 PconnPool::pop(const char *host, u_short port, const char *domain, IPAddress &client_address, bool isRetriable)
 {
-    IdleConnList *list;
     const char * aKey = key(host, port, domain, client_address);
-    list = (IdleConnList *)hash_lookup(table, aKey);
 
-    if (list == NULL)
+    IdleConnList *list = (IdleConnList *)hash_lookup(table, aKey);
+    if (list == NULL) {
+        debugs(48, 3, "PconnPool::pop: lookup for key {" << aKey << "} failed.");
         return -1;
+    } 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.
 
@@ -361,7 +379,10 @@ PconnModule::dump(StoreEntry *e)
     int i;
 
     for (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);
+        (*(pools+i))->dumpHash(e);
     }
 }
 

diff --git a/src/pconn.h b/src/pconn.h
index 3253d88bb11655f8a7ecd46f4036f02186196b7a..836c04e912aa1063aef988de10236aa2f40628cd 100644 (file)
--- a/src/pconn.h
+++ b/src/pconn.h
@@ -76,6 +76,7 @@ public:
     int pop(const char *host, u_short port, const char *domain, IPAddress &client_address, bool retriable);
     void count(int uses);
     void dumpHist(StoreEntry *e);
+    void dumpHash(StoreEntry *e);
     void unlinkList(IdleConnList *list) const;
 
 private: