From: Amos Jeffries Date: Sun, 8 May 2011 06:11:18 +0000 (+1200) Subject: Cleanup: Improve Connection Pinning management X-Git-Tag: take07~16^2~27 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=b1cf235052c14875af6e380532683431fcb097e2;p=thirdparty%2Fsquid.git Cleanup: Improve Connection Pinning management Since 1xx handing went in HttpRequest has had two links to the one ConnStateData managing its client connection. * Rename the 1xx link to clientConnectionManager (since it is not actually the connection, but the manager object controlling the FD usage and stats. * Convert the pinning code to using the permanent clientConnectionManager link instead of a temporary pinned_connection link. This moves all connection pinning state fully into the ConnStateData manager objects scope. Side changes that appear to be buggy code previously: * do not alter pinning state at the point where the pinned connection is about to start being used. Changes are only relevant at the point of pinning or unpinning. * unpin operation now closes the Server FD if still open. Previously there was the possibility that some code paths would leave server FD open and pconn it. (especially since the above mentioned state alteration cleared the "pinned" flag). --- diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 0919540b2f..5cfe284de2 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -87,7 +87,6 @@ HttpRequest::init() #if USE_AUTH auth_user_request = NULL; #endif - pinned_connection = NULL; port = 0; canonical = NULL; memset(&flags, '\0', sizeof(flags)); @@ -157,9 +156,6 @@ HttpRequest::clean() range = NULL; } - if (pinned_connection) - cbdataReferenceDone(pinned_connection); - myportname.clean(); tag.clean(); @@ -642,9 +638,7 @@ bool HttpRequest::inheritProperties(const HttpMsg *aMsg) #if USE_AUTH auth_user_request = aReq->auth_user_request; #endif - if (aReq->pinned_connection) { - pinned_connection = cbdataReference(aReq->pinned_connection); - } + clientConnectionManager = aReq->clientConnectionManager; return true; } diff --git a/src/HttpRequest.h b/src/HttpRequest.h index 898c329a12..dc96ff9702 100644 --- a/src/HttpRequest.h +++ b/src/HttpRequest.h @@ -145,12 +145,6 @@ private: char host[SQUIDHOSTNAMELEN]; int host_is_numeric; - /*** - * The client side connection data of pinned connections for the client side - * request related objects - */ - ConnStateData *pinned_connection; - #if USE_ADAPTATION mutable Adaptation::History::Pointer adaptHistory_; ///< per-HTTP transaction info #endif @@ -248,20 +242,18 @@ public: static HttpRequest * CreateFromUrl(char * url); - void setPinnedConnection(ConnStateData *conn) { - pinned_connection = cbdataReference(conn); - } - ConnStateData *pinnedConnection() { - return pinned_connection; + if (clientConnectionManager.valid() && clientConnectionManager->pinning.pinned) + return clientConnectionManager.get(); + return NULL; } - void releasePinnedConnection() { - cbdataReferenceDone(pinned_connection); - } - - /// client-side conn manager, if known; used for 1xx response forwarding - CbcPointer clientConnection; + /** + * The client connection manager, if known; + * Used for any response actions needed directly to the client. + * ie 1xx forwarding or connection pinning state changes + */ + CbcPointer clientConnectionManager; int64_t getRangeOffsetLimit(); /* the result of this function gets cached in rangeOffsetLimit */ diff --git a/src/client_side.cc b/src/client_side.cc index 6b79f81e9f..cb1a3fc623 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -4035,7 +4035,8 @@ ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) * connection has gone away */ } -void ConnStateData::pinConnection(int pinning_fd, HttpRequest *request, struct peer *aPeer, bool auth) +void +ConnStateData::pinConnection(int pinning_fd, HttpRequest *request, struct peer *aPeer, bool auth) { fde *f; char desc[FD_DESC_SZ]; @@ -4069,7 +4070,8 @@ void ConnStateData::pinConnection(int pinning_fd, HttpRequest *request, struct p } -int ConnStateData::validatePinnedConnection(HttpRequest *request, const struct peer *aPeer) +int +ConnStateData::validatePinnedConnection(HttpRequest *request, const struct peer *aPeer) { bool valid = true; if (pinning.fd < 0) @@ -4089,21 +4091,15 @@ int ConnStateData::validatePinnedConnection(HttpRequest *request, const struct p } if (!valid) { - int pinning_fd=pinning.fd; - /* The pinning info is not safe, remove any pinning info*/ + /* The pinning info is not safe, remove any pinning info */ unpinConnection(); - - /* also close the server side socket, we should not use it for invalid/unauthenticated - requests... - */ - comm_close(pinning_fd); - return -1; } return pinning.fd; } -void ConnStateData::unpinConnection() +void +ConnStateData::unpinConnection() { if (pinning.peer) cbdataReferenceDone(pinning.peer); @@ -4112,6 +4108,8 @@ void ConnStateData::unpinConnection() comm_remove_close_handler(pinning.fd, pinning.closeHandler); pinning.closeHandler = NULL; } + /// also close the server side socket, we should not use it for any future requests... + comm_close(pinning.fd); pinning.fd = -1; safe_free(pinning.host); } diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 6b0fa67ace..6ab8f70aa9 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -269,7 +269,7 @@ clientReplyContext::processExpired() http->storeEntry(entry); assert(http->out.offset == 0); - http->request->clientConnection = http->getConn(); + http->request->clientConnectionManager = http->getConn(); /* * A refcounted pointer so that FwdState stays around as long as @@ -660,7 +660,7 @@ clientReplyContext::processMiss() if (http->flags.internal) r->protocol = AnyP::PROTO_INTERNAL; - r->clientConnection = http->getConn(); + r->clientConnectionManager = http->getConn(); /** Start forwarding to get the new object from network */ FwdState::fwdStart(http->getConn() != NULL ? http->getConn()->fd : -1, diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 8c55b1f7a6..86b1c22bf4 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -798,7 +798,8 @@ clientCheckPinning(ClientHttpRequest * http) } else { request->flags.connection_proxy_auth = 1; } - request->setPinnedConnection(http_conn); + // These should already be linked correctly. + assert(request->clientConnectionManager == http_conn); } } @@ -831,7 +832,8 @@ clientCheckPinning(ClientHttpRequest * http) } } if (may_pin && !request->pinnedConnection()) { - request->setPinnedConnection(http->getConn()); + // These should already be linked correctly. Just need the ServerConnection to pinn. + assert(request->clientConnectionManager == http_conn); } } } diff --git a/src/forward.cc b/src/forward.cc index 39828bded3..9523abb694 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -842,7 +842,6 @@ FwdState::connectStart() assert(pinned_connection); fd = pinned_connection->validatePinnedConnection(request, fs->_peer); if (fd >= 0) { - pinned_connection->unpinConnection(); #if 0 if (!fs->_peer) fs->code = HIER_DIRECT; @@ -858,8 +857,7 @@ FwdState::connectStart() return; } /* Failure. Fall back on next path */ - debugs(17,2,HERE << " Pinned connection " << pinned_connection << " not valid. Releasing."); - request->releasePinnedConnection(); + debugs(17,2,HERE << " Pinned connection " << pinned_connection << " not valid."); servers = fs->next; fwdServerFree(fs); connectStart(); diff --git a/src/http.cc b/src/http.cc index 148d17c59e..17bbbc3003 100644 --- a/src/http.cc +++ b/src/http.cc @@ -786,7 +786,7 @@ HttpStateData::handle1xx(HttpReply *reply) typedef NullaryMemFunT CbDialer; const AsyncCall::Pointer cb = JobCallback(11, 3, CbDialer, this, HttpStateData::proceedAfter1xx); - CallJobHere1(11, 4, orig_request->clientConnection, ConnStateData, + CallJobHere1(11, 4, orig_request->clientConnectionManager, ConnStateData, ConnStateData::sendControlMsg, HttpControlMsg(msg, cb)); // If the call is not fired, then the Sink is gone, and HttpStateData // will terminate due to an aborted store entry or another similar error.