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 80a840dd8dcf937304886a07f64670f00ab37107..40b7292c24038974ccd9b740745da1cb7b1cb097 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -267,7 +267,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 b8b08b1d7cbe505ee70ae3952c15aa083394e17e..3a899f1aa159dc52597a8f927f4d1251b1409dff 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);
 #if LINUX_TPROXY
@@ -784,16 +783,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->host;
     } else {
-        host = request->host;
-        port = request->port;
         ctimeout = Config.Timeout.connect;
     }
 
@@ -809,7 +801,16 @@ FwdState::connectStart()
     if (ftimeout < ctimeout)
         ctimeout = ftimeout;
 
-    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->host, client_addr, checkRetriable());
+    }
+    else {
+        host = request->host;
+        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;
@@ -1137,11 +1138,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, struct IN_ADDR *client_addr)
+FwdState::pconnPush(int fd, const peer *_peer, const HttpRequest *req, const char *domain, struct in_addr *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->host, req->port, NULL, client_addr);
+    }
 }
 
 void

diff --git a/src/forward.h b/src/forward.h
index ad9b0f15e7eef4cb3610bf041ddfbcb2369ef1ee..d587c50ce0ce8e7da8eb92b57f8b29e2017d1df7 100644 (file)
--- a/src/forward.h
+++ b/src/forward.h
@@ -5,6 +5,7 @@
 
 class CacheManager;
 class ErrorState;
+class HttpRequest;
 
 #include "comm.h"
 
@@ -44,7 +45,7 @@ public:
     bool checkRetry();
     bool checkRetriable();
     void dispatch();
-    void pconnPush(int fd, const char *host, int port, const char *domain, struct IN_ADDR *client_addr);
+    void pconnPush(int fd, const peer *_peer, const HttpRequest *req, const char *domain, struct in_addr *client_addr);
 
     bool dontRetry() { return flags.dont_retry; }
 

diff --git a/src/http.cc b/src/http.cc
index f11e7715e855257dd5f28ed07be2fb5d7fd426e1..28e63877eb4e2da4dc0f9aeed654efa06a3ee78c 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1167,6 +1167,7 @@ HttpStateData::processReplyBody()
 
             comm_remove_close_handler(fd, httpStateFree, this);
             fwd->unregister(fd);
+
 #if LINUX_TPROXY
 
             if (orig_request->flags.tproxy)
@@ -1174,14 +1175,7 @@ HttpStateData::processReplyBody()
 
 #endif
 
-            if (_peer) {
-                if (_peer->options.originserver)
-                    fwd->pconnPush(fd, _peer->name, orig_request->port, orig_request->host, client_addr);
-                else
-                    fwd->pconnPush(fd, _peer->name, _peer->http_port, NULL, client_addr);
-            } else {
-                fwd->pconnPush(fd, request->host, request->port, NULL, client_addr);
-            }
+            fwd->pconnPush(fd, _peer, request, orig_request->host, client_addr);
 
             fd = -1;
 

diff --git a/src/pconn.cc b/src/pconn.cc
index 16cd75db85cfe42d66afc13a949393a872568e3a..1412083a1336a34eb2d5972b0920f68c9ed74a19 100644 (file)
--- a/src/pconn.cc
+++ b/src/pconn.cc
@@ -1,6 +1,5 @@
-
 /*
- * $Id: pconn.cc,v 1.53.4.1 2008/02/24 12:06:41 amosjeffries Exp $
+ * $Id$
  *
  * DEBUG: section 48    Persistent Connections
  * AUTHOR: Duane Wessels
@@ -178,17 +177,18 @@ const char *
 
 PconnPool::key(const char *host, u_short port, const char *domain, struct IN_ADDR *client_address)
 {
-    LOCAL_ARRAY(char, buf, SQUIDHOSTNAMELEN * 2 + 10);
+    LOCAL_ARRAY(char, buf, SQUIDHOSTNAMELEN * 3 + 10);
 
     if (domain && client_address)
-        snprintf(buf, SQUIDHOSTNAMELEN * 2 + 10, "%s:%d-%s/%s", host, (int) port, inet_ntoa(*client_address), domain);
+        snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d-%s/%s", host, (int) port, inet_ntoa(*client_address), domain);
     else if (domain && (!client_address))
-        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)
-        snprintf(buf, SQUIDHOSTNAMELEN * 2 + 10, "%s:%d-%s", host, (int) port, inet_ntoa(*client_address));
+        snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d-%s", host, (int) port, inet_ntoa(*client_address));
     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 << "," << inet_ntoa(*client_address) << "is {" << buf << "}" );
     return buf;
 }
 
@@ -212,6 +212,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, stru
     } else if (shutting_down)
     {
         comm_close(fd);
+        debugs(48, 3, "PconnPool::push: Squid is shutting down. Refusing to do anything");
         return;
     }
 
@@ -253,8 +267,10 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, stru
     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);
@@ -265,7 +281,7 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, stru
     debugs(48, 3, "PconnPool::push: pushed FD " << fd << " for " << aKey);
 }
 
-/*
+/**
  * Return a pconn fd for host:port if available and retriable.
  * Otherwise, return -1.
  *
@@ -274,15 +290,17 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, stru
  * transactions create persistent connections but are not retriable.
  */
 int
-
-PconnPool::pop(const char *host, u_short port, const char *domain, struct IN_ADDR *client_address, bool isRetriable)
+PconnPool::pop(const char *host, u_short port, const char *domain, struct in_addr *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 8b08f07e82df238fc517a8c5f72a8d7fae04b579..98f7ae286a0737165708bd315d43fda83d8d4153 100644 (file)
--- a/src/pconn.h
+++ b/src/pconn.h
@@ -52,6 +52,7 @@ public:
     int pop(const char *host, u_short port, const char *domain, struct IN_ADDR *client_address, bool retriable);
     void count(int uses);
     void dumpHist(StoreEntry *e);
+    void dumpHash(StoreEntry *e);
     void unlinkList(IdleConnList *list) const;
 
 private: