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/FwdState.cc b/src/FwdState.cc
index cbb5c3a8082400867a75f7e2977dc10bc51aa4b0..c7a38c334cf7699f3da77aaa2213506977ffbd60 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -1119,6 +1119,7 @@ FwdState::connectStart()
         else
             serverConn = NULL;
         if (Comm::IsConnOpen(serverConn)) {
+            pinned_connection->stopPinnedConnectionMonitoring();
             flags.connected_okay = true;
             ++n_tries;
             request->flags.pinned = true;

diff --git a/src/client_side.cc b/src/client_side.cc
index 4f0f579e56c2b286dd37eb3d0a744152afc305ca..a731d38ec3026c0d3f438179b1ca7071231756e6 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -4483,7 +4483,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();
     }
@@ -4495,8 +4495,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
@@ -4533,6 +4535,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 89ad23fddb9a5f24210d5b4d4a54e89ca12dba6a..ec95d11cc5874050c3c51a0cf7322635715867df 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -267,6 +267,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;
 
@@ -333,6 +334,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);
@@ -380,6 +384,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);