Okay. after IRC chatter. Here is version 2. Which retains the
sub-optimal pconn discard on non-retriable requests.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.12
Beta testers wanted for 3.2.0.8 and 3.1.12.2
=== modified file 'src/adaptation/icap/ServiceRep.cc'
--- src/adaptation/icap/ServiceRep.cc 2011-05-13 10:38:28 +0000
+++ src/adaptation/icap/ServiceRep.cc 2011-06-16 15:01:27 +0000
@@ -24,7 +24,7 @@
theBusyConns(0),
theAllWaiters(0),
connOverloadReported(false),
- theIdleConns("ICAP Service"),
+ theIdleConns("ICAP Service",NULL),
isSuspended(0), notifying(false),
updateScheduled(false),
wasAnnouncedUp(true), // do not announce an "up" service at startup
@@ -85,8 +85,26 @@
{
Ip::Address client_addr;
- int connection = theIdleConns.pop(cfg().host.termedBuf(), cfg().port, NULL, client_addr,
- retriableXact);
+ int connection = -1;
+
+ /* 2011-06-17: rousskov:
+ * There are two things that happen at the same time in pop(). Both are important.
+ * 1) Ensure that we can use a pconn for this transaction.
+ * 2) Ensure that the number of idle pconns does not grow without bounds.
+ *
+ * Both happen in the beginning of the transaction. Both are dictated by real-world problems.
+ * retriable means you can repeat the request if you suspect the first try failed due to a pconn race.
+ * HTTP and ICAP rules prohibit the use of pconns for non-retriable requests.
+ *
+ * If there are zero idle connections, (2) is irrelevant. (2) is only relevant when there are many
+ * idle connections and we should not open more connections without closing some idle ones,
+ * or instead of just opening a new connection and leaving idle connections as is.
+ * In other words, (2) tells us to close one FD for each new one we open due to retriable.
+ */
+ if (retriableXact)
+ connection = theIdleConns.findUseableFD();
+ else
+ theIdleConns.closeN(1);
reused = connection >= 0; // reused a persistent connection
@@ -116,7 +134,7 @@
debugs(93, 3, HERE << "pushing pconn" << comment);
commSetTimeout(fd, -1, NULL, NULL);
Ip::Address anyAddr;
- theIdleConns.push(fd, cfg().host.termedBuf(), cfg().port, NULL, anyAddr);
+ theIdleConns.push(fd);
} else {
debugs(93, 3, HERE << "closing pconn" << comment);
// comm_close will clear timeout
@@ -133,7 +151,7 @@
void Adaptation::Icap::ServiceRep::noteConnectionUse(int fd)
{
Must(fd >= 0);
- fd_table[fd].noteUse(&theIdleConns);
+ fd_table[fd].noteUse(NULL); // pconn re-use but not via PconnPool API
}
void Adaptation::Icap::ServiceRep::setMaxConnections()
@@ -554,8 +572,7 @@
if (excess && theIdleConns.count() > 0) {
const int n = min(excess, theIdleConns.count());
debugs(93,5, HERE << "closing " << n << " pconns to relief debt");
- Ip::Address anyAddr;
- theIdleConns.closeN(n, cfg().host.termedBuf(), cfg().port, NULL, anyAddr);
+ theIdleConns.closeN(n);
}
scheduleNotification();
=== modified file 'src/adaptation/icap/ServiceRep.h'
--- src/adaptation/icap/ServiceRep.h 2011-05-13 10:38:28 +0000
+++ src/adaptation/icap/ServiceRep.h 2011-06-14 22:55:02 +0000
@@ -160,7 +160,7 @@
int theMaxConnections; ///< the maximum allowed connections to the service
// TODO: use a better type like the FadingCounter for connOverloadReported
mutable bool connOverloadReported; ///< whether we reported exceeding theMaxConnections
- PconnPool theIdleConns; ///< idle persistent connection pool
+ IdleConnList theIdleConns; ///< idle persistent connection pool
FadingCounter theSessionFailures;
const char *isSuspended; // also stores suspension reason for debugging
=== modified file 'src/pconn.cc'
--- src/pconn.cc 2011-05-13 10:38:28 +0000
+++ src/pconn.cc 2011-06-15 02:26:58 +0000
@@ -57,8 +57,8 @@
IdleConnList::~IdleConnList()
{
-
- parent->unlinkList(this);
+ if (parent)
+ parent->unlinkList(this);
if (nfds_alloc == PCONN_FDS_SZ)
pconn_fds_pool->freeOne(fds);
@@ -97,7 +97,53 @@
if (parent)
parent->noteConnectionRemoved();
- if (--nfds == 0) {
+ if (parent && --nfds == 0) {
+ debugs(48, 3, "IdleConnList::removeFD: deleting " << hashKeyStr(&hash));
+ delete this;
+ }
+}
+
+// almost a duplicate of removeFD. But drops multiple entries.
+void
+IdleConnList::closeN(size_t n)
+{
+ if (n < 1) {
+ debugs(48, 2, HERE << "Nothing to do.");
+ return;
+ } else if (n < (size_t)count()) {
+ debugs(48, 2, HERE << "Closing all entries.");
+ while (nfds >= 0) {
+ int fd = fds[--nfds];
+ fds[nfds] = -1;
+ clearHandlers(fd);
+ comm_close(fd);
+ if (parent)
+ parent->noteConnectionRemoved();
+ }
+ } else {
+ debugs(48, 2, HERE << "Closing " << n << " of " << nfds << " entries.");
+
+ size_t index = 0;
+ // ensure the first N entries are closed
+ while (index < n) {
+ int fd = fds[--nfds];
+ fds[nfds] = -1;
+ clearHandlers(fd);
+ comm_close(fd);
+ if (parent)
+ parent->noteConnectionRemoved();
+ }
+ // shuffle the list N down.
+ for (;index < (size_t)nfds; index++) {
+ fds[index - n] = fds[index];
+ }
+ // ensure the last N entries are unset
+ while (index < ((size_t)nfds) + n) {
+ fds[index] = -1;
+ }
+ }
+
+ if (parent && nfds == 0) {
debugs(48, 3, "IdleConnList::removeFD: deleting " << hashKeyStr(&hash));
delete this;
}
=== modified file 'src/pconn.h'
--- src/pconn.h 2011-05-13 10:38:28 +0000
+++ src/pconn.h 2011-06-15 00:07:46 +0000
@@ -35,6 +35,7 @@
int findFDIndex(int fd); ///< search from the end of array
void removeFD(int fd);
+ void closeN(size_t count);
void push(int fd);
int findUseableFD(); ///< find first from the end not pending read fd.
void clearHandlers(int fd);
