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 08082490239022264223e70b0ac05beafd3ecc0b..2daac4292742a2ea278026a79e35734778df5914 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -254,7 +254,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 {
@@ -975,7 +975,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 a3b2fd7fa5812a6878e75c6317ae17e4523fc12c..0e06a5aeed3a29c6580f79710aac060cd7fd5d7f 100644 (file)
--- a/src/base/RefCount.h
+++ b/src/base/RefCount.h
@@ -70,11 +70,12 @@ public:
 
     C * operator-> () const {return const_cast<C *>(p_); }
 
-    C & operator * () const {return *const_cast<C *>(p_); }
-
-    C const * getRaw() const {return p_; }
+    C & operator * () const {
+        assert(p_);
+        return *const_cast<C *>(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 758cfa30b0feff7405abbe000fba077404436d1e..8c21bc34e0b51c14ab4468070966fef308d8521e 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -592,6 +592,7 @@ ConnStateData::swanSong()
     clientdbEstablished(clientConnection->remote, -1);  /* decrement */
     pipeline.terminateAll(0);
 
+    // XXX: Closing pinned conn is too harsh: The Client may want to continue!
     unpinConnection(true);
 
     Server::swanSong(); // closes the client connection
@@ -2097,6 +2098,13 @@ ConnStateData::clientParseRequests()
     // On errors, bodyPipe may become nil, but readMore will be cleared
     while (!inBuf.isEmpty() && !bodyPipe && flags.readMore) {
 
+        // 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;
@@ -3298,22 +3306,18 @@ ConnStateData::doPeekAndSpliceStep()
 }
 
 void
-ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection)
+ConnStateData::httpsPeeked(PinnedIdleContext pic)
 {
     Must(sslServerBump != NULL);
+    Must(sslServerBump->request == pic.request);
+    Must(pipeline.empty() || pipeline.front()->http == nullptr || pipeline.front()->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 (!pipeline.empty() && pipeline.front()->http != nullptr && pipeline.front()->http->request)
-            pipeline.front()->http->request->detailError(sslServerBump->request->errType, sslServerBump->request->errDetail);
-    }
-
     getSslContextStart();
 }
 
@@ -3855,19 +3859,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);
+    Must(pic.request);
+    pinConnection(pic.connection, *pic.request);
+
+    // monitor pinned server connection for remote-end closures.
+    startPinnedConnectionMonitoring();
+
+    if (pipeline.empty())
+        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 &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;
@@ -3876,23 +3896,18 @@ 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) {
-        pinning.host = xstrdup(request->url.host());
-        pinning.port = request->url.port();
-        pinnedHost = pinning.host;
-    } else {
-        pinning.port = pinServer->remote.port();
-    }
+    pinning.host = xstrdup(request.url.host());
+    pinning.port = request.url.port();
+    pinnedHost = pinning.host;
     pinning.pinned = true;
-    if (aPeer)
+    if (CachePeer *aPeer = pinServer->getPeer())
         pinning.peer = cbdataReference(aPeer);
-    pinning.auth = auth;
+    pinning.auth = request.flags.connectionAuth;
     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);
@@ -4091,6 +4106,7 @@ ConnStateData::checkLogging()
     /* Create a temporary ClientHttpRequest object. Its destructor will log. */
     ClientHttpRequest http(this);
     http.req_sz = inBuf.length();
+    // XXX: Or we died while waiting for the pinned connection to become idle.
     char const *uri = "error:transaction-end-before-headers";
     http.uri = xstrdup(uri);
     setLogUri(&http, uri);
@@ -4108,3 +4124,9 @@ ConnStateData::mayTunnelUnsupportedProto()
            ;
 }
 
+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 23731fc3bb520ad63e66594d80cea798c349999f..b5665e6091b6436e98b70857031db9f5d3e5c7d1 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -28,6 +28,8 @@
 #include "ssl/support.h"
 #endif
 
+#include <iosfwd>
+
 class ClientHttpRequest;
 class HttpHdrRangeSpec;
 
@@ -158,9 +160,21 @@ public:
 
     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).
@@ -214,7 +228,7 @@ public:
     /// generated
     void doPeekAndSpliceStep();
     /// called by FwdState when it is done bumping the server
-    void httpsPeeked(Comm::ConnectionPointer serverConnection);
+    void httpsPeeked(PinnedIdleContext pic);
 
     /// Splice a bumped client connection on peek-and-splice mode
     bool splice();
@@ -346,7 +360,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 &request);
 
     /* PROXY protocol functionality */
     bool proxyProtocolValidateClient();
@@ -425,5 +439,7 @@ Http::Stream *parseHttpRequest(ConnStateData *, const Http1::RequestParserPointe
 void clientProcessRequest(ConnStateData *, const Http1::RequestParserPointer &, Http::Stream *);
 void clientPostHttpsAccept(ConnStateData *);
 
+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 c702ad53ae15772cb69924b950079166e2d7367d..578202953bc3d3c55380afb06ea2e720f9ff1361 100644 (file)
--- a/src/clients/FtpRelay.cc
+++ b/src/clients/FtpRelay.cc
@@ -211,9 +211,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 09bbf4e62043f6787ee58326af5bbf1b3b9b150e..864f4fab26d7c628c6c671330ea4553d783766d1 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1394,9 +1394,6 @@ HttpStateData::decodeAndWriteReplyBody()
 void
 HttpStateData::processReplyBody()
 {
-    Ip::Address client_addr;
-    bool ispinned = false;
-
     if (!flags.headers_parsed) {
         flags.do_next_read = true;
         maybeReadVirginBody();
@@ -1449,35 +1446,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;
 
+            auto 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->url.host());
+                fwd->pconnPush(serverConnectionSaved, request->url.host());
             }
 
-            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 5968695c1caecadc125c0623c42a49081a496478..bbede7bed3727f241e7dc1f78349329b677648a6 100644 (file)
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -303,12 +303,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 0dc49d9f5f43e81b23d22324ee5973f9d9f93dc9..04e3fe064813169ebb4c11f9aede6bf8cb31ac45 100644 (file)
--- a/src/tests/stub_client_side.cc
+++ b/src/tests/stub_client_side.cc
@@ -31,7 +31,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 &, HttpRequest *, CachePeer *, bool, bool) STUB
+void ConnStateData::pinBusyConnection(const Comm::ConnectionPointer &, const HttpRequest::Pointer &) STUB
+void ConnStateData::notePinnedConnectionBecameIdle(PinnedIdleContext) STUB
 void ConnStateData::unpinConnection(const bool) STUB
 const Comm::ConnectionPointer ConnStateData::validatePinnedConnection(HttpRequest *, const CachePeer *) STUB_RETVAL(NULL)
 void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &) STUB
@@ -40,7 +41,7 @@ void ConnStateData::requestTimeout(const CommTimeoutCbParams &) STUB
 void ConnStateData::swanSong() STUB
 void ConnStateData::quitAfterError(HttpRequest *) STUB
 #if USE_OPENSSL
-void ConnStateData::httpsPeeked(Comm::ConnectionPointer) STUB
+void ConnStateData::httpsPeeked(PinnedIdleContext) STUB
 void ConnStateData::getSslContextStart() STUB
 void ConnStateData::getSslContextDone(Security::ContextPointer &, bool) STUB
 void ConnStateData::sslCrtdHandleReplyWrapper(void *, const Helper::Reply &) STUB