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.

diff --git a/src/forward.cc b/src/forward.cc
index 50b4ded9efe3347f0cfcdd2dcc4f8f91c7ad1221..e78081b5fa336248bfea0fa663357481d07c37cd 100644 (file)
--- 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 01cd931f21e56abffae8b041f5bf6de0ac7de455..b7d40e2d30e9185a6eb2a00e5826d50135e6f079 100644 (file)
--- 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);
 };