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 4d78877906a19c513e7b6064170282cf629ed15a..33348fb9e66c3b7d492d74e545bda5c09bc01327 100644 (file)
--- 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<ConnStateData, CommCloseCbParams> 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 93f2c7f9209ae52f6cc334f6a906d0fd7642980b..655c684492f0fd37e23bb25d02ddd27670285556 100644 (file)
--- 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 067f6a8f04894f583e4b3a1a91b966cfd4791fc6..9c7629276de406cafaee73f45253c1d525d8273d 100644 (file)
--- 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 ddf96ae5351fbd485bf4163b5021b7e1fa1f7617..ef1d0c3aa5a2c14b0a292cdb8e453a042f6d7783 100644 (file)
--- 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 7d109919dcf454130df0b0ac79d3a5ead1d8232d..7ed9de2d5e3513a7f7309b1de81c43ee2d4fbf5c 100644 (file)
--- 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 2f32f735691c992fadb8945c93fca281eb64f683..f7c251f964f2c2bff96e9eabaa491e0822ecbf07 100644 (file)
--- 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 6c2eb0af892c9c9e7c1bd4859a9dae10f8b8b660..916ab5aaf389d527e2578a0bec158f1470fc5ce8 100644 (file)
--- 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 3de71c1ec32b055e34b8e8b1485e6dfd9c31e5ef..d153802b2d942b15ee9c4da832625ad241960097 100644 (file)
--- 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 27401de98f45108c9a6727a0927d981ec0129832..cc106f60d52d7f9c6895602c486f26e9746708a7 100644 (file)
--- 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 */