Close idle client connections associated with closed idle pinned connections.

Squid was not monitoring idle persistent connections pinned to servers. Squid
would discover that the pinned server connection is closed only after
receiving a new request on the idle client connection and trying to write that
request to the server. In such cases, Squid propagates the pinned connection
closure to the client (as it should).

Chrome and, to a lesser extent, Firefox handle such races by opening a new
connection and resending the failed [idempotent] request transparently to the
user. However, IE usually displays an error page to the user.

While some pconn races cannot be avoided, without monitoring idle pconns,
Squid virtually guaranteed such a race in environments where origin server
idle connection timeout is smaller than client/Squid timeouts and users
are revisiting pages in the window between those two timeouts.

Squid now monitors idle pinned connections similar to idle connections in the
pconn pool and closes the corresponding idle client connection to keep the two
sides in sync (to the extent possible).

It is theoretically possible that this change will break servers that send
whitespace on an idle persistent connection or perhaps send some SSL keepalive
traffic. No such cases are known to exist though.

diff --git a/src/client_side.cc b/src/client_side.cc
index d7038b2a774d45eb1af6fddf20b115b33c1ea1ac..40fe8709e23f642bae27b10e25277da23aee9e86 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -4425,7 +4425,7 @@ ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io)
     pinning.closeHandler = NULL; // Comm unregisters handlers before calling
     const bool sawZeroReply = pinning.zeroReply; // reset when unpinning
     unpinConnection();
-    if (sawZeroReply) {
+    if (sawZeroReply && clientConnection != NULL) {
         debugs(33, 3, "Closing client connection on pinned zero reply.");
         clientConnection->close();
     }
@@ -4437,8 +4437,10 @@ ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpReque
     char desc[FD_DESC_SZ];
 
     if (Comm::IsConnOpen(pinning.serverConnection)) {
-        if (pinning.serverConnection->fd == pinServer->fd)
+        if (pinning.serverConnection->fd == pinServer->fd) {
+            startPinnedConnectionMonitoring();
             return;
+        }
     }
 
     unpinConnection(); // closes pinned connection, if any, and resets fields
@@ -4475,6 +4477,57 @@ ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpReque
     Params &params = GetCommParams<Params>(pinning.closeHandler);
     params.conn = pinning.serverConnection;
     comm_add_close_handler(pinning.serverConnection->fd, pinning.closeHandler);
+
+    startPinnedConnectionMonitoring();
+}
+
+/// Assign a read handler to an idle pinned connection so that we can detect connection closures.
+void
+ConnStateData::startPinnedConnectionMonitoring()
+{
+    if (pinning.readHandler != NULL)
+        return; // already monitoring
+
+    typedef CommCbMemFunT<ConnStateData, CommIoCbParams> Dialer;
+    pinning.readHandler = JobCallback(33, 3,
+                                      Dialer, this, ConnStateData::clientPinnedConnectionRead);
+    static char unusedBuf[8];
+    comm_read(pinning.serverConnection, unusedBuf, sizeof(unusedBuf), pinning.readHandler);
+}
+
+void
+ConnStateData::stopPinnedConnectionMonitoring()
+{
+    if (pinning.readHandler != NULL) {
+        comm_read_cancel(pinning.serverConnection->fd, pinning.readHandler);
+        pinning.readHandler = NULL;
+    }
+}
+
+/// Our read handler called by Comm when the server either closes an idle pinned connection or
+/// perhaps unexpectedly sends something on that idle (from Squid p.o.v.) connection.
+void
+ConnStateData::clientPinnedConnectionRead(const CommIoCbParams &io)
+{
+    pinning.readHandler = NULL; // Comm unregisters handlers before calling
+
+    if (io.flag == COMM_ERR_CLOSING)
+        return; // close handler will clean up
+
+    // We could use getConcurrentRequestCount(), but this may be faster.
+    const bool clientIsIdle = !getCurrentContext();
+
+    debugs(33, 3, "idle pinned " << pinning.serverConnection << " read " <<
+           io.size << (clientIsIdle ? " with idle client" : ""));
+
+    assert(pinning.serverConnection == io.conn);
+    pinning.serverConnection->close();
+
+    // If we are still sending data to the client, do not close now. When we are done sending,
+    // ClientSocketContext::keepaliveNextRequest() checks pinning.serverConnection and will close.
+    // However, if we are idle, then we must close to inform the idle client and minimize races.
+    if (clientIsIdle && clientConnection != NULL)
+        clientConnection->close();
 }
 
 const Comm::ConnectionPointer

diff --git a/src/client_side.h b/src/client_side.h
index 10c456757ad4d8c50650e41e10cc47bd5c713a98..4d87a8da999b272273697ba20f9713d8dc72816d 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -265,6 +265,7 @@ public:
         bool auth;               /* pinned for www authentication */
         bool zeroReply; ///< server closed w/o response (ERR_ZERO_SIZE_OBJECT)
         CachePeer *peer;             /* CachePeer the connection goes via */
+        AsyncCall::Pointer readHandler; ///< detects serverConnection closure
         AsyncCall::Pointer closeHandler; /*The close handler for pinned server side connection*/
     } pinning;
 
@@ -330,6 +331,9 @@ public:
     /// the client-side-detected error response instead of getting stuck.
     void quitAfterError(HttpRequest *request); // meant to be private
 
+    /// The caller assumes responsibility for connection closure detection.
+    void stopPinnedConnectionMonitoring();
+
 #if USE_SSL
     /// called by FwdState when it is done bumping the server
     void httpsPeeked(Comm::ConnectionPointer serverConnection);
@@ -377,6 +381,9 @@ protected:
     void abortChunkedRequestBody(const err_type error);
     err_type handleChunkedRequestBody(size_t &putSize);
 
+    void startPinnedConnectionMonitoring();
+    void clientPinnedConnectionRead(const CommIoCbParams &io);
+
 private:
     int connReadWasError(comm_err_t flag, int size, int xerrno);
     int connFinishedWithConn(int size);

diff --git a/src/forward.cc b/src/forward.cc
index 5afce2b00ebdde20e26ce616eb64dc94565330c8..e8cca4b1c334e82436e3894b997e0fd020823fc9 100644 (file)
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -975,6 +975,7 @@ FwdState::connectStart()
         else
             serverConn = NULL;
         if (Comm::IsConnOpen(serverConn)) {
+            pinned_connection->stopPinnedConnectionMonitoring();
             flags.connected_okay = true;
 #if 0
             if (!serverConn->getPeer())