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 0919540b2f1bd7ca94de8bb89256318ae25ca4b0..5cfe284de2c96af1287f16cd369b0c8618986f7a 100644 (file)
--- 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 898c329a121c41fc12817bed525e8dc3b9b7742f..dc96ff9702bd066517575660f95ed7afdb9c5d8d 100644 (file)
--- 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<ConnStateData> 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<ConnStateData> 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 6b79f81e9f46b3af09287d8ef9d87001cbd38f67..cb1a3fc6238ed6928f11ea8f9505c42671449aff 100644 (file)
--- 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 6b0fa67ace9bb874449ef1dea472909715175f7b..6ab8f70aa92587f8cc3157cbaa457e61c7827317 100644 (file)
--- 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 8c55b1f7a6542c2122bb3f0ececdd3d655983e98..86b1c22bf4b40acec6feb6699118343ebb1bfb63 100644 (file)
--- 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 39828bded361c44b37cb52c1d9d589fb653d6e84..9523abb6943b217a2877a9c2fee9e5b9096df53f 100644 (file)
--- 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 148d17c59e7a7ca4ddfe5d8d1d58f4385b0bf4c8..17bbbc30037f902cb1c8d76abee0dd1310781b7f 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -786,7 +786,7 @@ HttpStateData::handle1xx(HttpReply *reply)
     typedef NullaryMemFunT<HttpStateData> 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.