Author: Amos Jeffries <squid3@treenet.co.nz>, Christos Tsantilas <chtsanti@users...

Add DNS lookup step to ICAP connect

This patch add an async DNS lookup step to the ICAP connection setup process.
As a side effect it adds a little bit of tcp_outgoing_address support to ICAP.
Its limited to "dst" ACL at present. So outgoing ACL selections depending on
HTTP request details wont work. Which makes sense since this connection may
be reused for multiple requests.

More than one IP result are not used  since there seems to be no sane place
to store multiple IPs between connect attempts. It currently relies on ipcache
MarkGood/MarkBad keeping a good usable IP at the top of the IP set.

This patch also try to make Max-Connections ICAP feature cooperate well with
the new DNS lookup code

diff --git a/src/adaptation/icap/ServiceRep.cc b/src/adaptation/icap/ServiceRep.cc
index 1b775e54901b9d5961b6e92dbef0a3a5dcddfe9e..2cdbb594541faefb0497c694322bac0f07dc933c 100644 (file)
--- a/src/adaptation/icap/ServiceRep.cc
+++ b/src/adaptation/icap/ServiceRep.cc
@@ -25,17 +25,19 @@ Adaptation::Icap::ServiceRep::ServiceRep(const ServiceConfigPointer &svcCfg):
         theBusyConns(0),
         theAllWaiters(0),
         connOverloadReported(false),
-        theIdleConns("ICAP Service",NULL),
+        theIdleConns(NULL),
         isSuspended(0), notifying(false),
         updateScheduled(false),
         wasAnnouncedUp(true), // do not announce an "up" service at startup
         isDetached(false)
 {
     setMaxConnections();
+    theIdleConns = new IdleConnList("ICAP Service", NULL);
 }
 
 Adaptation::Icap::ServiceRep::~ServiceRep()
 {
+    delete theIdleConns;
     Must(!theOptionsFetcher);
     delete theOptions;
 }
@@ -102,17 +104,13 @@ Adaptation::Icap::ServiceRep::getConnection(bool retriableXact, bool &reused)
      * In other words, (2) tells us to close one FD for each new one we open due to retriable.
      */
     if (retriableXact)
-        connection = theIdleConns.pop();
+        connection = theIdleConns->pop();
     else
-        theIdleConns.closeN(1);
-
-    if (!(reused = Comm::IsConnOpen(connection)))
-        connection = new Comm::Connection;
-    else {
-        debugs(93,3, HERE << "reused pconn " << connection);
-        ++theBusyConns;
-    }
+        theIdleConns->closeN(1);
 
+    reused = Comm::IsConnOpen(connection);
+    ++theBusyConns;
+    debugs(93,3, HERE << "got connection: " << connection);
     return connection;
 }
 
@@ -124,7 +122,7 @@ void Adaptation::Icap::ServiceRep::putConnection(const Comm::ConnectionPointer &
     if (isReusable && excessConnections() == 0) {
         debugs(93, 3, HERE << "pushing pconn" << comment);
         commUnsetConnTimeout(conn);
-        theIdleConns.push(conn);
+        theIdleConns->push(conn);
     } else {
         debugs(93, 3, HERE << "closing pconn" << comment);
         // comm_close will clear timeout
@@ -144,6 +142,12 @@ void Adaptation::Icap::ServiceRep::noteConnectionUse(const Comm::ConnectionPoint
     fd_table[conn->fd].noteUse(NULL); // pconn re-use but not via PconnPool API
 }
 
+void Adaptation::Icap::ServiceRep::noteConnectionFailed(const char *comment)
+{
+    debugs(93, 3, HERE << "Connection failed: " << comment);
+    --theBusyConns;
+}
+
 void Adaptation::Icap::ServiceRep::setMaxConnections()
 {
     if (cfg().maxConn >= 0)
@@ -171,8 +175,8 @@ int Adaptation::Icap::ServiceRep::availableConnections() const
     if (!available && !connOverloadReported) {
         debugs(93, DBG_IMPORTANT, "WARNING: ICAP Max-Connections limit " <<
                "exceeded for service " << cfg().uri << ". Open connections now: " <<
-               theBusyConns + theIdleConns.count() << ", including " <<
-               theIdleConns.count() << " idle persistent connections.");
+               theBusyConns + theIdleConns->count() << ", including " <<
+               theIdleConns->count() << " idle persistent connections.");
         connOverloadReported = true;
     }
 
@@ -191,7 +195,7 @@ int Adaptation::Icap::ServiceRep::excessConnections() const
     // Waiters affect the number of needed connections but a needed
     // connection may still be excessive from Max-Connections p.o.v.
     // so we should not account for waiting transaction needs here.
-    const int debt =  theBusyConns + theIdleConns.count() - theMaxConnections;
+    const int debt =  theBusyConns + theIdleConns->count() - theMaxConnections;
     if (debt > 0)
         return debt;
     else
@@ -378,7 +382,7 @@ void Adaptation::Icap::ServiceRep::callWhenAvailable(AsyncCall::Pointer &cb, boo
     debugs(93,8, "ICAPServiceRep::callWhenAvailable");
     Must(cb!=NULL);
     Must(up());
-    Must(!theIdleConns.count()); // or we should not be waiting
+    Must(!theIdleConns->count()); // or we should not be waiting
 
     Client i;
     i.service = Pointer(this);
@@ -560,11 +564,10 @@ void Adaptation::Icap::ServiceRep::handleNewOptions(Adaptation::Icap::Options *n
     setMaxConnections();
     const int excess = excessConnections();
     // if we owe connections and have idle pconns, close the latter
-    // XXX:  but ... idle pconn to *where*?
-    if (excess && theIdleConns.count() > 0) {
-        const int n = min(excess, theIdleConns.count());
+    if (excess && theIdleConns->count() > 0) {
+        const int n = min(excess, theIdleConns->count());
         debugs(93,5, HERE << "closing " << n << " pconns to relief debt");
-        theIdleConns.closeN(n);
+        theIdleConns->closeN(n);
     }
 
     scheduleNotification();

diff --git a/src/adaptation/icap/ServiceRep.h b/src/adaptation/icap/ServiceRep.h
index bcbe6ed16b675b2c56fdd2160ddd053acf39e243..f7c342469fa39ae1938da3f79f9a70c0f4f1a789 100644 (file)
--- a/src/adaptation/icap/ServiceRep.h
+++ b/src/adaptation/icap/ServiceRep.h
@@ -113,6 +113,7 @@ public:
     Comm::ConnectionPointer getConnection(bool isRetriable, bool &isReused);
     void putConnection(const Comm::ConnectionPointer &conn, bool isReusable, const char *comment);
     void noteConnectionUse(const Comm::ConnectionPointer &conn);
+    void noteConnectionFailed(const char *comment);
 
     void noteFailure(); // called by transactions to report service failure
 
@@ -160,7 +161,7 @@ private:
     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
-    IdleConnList theIdleConns; ///< idle persistent connection pool
+    IdleConnList *theIdleConns; ///< idle persistent connection pool
 
     FadingCounter theSessionFailures;
     const char *isSuspended; // also stores suspension reason for debugging

diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc
index a305eee4ddb0d5ac777a43b73f35f32126db93d1..a719326fa6adcc4cfded86d3f8ceb3c7d5b95968 100644 (file)
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -16,6 +16,7 @@
 #include "pconn.h"
 #include "HttpRequest.h"
 #include "HttpReply.h"
+#include "ipcache.h"
 #include "acl/FilledChecklist.h"
 #include "icap_log.h"
 #include "fde.h"
@@ -85,6 +86,13 @@ void Adaptation::Icap::Xaction::start()
     Must(static_cast<size_t>(readBuf.potentialSpaceSize()) <= commBufSize);
 }
 
+static void
+icapLookupDnsResults(const ipcache_addrs *ia, const DnsLookupDetails &, void *data)
+{
+    Adaptation::Icap::Xaction *xa = static_cast<Adaptation::Icap::Xaction *>(data);
+    xa->dnsLookupDone(ia);
+}
+
 // TODO: obey service-specific, OPTIONS-reported connection limit
 void
 Adaptation::Icap::Xaction::openConnection()
@@ -101,11 +109,6 @@ Adaptation::Icap::Xaction::openConnection()
 
     if (wasReused && Comm::IsConnOpen(connection)) {
         // Set comm Close handler
-        typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommCloseCbParams> CloseDialer;
-        closer =  asyncCall(93, 5, "Adaptation::Icap::Xaction::noteCommClosed",
-                            CloseDialer(this,&Adaptation::Icap::Xaction::noteCommClosed));
-        comm_add_close_handler(connection->fd, closer);
-
         // fake the connect callback
         // TODO: can we sync call Adaptation::Icap::Xaction::noteCommConnected here instead?
         typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommConnectCbParams> Dialer;
@@ -124,23 +127,42 @@ Adaptation::Icap::Xaction::openConnection()
     // Attempt to open a new connection...
     debugs(93,3, typeName << " opens connection to " << s.cfg().host.termedBuf() << ":" << s.cfg().port);
 
-    // TODO: find the IPs and attempt each one if this is a named service.
-    connection->remote = s.cfg().host.termedBuf();
-    connection->remote.SetPort(s.cfg().port);
+    // Locate the Service IP(s) to open
+    ipcache_nbgethostbyname(s.cfg().host.termedBuf(), icapLookupDnsResults, this);
+}
 
-    // TODO: service bypass status may differ from that of a transaction
-    typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommTimeoutCbParams> TimeoutDialer;
-    AsyncCall::Pointer timeoutCall =  asyncCall(93, 5, "Adaptation::Icap::Xaction::noteCommTimedout",
-                                      TimeoutDialer(this,&Adaptation::Icap::Xaction::noteCommTimedout));
+void
+Adaptation::Icap::Xaction::dnsLookupDone(const ipcache_addrs *ia)
+{
+    Adaptation::Icap::ServiceRep &s = service();
 
-    commSetTimeout(connection->fd, TheConfig.connect_timeout(
-                       service().cfg().bypass), timeoutCall);
+    if (ia == NULL) {
+        debugs(44, DBG_IMPORTANT, "ICAP: Unknown service host: " << s.cfg().host);
 
-    typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommCloseCbParams> CloseDialer;
-    closer =  asyncCall(93, 5, "Adaptation::Icap::Xaction::noteCommClosed",
-                        CloseDialer(this,&Adaptation::Icap::Xaction::noteCommClosed));
-    comm_add_close_handler(connection->fd, closer);
+#if WHEN_IPCACHE_NBGETHOSTBYNAME_USES_ASYNC_CALLS
+        dieOnConnectionFailure(); // throws
+#else // take a step back into protected Async call dialing.
+        // fake the connect callback
+        typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommConnectCbParams> Dialer;
+        CbcPointer<Xaction> self(this);
+        Dialer dialer(self, &Adaptation::Icap::Xaction::noteCommConnected);
+        dialer.params.conn = connection;
+        dialer.params.flag = COMM_ERROR;
+        // fake other parameters by copying from the existing connection
+        connector = asyncCall(93,3, "Adaptation::Icap::Xaction::noteCommConnected", dialer);
+        ScheduleCallHere(connector);
+#endif
+        return;
+    }
 
+    assert(ia->cur < ia->count);
+
+    connection = new Comm::Connection;
+    connection->remote = ia->in_addrs[ia->cur];
+    connection->remote.SetPort(s.cfg().port);
+    getOutgoingAddress(NULL, connection);
+
+    // TODO: service bypass status may differ from that of a transaction
     typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommConnectCbParams> ConnectDialer;
     connector = JobCallback(93,3, ConnectDialer, this, Adaptation::Icap::Xaction::noteCommConnected);
     Comm::ConnOpener *cs = new Comm::ConnOpener(connection, connector, TheConfig.connect_timeout(service().cfg().bypass));
@@ -206,6 +228,12 @@ void Adaptation::Icap::Xaction::noteCommConnected(const CommConnectCbParams &io)
     if (io.flag != COMM_OK)
         dieOnConnectionFailure(); // throws
 
+    typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommTimeoutCbParams> TimeoutDialer;
+    AsyncCall::Pointer timeoutCall =  asyncCall(93, 5, "Adaptation::Icap::Xaction::noteCommTimedout",
+                                      TimeoutDialer(this,&Adaptation::Icap::Xaction::noteCommTimedout));
+    commSetTimeout(io.conn->fd, TheConfig.connect_timeout(
+                       service().cfg().bypass), timeoutCall);
+
     typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommCloseCbParams> CloseDialer;
     closer =  asyncCall(93, 5, "Adaptation::Icap::Xaction::noteCommClosed",
                         CloseDialer(this,&Adaptation::Icap::Xaction::noteCommClosed));
@@ -221,6 +249,7 @@ void Adaptation::Icap::Xaction::dieOnConnectionFailure()
 {
     debugs(93, 2, HERE << typeName <<
            " failed to connect to " << service().cfg().uri);
+    service().noteConnectionFailed("failure");
     detailError(ERR_DETAIL_ICAP_XACT_START);
     throw TexcHere("cannot connect to the ICAP service");
 }
@@ -268,7 +297,11 @@ void Adaptation::Icap::Xaction::handleCommTimedout()
            theService->cfg().uri << status());
     reuseConnection = false;
     const bool whileConnecting = connector != NULL;
-    closeConnection(); // so that late Comm callbacks do not disturb bypass
+    if (whileConnecting) {
+        assert(!haveConnection());
+        theService->noteConnectionFailed("timedout");
+    } else
+        closeConnection(); // so that late Comm callbacks do not disturb bypass
     throw TexcHere(whileConnecting ?
                    "timed out while connecting to the ICAP service" :
                    "timed out while talking to the ICAP service");

diff --git a/src/adaptation/icap/Xaction.h b/src/adaptation/icap/Xaction.h
index 1d4339343a215f3ff4372f4b4c46410950bc3c97..c46fb8f95fb9a15f844fdfe59bc7c1f4ae52605e 100644 (file)
--- a/src/adaptation/icap/Xaction.h
+++ b/src/adaptation/icap/Xaction.h
@@ -41,6 +41,7 @@
 #include "adaptation/Initiate.h"
 #include "AccessLogEntry.h"
 #include "HttpReply.h"
+#include "ipcache.h"
 
 class CommConnectCbParams;
 
@@ -133,6 +134,7 @@ public:
     // custom exception handling and end-of-call checks
     virtual void callException(const std::exception  &e);
     virtual void callEnd();
+    void dnsLookupDone(const ipcache_addrs *ia);
 
 protected:
     // logging

diff --git a/src/comm.cc b/src/comm.cc
index 9abc343ae6d601886a8ca176750f183c48fdd106..35b3b2a0aae920f0d5c6053d80efd490e38813f3 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -1156,7 +1156,7 @@ _comm_close(int fd, char const *file, int line)
 
     commCallCloseHandlers(fd);
 
-    if (F->pconn.uses)
+    if (F->pconn.uses && F->pconn.pool)
         F->pconn.pool->noteUses(F->pconn.uses);
 
     comm_empty_os_read_buffers(fd);