From: Amos Jeffries Date: Fri, 3 Dec 2010 13:40:18 +0000 (+1300) Subject: Reduce implicit Comm::Connection closures X-Git-Tag: take08~55^2~124^2~28 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e3a4aeccfab5a5818896f43369c03e980d000b79;p=thirdparty%2Fsquid.git Reduce implicit Comm::Connection closures * permits the ConnOpener parent Jobs to abort the opening by canceling their callbacks early. * makes FTP, forwarding and ConnStateData components cancel pending comm opens from their destructors (swanSong in ConnStateData). This seems to reduce the cycles consumed by ConnOpener and also the Async call delays somewhat in the cases where client connections are aborted. --- diff --git a/src/client_side.cc b/src/client_side.cc index 4d78877906..33348fb9e6 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -757,7 +757,6 @@ ConnStateData::swanSong() debugs(33, 2, HERE << clientConn); flags.readMoreRequests = false; clientdbEstablished(clientConn->remote, -1); /* decrement */ - clientConn = NULL; assert(areAllContextsForThisConnection()); freeAllContexts(); @@ -766,8 +765,13 @@ ConnStateData::swanSong() auth_user_request->onConnectionClose(this); } - if (pinning.fd >= 0) - comm_close(pinning.fd); + if (Comm::IsConnOpen(pinning.serverConn)) + pinning.serverConn->close(); + pinning.serverConn = NULL; + + if (Comm::IsConnOpen(clientConn)) + clientConn->close(); + clientConn = NULL; BodyProducer::swanSong(); flags.swanSang = true; @@ -1522,7 +1526,7 @@ ClientSocketContext::keepaliveNextRequest() debugs(33, 3, HERE << conn->clientConn); connIsFinished(); - if (conn->pinning.pinned && conn->pinning.fd == -1) { + if (conn->pinning.pinned && !Comm::IsConnOpen(conn->pinning.serverConn)) { debugs(33, 2, HERE << conn->clientConn << " Connection was pinned but server side gone. Terminating client connection"); conn->clientConn->close(); return; @@ -3818,7 +3822,6 @@ ConnStateData::ConnStateData() : closing_(false), switchedToHttps_(false) { - pinning.fd = -1; pinning.pinned = false; pinning.auth = false; } @@ -3980,54 +3983,47 @@ ConnStateData::sendControlMsg(HttpControlMsg msg) void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) { - pinning.fd = -1; - if (pinning.peer) { - cbdataReferenceDone(pinning.peer); - } - safe_free(pinning.host); - /* NOTE: pinning.pinned should be kept. This combined with fd == -1 at the end of a request indicates that the host - * connection has gone away */ + unpinConnection(); } -void ConnStateData::pinConnection(int pinning_fd, HttpRequest *request, struct peer *aPeer, bool auth) +void +ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, struct peer *aPeer, bool auth) { char desc[FD_DESC_SZ]; - if (pinning.fd == pinning_fd) - return; - else if (pinning.fd != -1) - comm_close(pinning.fd); + if (Comm::IsConnOpen(pinning.serverConn)) { + if (pinning.serverConn->fd == pinServerConn->fd) + return; - if (pinning.host) - safe_free(pinning.host); + unpinConnection(); // clears fields ready for re-use. Prevent close() scheduling our close handler. + pinning.serverConn->close(); + } else + unpinConnection(); // clears fields ready for re-use. - pinning.fd = pinning_fd; + pinning.serverConn = pinServerConn; pinning.host = xstrdup(request->GetHost()); pinning.port = request->port; pinning.pinned = true; - if (pinning.peer) - cbdataReferenceDone(pinning.peer); if (aPeer) pinning.peer = cbdataReference(aPeer); pinning.auth = auth; - snprintf(desc, FD_DESC_SZ, "%s pinned connection for %s:%d (%d)", - (auth || !aPeer) ? request->GetHost() : aPeer->name, fd_table[clientConn->fd].ipaddr, - clientConn->remote.GetPort(), clientConn->fd); - fd_note(pinning_fd, desc); + char stmp[MAX_IPSTRLEN]; + snprintf(desc, FD_DESC_SZ, "%s pinned connection for %s (%d)", + (auth || !aPeer) ? request->GetHost() : aPeer->name, clientConn->remote.ToURL(stmp,MAX_IPSTRLEN), clientConn->fd); + fd_note(pinning.serverConn->fd, desc); typedef CommCbMemFunT Dialer; pinning.closeHandler = JobCallback(33, 5, Dialer, this, ConnStateData::clientPinnedConnectionClosed); - comm_add_close_handler(pinning_fd, pinning.closeHandler); - + comm_add_close_handler(pinning.serverConn->fd, pinning.closeHandler); } -int ConnStateData::validatePinnedConnection(HttpRequest *request, const struct peer *aPeer) +const Comm::ConnectionPointer +ConnStateData::validatePinnedConnection(HttpRequest *request, const struct peer *aPeer) { bool valid = true; - if (pinning.fd < 0) - return -1; - + if (!Comm::IsConnOpen(pinning.serverConn)) + valid = false; if (pinning.auth && request && strcasecmp(pinning.host, request->GetHost()) != 0) { valid = false; } @@ -4042,29 +4038,32 @@ 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*/ unpinConnection(); /* also close the server side socket, we should not use it for invalid/unauthenticated requests... */ - comm_close(pinning_fd); - return -1; + if (Comm::IsConnOpen(pinning.serverConn)) + pinning.serverConn->close(); } - return pinning.fd; + return pinning.serverConn; } -void ConnStateData::unpinConnection() +void +ConnStateData::unpinConnection() { if (pinning.peer) cbdataReferenceDone(pinning.peer); if (pinning.closeHandler != NULL) { - comm_remove_close_handler(pinning.fd, pinning.closeHandler); + comm_remove_close_handler(pinning.serverConn->fd, pinning.closeHandler); pinning.closeHandler = NULL; } - pinning.fd = -1; + safe_free(pinning.host); + + /* NOTE: pinning.pinned should be kept. This combined with fd == -1 at the end of a request indicates that the host + * connection has gone away */ } diff --git a/src/client_side.h b/src/client_side.h index 93f2c7f920..655c684492 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -249,7 +249,7 @@ public: bool swanSang; // XXX: temporary flag to check proper cleanup } flags; struct { - int fd; /* pinned server side connection */ + Comm::ConnectionPointer serverConn; /* pinned server side connection */ char *host; /* host name of pinned connection */ int port; /* port of pinned connection */ bool pinned; /* this connection was pinned */ @@ -278,7 +278,7 @@ public: /** * Correlate the current ConnStateData object with the pinning_fd socket descriptor. */ - void pinConnection(int fd, HttpRequest *request, struct peer *peer, bool auth); + void pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, struct peer *peer, bool auth); /** * Decorrelate the ConnStateData object from its pinned peer */ @@ -288,9 +288,9 @@ public: * if pinned info is not valid. \param request if it is not NULL also checks if the pinning info refers to the request client side HttpRequest \param peer if it is not NULL also check if the peer is the pinning peer - \return The fd of the server side connection or -1 if fails. + \return The details of the server side connection (may be closed if failures were present). */ - int validatePinnedConnection(HttpRequest *request, const struct peer *peer); + const Comm::ConnectionPointer validatePinnedConnection(HttpRequest *request, const struct peer *peer); /** * returts the pinned peer if exists, NULL otherwise */ diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 067f6a8f04..9c7629276d 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -780,7 +780,7 @@ clientCheckPinning(ClientHttpRequest * http) request->flags.connection_auth_disabled = http_conn->port->connection_auth_disabled; if (!request->flags.connection_auth_disabled) { - if (http_conn->pinning.fd != -1) { + if (Comm::IsConnOpen(http_conn->pinning.serverConn)) { if (http_conn->pinning.auth) { request->flags.connection_auth = 1; request->flags.auth = 1; diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc index ddf96ae535..ef1d0c3aa5 100644 --- a/src/comm/ConnOpener.cc +++ b/src/comm/ConnOpener.cc @@ -35,16 +35,16 @@ bool Comm::ConnOpener::doneAll() const { // is the conn_ to be opened still waiting? - if (conn_ != NULL) { - return false; + if (conn_ == NULL) { + return AsyncJob::doneAll(); } // is the callback still to be called? - if (callback_ != NULL) { - return false; + if (callback_ == NULL || callback_->canceled()) { + return AsyncJob::doneAll(); } - return AsyncJob::doneAll(); + return false; } void @@ -71,8 +71,11 @@ Comm::ConnOpener::swanSong() } if (callback_ != NULL) { - // inform the still-waiting caller we are dying - doneConnecting(COMM_ERR_CONNECT, 0); + if (callback_->canceled()) + callback_ = NULL; + else + // inform the still-waiting caller we are dying + doneConnecting(COMM_ERR_CONNECT, 0); } AsyncJob::swanSong(); diff --git a/src/forward.cc b/src/forward.cc index 7d109919dc..7ed9de2d5e 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -175,6 +175,11 @@ FwdState::~FwdState() entry = NULL; + if (calls.connector != NULL) { + calls.connector->cancel("FwdState destructed"); + calls.connector = NULL; + } + if (Comm::IsConnOpen(serverConn)) { comm_remove_close_handler(serverConnection()->fd, fwdServerClosedWrapper, this); debugs(17, 3, HERE << "closing FD " << serverConnection()->fd); @@ -789,13 +794,12 @@ FwdState::connectStart() if (serverDestinations[0]->peerType == PINNED) { ConnStateData *pinned_connection = request->pinnedConnection(); assert(pinned_connection); - serverDestinations[0]->fd = pinned_connection->validatePinnedConnection(request, serverDestinations[0]->getPeer()); - if (Comm::IsConnOpen(serverDestinations[0])) { - serverConn = serverDestinations[0]; - pinned_connection->unpinConnection(); + serverConn = pinned_connection->validatePinnedConnection(request, serverDestinations[0]->getPeer()); + if (Comm::IsConnOpen(serverConn)) { + pinned_connection->unpinConnection(); // XXX: this should be just remove the pinning close handler ?? #if 0 - if (!serverDestinations[0]->getPeer()) - serverDestinations[0]->peerType = HIER_DIRECT; + if (!serverConn->getPeer()) + serverConn->peerType = HIER_DIRECT; #endif n_tries++; request->flags.pinned = 1; @@ -806,7 +810,7 @@ FwdState::connectStart() return; } /* Failure. Fall back on next path */ - debugs(17,2,HERE << " Pinned connection " << pinned_connection << " not valid. Releasing."); + debugs(17, 2, HERE << " Pinned connection " << pinned_connection << " not valid. Releasing."); request->releasePinnedConnection(); serverDestinations.shift(); startConnectionOrFail(); @@ -871,8 +875,8 @@ FwdState::connectStart() debugs(17, 3, "fwdConnectStart: got outgoing addr " << serverDestinations[0]->local << ", tos " << int(serverDestinations[0]->tos)); #endif - AsyncCall::Pointer call = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this)); - Comm::ConnOpener *cs = new Comm::ConnOpener(serverDestinations[0], call, ctimeout); + calls.connector = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this)); + Comm::ConnOpener *cs = new Comm::ConnOpener(serverDestinations[0], calls.connector, ctimeout); cs->setHost(host); AsyncJob::Start(cs); } diff --git a/src/forward.h b/src/forward.h index 2f32f73569..f7c251f964 100644 --- a/src/forward.h +++ b/src/forward.h @@ -95,6 +95,10 @@ private: http_status last_status; #endif + // AsyncCalls which we set and may need cancelling. + struct { + AsyncCall::Pointer connector; ///< a call linking us to the ConnOpener producing serverConn. + } calls; struct { unsigned int dont_retry:1; diff --git a/src/ftp.cc b/src/ftp.cc index 6c2eb0af89..916ab5aaf3 100644 --- a/src/ftp.cc +++ b/src/ftp.cc @@ -164,8 +164,9 @@ public: */ Comm::ConnectionPointer listenConn; + AsyncCall::Pointer opener; ///< Comm opener handler callback. private: - AsyncCall::Pointer closer; /// Comm close handler callback + AsyncCall::Pointer closer; ///< Comm close handler callback }; /// \ingroup ServerProtocolFTPInternal @@ -293,6 +294,7 @@ public: virtual bool doneWithServer() const; virtual bool haveControlChannel(const char *caller_name) const; AsyncCall::Pointer dataCloser(); /// creates a Comm close callback + AsyncCall::Pointer dataOpener(); /// creates a Comm connect callback private: // BodyConsumer for HTTP: consume request body. @@ -506,11 +508,15 @@ FtpStateData::~FtpStateData() reply_hdr = NULL; } + if (data.opener != NULL) { + data.opener->cancel("FtpStateData destructed"); + data.opener = NULL; + } data.close(); if (Comm::IsConnOpen(ctrl.conn)) { debugs(9, DBG_IMPORTANT, HERE << "Internal bug: FtpStateData left " << - "control FD " << ctrl.conn->fd << " open"); + "open control channel " << ctrl.conn); } if (ctrl.buf) { @@ -2442,8 +2448,8 @@ ftpReadEPSV(FtpStateData* ftpState) debugs(9, 3, HERE << "connecting to " << conn->remote); - AsyncCall::Pointer call = commCbCall(9,3, "FtpStateData::ftpPasvCallback", CommConnectCbPtrFun(FtpStateData::ftpPasvCallback, ftpState)); - Comm::ConnOpener *cs = new Comm::ConnOpener(conn, call, Config.Timeout.connect); + ftpState->data.opener = commCbCall(9,3, "FtpStateData::ftpPasvCallback", CommConnectCbPtrFun(FtpStateData::ftpPasvCallback, ftpState)); + Comm::ConnOpener *cs = new Comm::ConnOpener(conn, ftpState->data.opener, Config.Timeout.connect); cs->setHost(ftpState->data.host); AsyncJob::Start(cs); } @@ -2689,8 +2695,8 @@ ftpReadPasv(FtpStateData * ftpState) debugs(9, 3, HERE << "connecting to " << conn->remote); - AsyncCall::Pointer call = commCbCall(9,3, "FtpStateData::ftpPasvCallback", CommConnectCbPtrFun(FtpStateData::ftpPasvCallback, ftpState)); - Comm::ConnOpener *cs = new Comm::ConnOpener(conn, call, Config.Timeout.connect); + ftpState->data.opener = commCbCall(9,3, "FtpStateData::ftpPasvCallback", CommConnectCbPtrFun(FtpStateData::ftpPasvCallback, ftpState)); + Comm::ConnOpener *cs = new Comm::ConnOpener(conn, ftpState->data.opener, Config.Timeout.connect); cs->setHost(ftpState->data.host); AsyncJob::Start(cs); } @@ -2700,6 +2706,7 @@ FtpStateData::ftpPasvCallback(const Comm::ConnectionPointer &conn, comm_err_t st { FtpStateData *ftpState = (FtpStateData *)data; debugs(9, 3, HERE); + ftpState->data.opener = NULL; if (status != COMM_OK) { debugs(9, 2, HERE << "Failed to connect. Retrying via another method."); diff --git a/src/http.cc b/src/http.cc index 3de71c1ec3..d153802b2d 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1433,7 +1433,7 @@ HttpStateData::processReplyBody() } if (orig_request->pinnedConnection() && ispinned) { - orig_request->pinnedConnection()->pinConnection(serverConnection->fd, orig_request, _peer, + orig_request->pinnedConnection()->pinConnection(serverConnection, orig_request, _peer, (request->flags.connection_auth != 0)); } else { fwd->pconnPush(serverConnection, request->GetHost()); diff --git a/src/peer_select.cc b/src/peer_select.cc index 27401de98f..cc106f60d5 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -429,16 +429,15 @@ static void peerSelectPinned(ps_state * ps) { HttpRequest *request = ps->request; - peer *peer; if (!request->pinnedConnection()) return; - peer = request->pinnedConnection()->pinnedPeer(); - if (request->pinnedConnection()->validatePinnedConnection(request, peer) != -1) { - if (peer && peerAllowedToUse(peer, request)) { - peerAddFwdServer(&ps->servers, peer, PINNED); + peer *pear = request->pinnedConnection()->pinnedPeer(); + if (Comm::IsConnOpen(request->pinnedConnection()->validatePinnedConnection(request, pear))) { + if (pear && peerAllowedToUse(pear, request)) { + peerAddFwdServer(&ps->servers, pear, PINNED); if (ps->entry) ps->entry->ping_status = PING_DONE; /* Skip ICP */ - } else if (!peer && ps->direct != DIRECT_NO) { + } else if (!pear && ps->direct != DIRECT_NO) { peerAddFwdServer(&ps->servers, NULL, PINNED); if (ps->entry) ps->entry->ping_status = PING_DONE; /* Skip ICP */