From: rousskov <>
Date: Fri, 11 May 2007 19:20:57 +0000 (+0000)
Subject: Bug #1957 fix: Close a persistent ICAP connection if we have to open a
X-Git-Tag: SQUID_3_0_PRE7~277
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c8ceec27fb6c783905751c3d8c471a626bd2f7af;p=thirdparty%2Fsquid.git

Bug #1957 fix: Close a persistent ICAP connection if we have to open a
new connection because the transaction is not retriable.

This change avoids a growing number of open connections when many
transactions create persistent connections but only few are retriable
and can reuse them.

FwdState was already doing that. I moved FwdState logic to
PconnPool::pop so that any PconnPool user thinks about the problem and
benefits from the common solution. The change should have no material
affect on FwdState.
---

diff --git a/src/ICAP/ICAPXaction.cc b/src/ICAP/ICAPXaction.cc
index 4e2a5bdc6b..4249a62a67 100644
--- a/src/ICAP/ICAPXaction.cc
+++ b/src/ICAP/ICAPXaction.cc
@@ -102,26 +102,22 @@ void ICAPXaction::openConnection()
 
     const ICAPServiceRep &s = service();
 
-    // if we cannot retry, we must not reuse pconns because of race conditions
-    if (isRetriable) {
-        // TODO: check whether NULL domain is appropriate here
-        connection = icapPconnPool->pop(s.host.buf(), s.port, NULL, NULL);
-
-        if (connection >= 0) {
-            debugs(93,3, HERE << "reused pconn FD " << connection);
-            connector = &ICAPXaction_noteCommConnected; // make doneAll() false
-            eventAdd("ICAPXaction::reusedConnection",
-                 reusedConnection,
-                 this,
-                 0.0,
-                 0,
-                 true);
-            return;
-        }
-
-        disableRetries(); // we only retry pconn failures
+    // TODO: check whether NULL domain is appropriate here
+    connection = icapPconnPool->pop(s.host.buf(), s.port, NULL, NULL, isRetriable);
+    if (connection >= 0) {
+        debugs(93,3, HERE << "reused pconn FD " << connection);
+        connector = &ICAPXaction_noteCommConnected; // make doneAll() false
+        eventAdd("ICAPXaction::reusedConnection",
+             reusedConnection,
+             this,
+             0.0,
+             0,
+             true);
+        return;
     }
 
+    disableRetries(); // we only retry pconn failures
+
     connection = comm_open(SOCK_STREAM, 0, getOutgoingAddr(NULL), 0,
         COMM_NONBLOCKING, s.uri.buf());
 
diff --git a/src/forward.cc b/src/forward.cc
index 3975dfc5d0..40a943e8a6 100644
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: forward.cc,v 1.163 2007/04/30 16:56:09 wessels Exp $
+ * $Id: forward.cc,v 1.164 2007/05/11 13:20:57 rousskov Exp $
  *
  * DEBUG: section 17    Request Forwarding
  * AUTHOR: Duane Wessels
@@ -784,27 +784,20 @@ FwdState::connectStart()
     if (ftimeout < ctimeout)
         ctimeout = ftimeout;
 
-    if ((fd = fwdPconnPool->pop(host, port, domain, client_addr)) >= 0) {
-        if (checkRetriable()) {
-            debugs(17, 3, "fwdConnectStart: reusing pconn FD " << fd);
-            server_fd = fd;
-            n_tries++;
+    fd = fwdPconnPool->pop(host, port, domain, client_addr, checkRetriable());
+    if (fd >= 0) {
+        debugs(17, 3, "fwdConnectStart: reusing pconn FD " << fd);
+        server_fd = fd;
+        n_tries++;
 
-            if (!fs->_peer)
-                origin_tries++;
+        if (!fs->_peer)
+            origin_tries++;
 
-            comm_add_close_handler(fd, fwdServerClosedWrapper, this);
+        comm_add_close_handler(fd, fwdServerClosedWrapper, this);
 
-            dispatch();
+        dispatch();
 
-            return;
-        } else {
-            /* Discard the persistent connection to not cause
-             * an imbalance in number of connections open if there
-             * is a lot of POST requests
-             */
-            comm_close(fd);
-        }
+        return;
     }
 
 #if URL_CHECKSUM_DEBUG
diff --git a/src/pconn.cc b/src/pconn.cc
index 6de905be67..ac4ae03cae 100644
--- a/src/pconn.cc
+++ b/src/pconn.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: pconn.cc,v 1.50 2007/04/30 16:56:09 wessels Exp $
+ * $Id: pconn.cc,v 1.51 2007/05/11 13:20:57 rousskov Exp $
  *
  * DEBUG: section 48    Persistent Connections
  * AUTHOR: Duane Wessels
@@ -266,11 +266,16 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, stru
 }
 
 /*
- * return a pconn fd for host:port, or -1 if none are available
+ * 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, struct IN_ADDR *client_address)
+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);
@@ -285,6 +290,11 @@ PconnPool::pop(const char *host, u_short port, const char *domain, struct IN_ADD
     {
         list->clearHandlers(fd);
         list->removeFD(fd);	/* might delete list */
+
+        if (!isRetriable) {
+            comm_close(fd);
+            return -1;
+        }
     }
 
     return fd;
diff --git a/src/pconn.h b/src/pconn.h
index 0d3ec5d8a9..a2d4df6ec5 100644
--- a/src/pconn.h
+++ b/src/pconn.h
@@ -49,7 +49,7 @@ public:
 
     void moduleInit();
     void push(int fd, const char *host, u_short port, const char *domain, struct IN_ADDR *client_address);
-    int pop(const char *host, u_short port, const char *domain, struct IN_ADDR *client_address);
+    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 unlinkList(IdleConnList *list) const;