Retry requests that failed due to a persistent connection race

instead of replying with ERR_ZERO_SIZE_OBJECT "Bad Gateway".

Trunk r12050 code contains a portion of these changes. The rest is code
to re-pin a bump-server-first connection if a bumped pinned pconn fails
due to a race. We use a new canRePin flag to mark connections that can
be repinned and assume that pinned connections unrelated to SslBump
cannot be repinned.

diff --git a/src/client_side.cc b/src/client_side.cc
index 20837434ce8899fdab890cdd0fee777fdcf0adb6..8bcdaba2db8210e2b2c5c4d07dc333fe0ee3bf3a 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2621,6 +2621,7 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
 
     request->flags.accelerated = http->flags.accel;
     request->flags.sslBumped = conn->switchedToHttps();
+    request->flags.canRePin = request->flags.sslBumped && conn->pinning.pinned;
     request->flags.ignore_cc = conn->port->ignore_cc;
     // TODO: decouple http->flags.accel from request->flags.sslBumped
     request->flags.no_direct = (request->flags.accelerated && !request->flags.sslBumped) ?
@@ -4334,8 +4335,12 @@ ConnStateData::sendControlMsg(HttpControlMsg msg)
 void
 ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io)
 {
-    pinning.closeHandler = NULL; // Comm unregisters handlers before calling
-    unpinConnection();
+    // it might be possible for FwdState to repin a failed connection sooner
+    // than this close callback is called for the failed connection
+    if (pinning.serverConnection == io.conn) {
+        pinning.closeHandler = NULL; // Comm unregisters handlers before calling
+        unpinConnection();
+    }
 }
 
 void
@@ -4352,6 +4357,8 @@ ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpReque
 
     pinning.serverConnection = pinServer;
 
+    debugs(33, 3, HERE << pinning.serverConnection);
+
     // when pinning an SSL bumped connection, the request may be NULL
     const char *pinnedHost = "[unknown]";
     if (request) {
@@ -4375,12 +4382,18 @@ ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpReque
     typedef CommCbMemFunT<ConnStateData, CommCloseCbParams> Dialer;
     pinning.closeHandler = JobCallback(33, 5,
                                        Dialer, this, ConnStateData::clientPinnedConnectionClosed);
+    // remember the pinned connection so that cb does not unpin a fresher one
+    typedef CommCloseCbParams Params;
+    Params &params = GetCommParams<Params>(pinning.closeHandler);
+    params.conn = pinning.serverConnection;
     comm_add_close_handler(pinning.serverConnection->fd, pinning.closeHandler);
 }
 
 const Comm::ConnectionPointer
 ConnStateData::validatePinnedConnection(HttpRequest *request, const struct peer *aPeer)
 {
+    debugs(33, 7, HERE << pinning.serverConnection);
+
     bool valid = true;
     if (!Comm::IsConnOpen(pinning.serverConnection))
         valid = false;
@@ -4408,6 +4421,8 @@ ConnStateData::validatePinnedConnection(HttpRequest *request, const struct peer
 void
 ConnStateData::unpinConnection()
 {
+    debugs(33, 3, HERE << pinning.serverConnection);
+
     if (pinning.peer)
         cbdataReferenceDone(pinning.peer);
 

diff --git a/src/forward.cc b/src/forward.cc
index 97c30ae5b006c04accdb1694db84a620247e33d1..b10bbc92ba98a98d6ab7972cf7c1b10ff166761a 100644 (file)
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -98,6 +98,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();
@@ -362,6 +363,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());
@@ -570,7 +576,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;
     }
@@ -822,6 +832,12 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, comm_err_t status, in
     if (serverConnection()->getPeer())
         peerConnectSucceded(serverConnection()->getPeer());
 
+    if (request->flags.canRePin && request->clientConnectionManager.valid()) {
+        debugs(17, 3, HERE << "repinning " << serverConn);
+        request->clientConnectionManager->pinConnection(serverConn,
+            request, serverConn->getPeer(), request->flags.auth);
+    }
+
 #if USE_SSL
     if ((serverConnection()->getPeer() && serverConnection()->getPeer()->use_ssl) ||
             (!serverConnection()->getPeer() && request->protocol == AnyP::PROTO_HTTPS)) {
@@ -901,8 +917,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())
@@ -913,14 +932,21 @@ FwdState::connectStart()
             if (pinned_connection->pinnedAuth())
                 request->flags.auth = 1;
             comm_add_close_handler(serverConn->fd, fwdServerClosedWrapper, this);
+            // the server may close the pinned connection before this request
+            pconnRace = racePossible;
             dispatch();
             return;
         }
-        /* Failure. Fall back on next path */
-        debugs(17,2,HERE << " Pinned connection " << pinned_connection << " not valid.");
-        serverDestinations.shift();
-        startConnectionOrFail();
-        return;
+        /* Failure. Fall back on next path unless we can re-pin */
+        debugs(17,2,HERE << "Pinned connection failed: " << pinned_connection);
+        if (pconnRace != raceHappened || !request->flags.canRePin) {
+            serverDestinations.shift();
+            pconnRace = raceImpossible;
+            startConnectionOrFail();
+            return;
+        }
+        debugs(17,3, HERE << "There was a pconn race. Will try to repin.");
+        // and fall through to regular handling
     }
 
     // Use pconn to avoid opening a new connection.
@@ -930,10 +956,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++;
@@ -959,6 +994,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 505eff5df844e51469c6d03dcfc8a4df3c91d342..691167a4fd6f6ffc83d8c3a494fe215233fc5da5 100644 (file)
--- a/src/forward.h
+++ b/src/forward.h
@@ -106,6 +106,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);
 };

diff --git a/src/structs.h b/src/structs.h
index 8ee346b4a3927cb1b6c70e65d6499080669119c3..be0b42dd4aae64c80081ea3ba1656c9207aaacd0 100644 (file)
--- a/src/structs.h
+++ b/src/structs.h
@@ -1009,7 +1009,7 @@ struct _iostats {
 
 
 struct request_flags {
-    request_flags(): range(0),nocache(0),ims(0),auth(0),cachable(0),hierarchical(0),loopdetect(0),proxy_keepalive(0),proxying(0),refresh(0),redirected(0),need_validation(0),fail_on_validation_err(0),stale_if_hit(0),accelerated(0),ignore_cc(0),intercepted(0),spoof_client_ip(0),internal(0),internalclient(0),must_keepalive(0),chunked_reply(0),stream_error(0),sslPeek(0),sslBumped(0),destinationIPLookedUp_(0) {
+    request_flags(): range(0),nocache(0),ims(0),auth(0),cachable(0),hierarchical(0),loopdetect(0),proxy_keepalive(0),proxying(0),refresh(0),redirected(0),need_validation(0),fail_on_validation_err(0),stale_if_hit(0),accelerated(0),ignore_cc(0),intercepted(0),spoof_client_ip(0),internal(0),internalclient(0),must_keepalive(0),pinned(0),canRePin(0),chunked_reply(0),stream_error(0),sslPeek(0),sslBumped(0),destinationIPLookedUp_(0) {
 #if USE_HTTP_VIOLATIONS
         nocache_hack = 0;
 #endif
@@ -1047,6 +1047,7 @@ unsigned int proxying:
     unsigned int connection_auth_disabled:1; /** Connection oriented auth can not be supported */
     unsigned int connection_proxy_auth:1; /** Request wants connection oriented auth */
     unsigned int pinned:1;      /* Request sent on a pinned connection */
+    unsigned int canRePin:1; ///< OK to reopen a failed pinned connection
     unsigned int auth_sent:1;   /* Authentication forwarded */
     unsigned int no_direct:1;  /* Deny direct forwarding unless overriden by always_direct. Used in accelerator mode */
     unsigned int chunked_reply:1; /**< Reply with chunked transfer encoding */