From e884bbde54bd98d7e668d759128eeee9e41d3b99 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Fri, 3 Dec 2010 13:08:30 +1300 Subject: [PATCH] Some logic fixes and optimizations from run testing --- src/comm.cc | 10 +++++----- src/comm/ConnOpener.cc | 43 ++++++++++++++++++++++-------------------- src/pconn.cc | 26 +++++++++++++++++++------ 3 files changed, 48 insertions(+), 31 deletions(-) diff --git a/src/comm.cc b/src/comm.cc index 0738f080d5..0aaab34e77 100644 --- a/src/comm.cc +++ b/src/comm.cc @@ -212,6 +212,7 @@ comm_empty_os_read_buffers(int fd) /** * Return whether the FD has a pending completed callback. + * NP: does not work. */ int comm_has_pending_read_callback(int fd) @@ -526,10 +527,6 @@ comm_openex(int sock_type, int new_socket; struct addrinfo *AI = NULL; - // temporary for the transition. comm_openex will eventually have a conn to play with. - Comm::ConnectionPointer conn = new Comm::Connection; - conn->local = addr; - PROF_start(comm_open); /* Create socket for accepting new connections. */ statCounter.syscalls.sock.sockets++; @@ -539,7 +536,7 @@ comm_openex(int sock_type, AI->ai_socktype = sock_type; AI->ai_protocol = proto; - debugs(50, 3, "comm_openex: Attempt open socket for: " << conn ); + debugs(50, 3, "comm_openex: Attempt open socket for: " << addr ); new_socket = socket(AI->ai_family, AI->ai_socktype, AI->ai_protocol); /* under IPv6 there is the possibility IPv6 is present but disabled. */ @@ -574,6 +571,9 @@ comm_openex(int sock_type, return -1; } + // temporary for the transition. comm_openex will eventually have a conn to play with. + Comm::ConnectionPointer conn = new Comm::Connection; + conn->local = addr; conn->fd = new_socket; debugs(50, 3, "comm_openex: Opened socket " << conn << " : family=" << AI->ai_family << ", type=" << AI->ai_socktype << ", protocol=" << AI->ai_protocol ); diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc index c67f122dc0..ddf96ae535 100644 --- a/src/comm/ConnOpener.cc +++ b/src/comm/ConnOpener.cc @@ -99,10 +99,24 @@ Comm::ConnOpener::getHost() const /** * Connection attempt are completed. One way or the other. * Pass the results back to the external handler. + * NP: on connection errors the connection close() must be called first. */ void Comm::ConnOpener::doneConnecting(comm_err_t status, int xerrno) { + // only mark the address good/bad AFTER connect is finished. + if (host_ != NULL) { + if (xerrno == 0) + ipcacheMarkGoodAddr(host_, conn_->remote); + else { + ipcacheMarkBadAddr(host_, conn_->remote); +#if USE_ICMP + if (Config.onoff.test_reachability) + netdbDeleteAddrNetwork(conn_->remote); +#endif + } + } + if (callback_ != NULL) { typedef CommConnectCbParams Params; Params ¶ms = GetCommParams(callback_); @@ -189,6 +203,8 @@ Comm::ConnOpener::connect() // check for timeout FIRST. if (squid_curtime - connectStart_ > connectTimeout_) { debugs(5, 5, HERE << conn_ << ": * - ERR took too long already."); + calls_.earlyAbort_->cancel("Comm::ConnOpener::connect timed out"); + calls_.earlyAbort_ = NULL; conn_->close(); doneConnecting(COMM_TIMEOUT, errno); return; @@ -200,42 +216,29 @@ Comm::ConnOpener::connect() case COMM_OK: debugs(5, 5, HERE << conn_ << ": COMM_OK - connected"); - connected(); - - if (host_ != NULL) - ipcacheMarkGoodAddr(host_, conn_->remote); doneConnecting(COMM_OK, 0); break; default: - debugs(5, 5, HERE << conn_ << ": * - try again"); failRetries_++; - if (host_ != NULL) - ipcacheMarkBadAddr(host_, conn_->remote); -#if USE_ICMP - if (Config.onoff.test_reachability) - netdbDeleteAddrNetwork(conn_->remote); -#endif // check for timeout FIRST. if(squid_curtime - connectStart_ > connectTimeout_) { - debugs(5, 5, HERE << conn_ << ": * - ERR took too long already."); - if (calls_.earlyAbort_ != NULL) { - calls_.earlyAbort_->cancel("Comm::ConnOpener::connect timed out"); - calls_.earlyAbort_ = NULL; - } + debugs(5, 5, HERE << conn_ << ": * - ERR took too long to receive response."); + calls_.earlyAbort_->cancel("Comm::ConnOpener::connect timed out"); + calls_.earlyAbort_ = NULL; conn_->close(); doneConnecting(COMM_TIMEOUT, errno); } else if (failRetries_ < Config.connect_retries) { + debugs(5, 5, HERE << conn_ << ": * - try again"); eventAdd("Comm::ConnOpener::DelayedConnectRetry", Comm::ConnOpener::DelayedConnectRetry, this, 0.05, 0); + return; } else { // send ERROR back to the upper layer. debugs(5, 5, HERE << conn_ << ": * - ERR tried too many times already."); - if (calls_.earlyAbort_ != NULL) { - calls_.earlyAbort_->cancel("Comm::ConnOpener::connect failed"); - calls_.earlyAbort_ = NULL; - } + calls_.earlyAbort_->cancel("Comm::ConnOpener::connect failed"); + calls_.earlyAbort_ = NULL; conn_->close(); doneConnecting(COMM_ERR_CONNECT, errno); } diff --git a/src/pconn.cc b/src/pconn.cc index e14865e6fe..4933495b5a 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -157,18 +157,29 @@ IdleConnList::findUseable(const Comm::ConnectionPointer &key) { assert(size_); + // small optimization: do the constant bool tests only once. + const bool keyCheckAddr = !key->local.IsAnyAddr(); + const bool keyCheckPort = key->local.GetPort() > 0; + for (int i=size_-1; i>=0; i--) { - // callback pending indicates that remote end of the conn has just closed. - if (comm_has_pending_read_callback(theList_[i]->fd)) + // Is the FD pending completion of the closure callback? + // this flag is set while our early-read/close handler is + // waiting for a remote response. It gets unset when the + // handler is scheduled. + if (!fd_table[theList_[i]->fd].flags.read_pending) + continue; + + // connection already closed. useless. + if (!Comm::IsConnOpen(theList_[i])) continue; // local end port is required, but dont match. - if (key->local.GetPort() > 0 && key->local.GetPort() != theList_[i]->local.GetPort()) + if (keyCheckPort && key->local.GetPort() != theList_[i]->local.GetPort()) continue; // local address is required, but does not match. - if (!key->local.IsAnyAddr() && key->local.matchIPAddr(theList_[i]->local) != 0) + if (keyCheckAddr && key->local.matchIPAddr(theList_[i]->local) != 0) continue; // finally, a match. pop and return it. @@ -196,8 +207,11 @@ IdleConnList::Read(const Comm::ConnectionPointer &conn, char *buf, size_t len, c if (index >= 0) { /* might delete list */ list->removeAt(index); - conn->close(); } + // else we lost a race. + // Somebody started using the pconn since the remote end disconnected. + // pass the closure info on! + conn->close(); } void @@ -330,7 +344,7 @@ PconnPool::pop(const Comm::ConnectionPointer &destLink, const char *domain, bool /* may delete list */ Comm::ConnectionPointer temp = list->findUseable(destLink); - if (Comm::IsConnOpen(temp) && !isRetriable) + if (!isRetriable && Comm::IsConnOpen(temp)) temp->close(); return temp; -- 2.47.2