From fb505fa1d66db2c2746aea9e8249890ba05c8436 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Tue, 12 Jul 2011 22:02:48 +0300 Subject: [PATCH] Author: Amos Jeffries , Christos Tsantilas 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 --- src/adaptation/icap/ServiceRep.cc | 41 +++++++++--------- src/adaptation/icap/ServiceRep.h | 3 +- src/adaptation/icap/Xaction.cc | 71 ++++++++++++++++++++++--------- src/adaptation/icap/Xaction.h | 2 + src/comm.cc | 2 +- 5 files changed, 79 insertions(+), 40 deletions(-) diff --git a/src/adaptation/icap/ServiceRep.cc b/src/adaptation/icap/ServiceRep.cc index 1b775e5490..2cdbb59454 100644 --- 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 bcbe6ed16b..f7c342469f 100644 --- 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 a305eee4dd..a719326fa6 100644 --- 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(readBuf.potentialSpaceSize()) <= commBufSize); } +static void +icapLookupDnsResults(const ipcache_addrs *ia, const DnsLookupDetails &, void *data) +{ + Adaptation::Icap::Xaction *xa = static_cast(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 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 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 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 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 Dialer; + CbcPointer 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 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 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 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 1d4339343a..c46fb8f95f 100644 --- 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 9abc343ae6..35b3b2a0aa 100644 --- 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); -- 2.39.5