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 4e2a5bdc6b4637855a96dc5562d5a5493399a4e5..4249a62a6728f396571ad2166180a02e0c375725 100644 (file)
--- 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 3975dfc5d0ccfe5ca7494d3609ea2c0fde128e23..40a943e8a63ba16e301f6222575cdf748d39a4af 100644 (file)
--- 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 6de905be6736950a51b24ed7d8749110c699442f..ac4ae03caed228457b0ad552ebb2774528367a90 100644 (file)
--- 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 0d3ec5d8a9d74d4e6eb1c10e7540502cf55694cd..a2d4df6ec51e26c045d00f716185a20649c3ddfa 100644 (file)
--- 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;