Some logic fixes and optimizations from run testing

diff --git a/src/comm.cc b/src/comm.cc
index 0738f080d5d1b86f7beb3445a61d640c9270c127..0aaab34e77cae0e2d57d6e915564bd8fa087c42f 100644 (file)
--- 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 c67f122dc04c5faea83ae2865d122c2ee626ac43..ddf96ae5351fbd485bf4163b5021b7e1fa1f7617 100644 (file)
--- 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 &params = GetCommParams<Params>(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 e14865e6fe10820fc72f557ebfd2c6d0e337a3f8..4933495b5a99cb4fe1224392749304c24c5c1f65 100644 (file)
--- 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;