Bug 4464: Reduce "!Comm::MonitorsRead(serverConnection->fd)" assertions.

* Protect Squid Client classes from new requests that compete with
  ongoing pinned connection use and
* resume dealing with new requests when those Client classes are done
  using the pinned connection.

Replaced primary ConnStateData::pinConnection() calls with a pair of
pinBusyConnection() and notePinnedConnectionBecameIdle() calls,
depending on the pinned connection state ("busy" or "idle").

Removed pinConnection() parameters that were not longer used or could be computed from the remaining parameters.

Removed ConnStateData::httpsPeeked() code "hiding" the originating
request and connection peer details while entering the first "idle"
state. The old (trunk r11880.1.6) bump-server-first code used a pair of
NULLs because "Intercepted connections do not have requests at the
connection pinning stage", but that limitation no longer applicable
because Squid always fakes (when intercepting) or parses (a CONNECT)
request now, even during SslBump step1.

The added XXX and TODOs are not directly related to this fix. They
were added to document problems discovered while working on this fix.

In v3.5 code, the same problems manifest as Read.cc
"fd_table[conn->fd].halfClosedReader != NULL" assertions.

This is a Measurement Factory project

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 7d363673b589001d9638177fabed1e6f1604782a..02f87797d9114be83b400389395904adaefc496c 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -246,7 +246,7 @@ FwdState::completed()
 #if USE_OPENSSL
             if (request->flags.sslPeek && request->clientConnectionManager.valid()) {
                 CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData,
-                             ConnStateData::httpsPeeked, Comm::ConnectionPointer(NULL));
+                             ConnStateData::httpsPeeked, ConnStateData::PinnedIdleContext(Comm::ConnectionPointer(nullptr), request));
             }
 #endif
         } else {
@@ -952,7 +952,7 @@ FwdState::dispatch()
 #if USE_OPENSSL
     if (request->flags.sslPeek) {
         CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData,
-                     ConnStateData::httpsPeeked, serverConnection());
+                     ConnStateData::httpsPeeked, ConnStateData::PinnedIdleContext(serverConnection(), request));
         unregister(serverConn); // async call owns it now
         complete(); // destroys us
         return;

diff --git a/src/base/RefCount.h b/src/base/RefCount.h
index 1e6d2d59fd7bb2d5e3ab554a191b4df996f559c7..282bc4ba7d6e5f0366f062c341cc9d1be87d380a 100644 (file)
--- a/src/base/RefCount.h
+++ b/src/base/RefCount.h
@@ -54,9 +54,7 @@ public:
 
     C & operator * () const {return *const_cast<C *>(p_); }
 
-    C const * getRaw() const {return p_; }
-
-    C * getRaw() {return const_cast<C *>(p_); }
+    C * getRaw() const { return const_cast<C *>(p_); }
 
     bool operator == (const RefCount& p) const {
         return p.p_ == p_;

diff --git a/src/client_side.cc b/src/client_side.cc
index 71bf2c98250ace00d9a9cc71dc0663c95b249ff5..58059368d124ef417f43e10402fd574dd53b4d7e 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -836,6 +836,7 @@ ConnStateData::swanSong()
     assert(areAllContextsForThisConnection());
     freeAllContexts();
 
+    // XXX: Closing pinned conn is too harsh: The Client may want to continue!
     unpinConnection(true);
 
     if (Comm::IsConnOpen(clientConnection))
@@ -1559,6 +1560,13 @@ ClientSocketContext::keepaliveNextRequest()
 
     debugs(33, 3, HERE << "ConnnStateData(" << conn->clientConnection << "), Context(" << clientConnection << ")");
     connIsFinished();
+    conn->kick();
+}
+
+void
+ConnStateData::kick()
+{
+    ConnStateData * conn = this; // XXX: Remove this diff minimization hack
 
     if (conn->pinning.pinned && !Comm::IsConnOpen(conn->pinning.serverConnection)) {
         debugs(33, 2, HERE << conn->clientConnection << " Connection was pinned but server side gone. Terminating client connection");
@@ -3240,6 +3248,13 @@ ConnStateData::clientParseRequests()
         if (in.buf.isEmpty())
             break;
 
+        // Prohibit concurrent requests when using a pinned to-server connection
+        // because our Client classes do not support request pipelining.
+        if (pinning.pinned && !pinning.readHandler) {
+            debugs(33, 3, clientConnection << " waits for busy " << pinning.serverConnection);
+            break;
+        }
+
         /* Limit the number of concurrent requests */
         if (concurrentRequestQueueFilled())
             break;
@@ -4434,22 +4449,19 @@ ConnStateData::doPeekAndSpliceStep()
 }
 
 void
-ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection)
+ConnStateData::httpsPeeked(PinnedIdleContext pic)
 {
     Must(sslServerBump != NULL);
+    Must(sslServerBump->request == pic.request);
+    Must(currentobject == NULL || currentobject->http == NULL || currentobject->http->request == pic.request.getRaw());
 
-    if (Comm::IsConnOpen(serverConnection)) {
-        pinConnection(serverConnection, NULL, NULL, false);
+    if (Comm::IsConnOpen(pic.connection)) {
+        notePinnedConnectionBecameIdle(pic);
 
         debugs(33, 5, HERE << "bumped HTTPS server: " << sslConnectHostOrIp);
-    } else {
+    } else
         debugs(33, 5, HERE << "Error while bumping: " << sslConnectHostOrIp);
 
-        //  copy error detail from bump-server-first request to CONNECT request
-        if (currentobject != NULL && currentobject->http != NULL && currentobject->http->request)
-            currentobject->http->request->detailError(sslServerBump->request->errType, sslServerBump->request->errDetail);
-    }
-
     getSslContextStart();
 }
 
@@ -4952,19 +4964,35 @@ ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io)
 }
 
 void
-ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth, bool monitor)
+ConnStateData::pinBusyConnection(const Comm::ConnectionPointer &pinServer, const HttpRequest::Pointer &request)
 {
-    if (!Comm::IsConnOpen(pinning.serverConnection) ||
-            pinning.serverConnection->fd != pinServer->fd)
-        pinNewConnection(pinServer, request, aPeer, auth);
+    pinConnection(pinServer, request);
+}
 
-    if (monitor)
-        startPinnedConnectionMonitoring();
+void
+ConnStateData::notePinnedConnectionBecameIdle(PinnedIdleContext pic)
+{
+    Must(pic.connection != NULL);
+    Must(pic.request != NULL);
+    pinConnection(pic.connection, pic.request);
+
+    // monitor pinned server connection for remote-end closures.
+    startPinnedConnectionMonitoring();
+
+    if (!currentobject)
+        kick(); // in case clientParseRequests() was blocked by a busy pic.connection
 }
 
+/// Forward future client requests using the given server connection.
 void
-ConnStateData::pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth)
+ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, const HttpRequest::Pointer &request)
 {
+    if (Comm::IsConnOpen(pinning.serverConnection) &&
+            pinning.serverConnection->fd == pinServer->fd) {
+        debugs(33, 3, "already pinned" << pinServer);
+        return;
+    }
+
     unpinConnection(true); // closes pinned connection, if any, and resets fields
 
     pinning.serverConnection = pinServer;
@@ -4973,23 +5001,21 @@ ConnStateData::pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRe
 
     Must(pinning.serverConnection != NULL);
 
-    // when pinning an SSL bumped connection, the request may be NULL
     const char *pinnedHost = "[unknown]";
-    if (request) {
+    if (request != NULL) {
         pinning.host = xstrdup(request->GetHost());
         pinning.port = request->port;
         pinnedHost = pinning.host;
+        pinning.auth = request->flags.connectionAuth;
     } else {
         pinning.port = pinServer->remote.port();
     }
     pinning.pinned = true;
-    if (aPeer)
-        pinning.peer = cbdataReference(aPeer);
-    pinning.auth = auth;
+    pinning.peer = cbdataReference(pinServer->getPeer());
     char stmp[MAX_IPSTRLEN];
     char desc[FD_DESC_SZ];
     snprintf(desc, FD_DESC_SZ, "%s pinned connection for %s (%d)",
-             (auth || !aPeer) ? pinnedHost : aPeer->name,
+             (pinning.auth || !pinning.peer) ? pinnedHost : pinning.peer->name,
              clientConnection->remote.toUrl(stmp,MAX_IPSTRLEN),
              clientConnection->fd);
     fd_note(pinning.serverConnection->fd, desc);
@@ -5164,3 +5190,8 @@ ConnStateData::unpinConnection(const bool andClose)
      * connection has gone away */
 }
 
+std::ostream &
+operator <<(std::ostream &os, const ConnStateData::PinnedIdleContext &pic)
+{
+    return os << pic.connection << ", request=" << pic.request;
+}

diff --git a/src/client_side.h b/src/client_side.h
index 93ad85e424927f6fd6dc63722c862ded048d9a49..d2cb4eace83a00c7e53888307973687bd6fcee2a 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -26,6 +26,8 @@
 #include "ssl/support.h"
 #endif
 
+#include <iosfwd>
+
 class ConnStateData;
 class ClientHttpRequest;
 class clientStreamNode;
@@ -188,6 +190,11 @@ public:
     /// Traffic parsing
     bool clientParseRequests();
     void readNextRequest();
+
+    // In v3.5, usually called via ClientSocketContext::keepaliveNextRequest().
+    /// try to make progress on a transaction or read more I/O
+    void kick();
+
     ClientSocketContext::Pointer getCurrentContext() const;
     void addContextToQueue(ClientSocketContext * context);
     int getConcurrentRequestCount() const;
@@ -287,9 +294,21 @@ public:
     bool handleReadData();
     bool handleRequestBodyData();
 
-    /// Forward future client requests using the given server connection.
-    /// Optionally, monitor pinned server connection for remote-end closures.
-    void pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth, bool monitor = true);
+    /// parameters for the async notePinnedConnectionBecameIdle() call
+    class PinnedIdleContext
+    {
+    public:
+        PinnedIdleContext(const Comm::ConnectionPointer &conn, const HttpRequest::Pointer &req): connection(conn), request(req) {}
+
+        Comm::ConnectionPointer connection; ///< to-server connection to be pinned
+        HttpRequest::Pointer request; ///< to-server request that initiated serverConnection
+    };
+
+    /// Called when a pinned connection becomes available for forwarding the next request.
+    void notePinnedConnectionBecameIdle(PinnedIdleContext pic);
+    /// Forward future client requests using the given to-server connection.
+    /// The connection is still being used by the current client request.
+    void pinBusyConnection(const Comm::ConnectionPointer &pinServerConn, const HttpRequest::Pointer &request);
     /// Undo pinConnection() and, optionally, close the pinned connection.
     void unpinConnection(const bool andClose);
     /// Returns validated pinnned server connection (and stops its monitoring).
@@ -345,7 +364,7 @@ public:
     /// generated
     void doPeekAndSpliceStep();
     /// called by FwdState when it is done bumping the server
-    void httpsPeeked(Comm::ConnectionPointer serverConnection);
+    void httpsPeeked(PinnedIdleContext pic);
 
     /// Start to create dynamic SSL_CTX for host or uses static port SSL context.
     void getSslContextStart();
@@ -449,7 +468,7 @@ private:
     void clientAfterReadingRequests();
     bool concurrentRequestQueueFilled() const;
 
-    void pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth);
+    void pinConnection(const Comm::ConnectionPointer &pinServerConn, const HttpRequest::Pointer &request);
 
     /* PROXY protocol functionality */
     bool proxyProtocolValidateClient();
@@ -516,5 +535,7 @@ ClientSocketContext *parseHttpRequest(ConnStateData *, HttpParser *, HttpRequest
 void clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *context, const HttpRequestMethod& method, Http::ProtocolVersion http_ver);
 void clientPostHttpsAccept(ConnStateData *connState);
 
+std::ostream &operator <<(std::ostream &os, const ConnStateData::PinnedIdleContext &pic);
+
 #endif /* SQUID_CLIENTSIDE_H */
 

diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc
index b8cc7f0b67f08f929b02df17a2591dd2bbdbf57c..0e68e0fb759b6e5fe9eb3ea517fb4c7372968cf8 100644 (file)
--- a/src/clients/FtpRelay.cc
+++ b/src/clients/FtpRelay.cc
@@ -210,9 +210,10 @@ Ftp::Relay::serverComplete()
                 mgr->unpinConnection(false);
                 ctrl.close();
             } else {
-                mgr->pinConnection(ctrl.conn, fwd->request,
-                                   ctrl.conn->getPeer(),
-                                   fwd->request->flags.connectionAuth);
+                CallJobHere1(9, 4, mgr,
+                             ConnStateData,
+                             notePinnedConnectionBecameIdle,
+                             ConnStateData::PinnedIdleContext(ctrl.conn, fwd->request));
                 ctrl.forget();
             }
         }

diff --git a/src/http.cc b/src/http.cc
index 61fefa60bed856a937002a541fd218ba7e12c307..161d58541cc2c90dc2993eabcf397cab94aa9bc7 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1383,9 +1383,6 @@ HttpStateData::decodeAndWriteReplyBody()
 void
 HttpStateData::processReplyBody()
 {
-    Ip::Address client_addr;
-    bool ispinned = false;
-
     if (!flags.headers_parsed) {
         flags.do_next_read = true;
         maybeReadVirginBody();
@@ -1435,35 +1432,49 @@ HttpStateData::processReplyBody()
         }
         break;
 
-        case COMPLETE_PERSISTENT_MSG:
+        case COMPLETE_PERSISTENT_MSG: {
             debugs(11, 5, "processReplyBody: COMPLETE_PERSISTENT_MSG from " << serverConnection);
-            /* yes we have to clear all these! */
+
+            // TODO: Remove serverConnectionSaved but preserve exception safety.
+
             commUnsetConnTimeout(serverConnection);
             flags.do_next_read = false;
 
             comm_remove_close_handler(serverConnection->fd, closeHandler);
             closeHandler = NULL;
-            fwd->unregister(serverConnection);
 
+            Ip::Address client_addr; // XXX: Remove as unused. Why was it added?
             if (request->flags.spoofClientIp)
                 client_addr = request->client_addr;
 
+            Comm::ConnectionPointer serverConnectionSaved = serverConnection;
+            fwd->unregister(serverConnection);
+            serverConnection = nullptr;
+
+            bool ispinned = false; // TODO: Rename to isOrShouldBePinned
             if (request->flags.pinned) {
                 ispinned = true;
             } else if (request->flags.connectionAuth && request->flags.authSent) {
                 ispinned = true;
             }
 
-            if (ispinned && request->clientConnectionManager.valid()) {
-                request->clientConnectionManager->pinConnection(serverConnection, request, _peer,
-                        (request->flags.connectionAuth));
+            if (ispinned) {
+                if (request->clientConnectionManager.valid()) {
+                    CallJobHere1(11, 4, request->clientConnectionManager,
+                                 ConnStateData,
+                                 notePinnedConnectionBecameIdle,
+                                 ConnStateData::PinnedIdleContext(serverConnectionSaved, request));
+                } else {
+                    // must not pool/share ispinned connections, even orphaned ones
+                    serverConnectionSaved->close();
+                }
             } else {
-                fwd->pconnPush(serverConnection, request->GetHost());
+                fwd->pconnPush(serverConnectionSaved, request->GetHost());
             }
 
-            serverConnection = NULL;
             serverComplete();
             return;
+        }
 
         case COMPLETE_NONPERSISTENT_MSG:
             debugs(11, 5, "processReplyBody: COMPLETE_NONPERSISTENT_MSG from " << serverConnection);

diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index 9678d6315487df38f3755e1b7aa76cb0f671b7cc..483af6210e428928a3ebfb6103328663f5a20df4 100644 (file)
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -301,12 +301,8 @@ Ftp::Server::notePeerConnection(Comm::ConnectionPointer conn)
     Must(http != NULL);
     HttpRequest *const request = http->request;
     Must(request != NULL);
-
-    // this is not an idle connection, so we do not want I/O monitoring
-    const bool monitor = false;
-
     // make FTP peer connection exclusive to our request
-    pinConnection(conn, request, conn->getPeer(), false, monitor);
+    pinBusyConnection(conn, request);
 }
 
 void

diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc
index 2b7b4fee633afa960b55abbd0fb7a84ccc3126a5..55e009837eb1a2dc411613763e98c86fc3fd2b21 100644 (file)
--- a/src/tests/stub_client_side.cc
+++ b/src/tests/stub_client_side.cc
@@ -60,7 +60,8 @@ void ConnStateData::noteMoreBodySpaceAvailable(BodyPipe::Pointer) STUB
 void ConnStateData::noteBodyConsumerAborted(BodyPipe::Pointer) STUB
 bool ConnStateData::handleReadData() STUB_RETVAL(false)
 bool ConnStateData::handleRequestBodyData() STUB_RETVAL(false)
-void ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth, bool monitor) STUB
+void ConnStateData::pinBusyConnection(const Comm::ConnectionPointer &, const HttpRequest::Pointer &) STUB
+void ConnStateData::notePinnedConnectionBecameIdle(PinnedIdleContext) STUB
 void ConnStateData::unpinConnection(const bool andClose) STUB
 const Comm::ConnectionPointer ConnStateData::validatePinnedConnection(HttpRequest *request, const CachePeer *peer) STUB_RETVAL(NULL)
 void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) STUB
@@ -70,7 +71,7 @@ void ConnStateData::requestTimeout(const CommTimeoutCbParams &params) STUB
 void ConnStateData::swanSong() STUB
 void ConnStateData::quitAfterError(HttpRequest *request) STUB
 #if USE_OPENSSL
-void ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection) STUB
+void ConnStateData::httpsPeeked(PinnedIdleContext) STUB
 void ConnStateData::getSslContextStart() STUB
 void ConnStateData::getSslContextDone(SSL_CTX * sslContext, bool isNew) STUB
 void ConnStateData::sslCrtdHandleReplyWrapper(void *data, const Helper::Reply &reply) STUB