From 73bd47c1f53f11bc7fbc7fddf11aab6ac2c8f5b3 Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Fri, 24 Feb 2012 21:32:57 -0700
Subject: [PATCH] Retry requests that failed due to a persistent connection
 race

... instead of replying with ERR_ZERO_SIZE_OBJECT "Bad Gateway".

The ERR_ZERO_SIZE_OBJECT errors were visible to the client when the
destination had only one address because serverDestinations.shift()
made the list of destination empty and startConnectionOrFail() failed.

When FwdState starts to use a pinned connection, the connection is treated as
an idle persistent connection as far as race detection is concerned.
Currently, pinned connections cannot be reopened, repinned, and retried after
a pconn race. This will change when server-side bumped connections become
pinned.

It felt wrong that a failed serverConn may remain set while we are opening a
new connection so I set it to NULL after a squid-dev discussion indicating
that doing so should be safe.

We also now reset the local port number to zero in case it was set to the
actual source port by ConnOpener or other code working with the previous
connection to the same serverDestinations[0] address, although simple tests
worked (and showed changing source port) without this reset.
---
 src/forward.cc | 40 +++++++++++++++++++++++++++++++++++-----
 src/forward.h  |  4 ++++
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/src/forward.cc b/src/forward.cc
index 50b4ded9ef..e78081b5fa 100644
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -97,6 +97,7 @@ FwdState::FwdState(const Comm::ConnectionPointer &client, StoreEntry * e, HttpRe
     entry = e;
     clientConn = client;
     request = HTTPMSGLOCK(r);
+    pconnRace = raceImpossible;
     start_t = squid_curtime;
     serverDestinations.reserve(Config.forward_max_tries);
     e->lock();
@@ -325,6 +326,11 @@ FwdState::fail(ErrorState * errorState)
     if (!errorState->request)
         errorState->request = HTTPMSGLOCK(request);
 
+    if (pconnRace == racePossible && err->type == ERR_ZERO_SIZE_OBJECT) {
+        debugs(17, 5, HERE << "pconn race happened");
+        pconnRace = raceHappened;
+    }
+
 #if USE_SSL
     if (errorState->type == ERR_SECURE_CONNECT_FAIL && errorState->detail)
         request->detailError(errorState->type, errorState->detail->errorNo());
@@ -535,7 +541,11 @@ FwdState::retryOrBail()
 {
     if (checkRetry()) {
         debugs(17, 3, HERE << "re-forwarding (" << n_tries << " tries, " << (squid_curtime - start_t) << " secs)");
-        serverDestinations.shift(); // last one failed. try another.
+        // we should retry the same destination if it failed due to pconn race
+        if (pconnRace == raceHappened)
+            debugs(17, 4, HERE << "retrying the same destination");
+        else
+            serverDestinations.shift(); // last one failed. try another.
         startConnectionOrFail();
         return;
     }
@@ -827,8 +837,11 @@ FwdState::connectStart()
     // XXX: also, logs will now lie if pinning is broken and leads to an error message.
     if (serverDestinations[0]->peerType == PINNED) {
         ConnStateData *pinned_connection = request->pinnedConnection();
-        assert(pinned_connection);
-        serverConn = pinned_connection->validatePinnedConnection(request, serverDestinations[0]->getPeer());
+        // pinned_connection may become nil after a pconn race
+        if (pinned_connection)
+            serverConn = pinned_connection->validatePinnedConnection(request, serverDestinations[0]->getPeer());
+        else
+            serverConn = NULL;
         if (Comm::IsConnOpen(serverConn)) {
 #if 0
             if (!serverConn->getPeer())
@@ -838,6 +851,8 @@ FwdState::connectStart()
             request->flags.pinned = 1;
             if (pinned_connection->pinnedAuth())
                 request->flags.auth = 1;
+            // the server may close the pinned connection before this request
+            pconnRace = racePossible;
             dispatch();
             return;
         }
@@ -855,10 +870,19 @@ FwdState::connectStart()
     } else {
         host = request->GetHost();
     }
-    Comm::ConnectionPointer temp = fwdPconnPool->pop(serverDestinations[0], host, checkRetriable());
+
+    Comm::ConnectionPointer temp;
+    // Avoid pconns after races so that the same client does not suffer twice.
+    // This does not increase the total number of connections because we just
+    // closed the connection that failed the race. And re-pinning assumes this.
+    if (pconnRace != raceHappened)
+        temp = fwdPconnPool->pop(serverDestinations[0], host, checkRetriable());
+
+    const bool openedPconn = Comm::IsConnOpen(temp);
+    pconnRace = openedPconn ? racePossible : raceImpossible;
 
     // if we found an open persistent connection to use. use it.
-    if (Comm::IsConnOpen(temp)) {
+    if (openedPconn) {
         serverConn = temp;
         debugs(17, 3, HERE << "reusing pconn " << serverConnection());
         n_tries++;
@@ -884,6 +908,12 @@ FwdState::connectStart()
         return;
     }
 
+    // We will try to open a new connection, possibly to the same destination.
+    // We reset serverDestinations[0] in case we are using it again because
+    // ConnOpener modifies its destination argument.
+    serverDestinations[0]->local.SetPort(0);
+    serverConn = NULL;
+
 #if URL_CHECKSUM_DEBUG
     entry->mem_obj->checkUrlChecksum();
 #endif
diff --git a/src/forward.h b/src/forward.h
index 01cd931f21..b7d40e2d30 100644
--- a/src/forward.h
+++ b/src/forward.h
@@ -105,6 +105,10 @@ private:
 
     Comm::ConnectionPointer serverConn; ///< a successfully opened connection to a server.
 
+    /// possible pconn race states
+    typedef enum { raceImpossible, racePossible, raceHappened } PconnRace;
+    PconnRace pconnRace; ///< current pconn race state
+
     // NP: keep this last. It plays with private/public
     CBDATA_CLASS2(FwdState);
 };
-- 
2.47.2