---------------------
PatchSet 10791 
Date: 2007/05/11 13:20:57
Author: rousskov
Branch: HEAD
Tag: (none) 
Log:
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.

Members: 
	src/forward.cc:1.163->1.164 
	src/pconn.cc:1.50->1.51 
	src/pconn.h:1.3->1.4 
	src/ICAP/ICAPXaction.cc:1.14->1.15 

Index: squid3/src/forward.cc
===================================================================
RCS file: /cvsroot/squid/squid3/src/forward.cc,v
retrieving revision 1.163
retrieving revision 1.164
diff -u -r1.163 -r1.164
--- squid3/src/forward.cc	30 Apr 2007 16:56:09 -0000	1.163
+++ squid3/src/forward.cc	11 May 2007 13:20:57 -0000	1.164
@@ -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 @@
     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
Index: squid3/src/pconn.cc
===================================================================
RCS file: /cvsroot/squid/squid3/src/pconn.cc,v
retrieving revision 1.50
retrieving revision 1.51
diff -u -r1.50 -r1.51
--- squid3/src/pconn.cc	30 Apr 2007 16:56:09 -0000	1.50
+++ squid3/src/pconn.cc	11 May 2007 13:20:57 -0000	1.51
@@ -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 @@
 }
 
 /*
- * 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 @@
     {
         list->clearHandlers(fd);
         list->removeFD(fd);	/* might delete list */
+
+        if (!isRetriable) {
+            comm_close(fd);
+            return -1;
+        }
     }
 
     return fd;
Index: squid3/src/pconn.h
===================================================================
RCS file: /cvsroot/squid/squid3/src/pconn.h,v
retrieving revision 1.3
retrieving revision 1.4
diff -u -r1.3 -r1.4
--- squid3/src/pconn.h	15 Apr 2007 14:46:17 -0000	1.3
+++ squid3/src/pconn.h	11 May 2007 13:20:57 -0000	1.4
@@ -49,7 +49,7 @@
 
     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;
Index: squid3/src/ICAP/ICAPXaction.cc
===================================================================
RCS file: /cvsroot/squid/squid3/src/ICAP/ICAPXaction.cc,v
retrieving revision 1.14
retrieving revision 1.15
diff -u -r1.14 -r1.15
--- squid3/src/ICAP/ICAPXaction.cc	8 May 2007 16:32:12 -0000	1.14
+++ squid3/src/ICAP/ICAPXaction.cc	11 May 2007 13:20:57 -0000	1.15
@@ -102,26 +102,22 @@
 
     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());
 
