From: Alex Rousskov Date: Mon, 20 Feb 2012 19:32:58 +0000 (-0700) Subject: Retry requests that failed due to a persistent connection race X-Git-Tag: BumpSslServerFirst.take05~21 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=85563fd92b5c72b42ba0b1e953f7bd238adce133;p=thirdparty%2Fsquid.git 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 20837434ce..8bcdaba2db 100644 --- 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 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 ¶ms = GetCommParams(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 97c30ae5b0..b10bbc92ba 100644 --- 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 505eff5df8..691167a4fd 100644 --- 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 8ee346b4a3..be0b42dd4a 100644 --- 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 */