From 1c6192b543e9d5436a4f4c1e7c4eda0433ab2959 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 29 Sep 2013 11:28:00 -0600 Subject: [PATCH] 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. --- src/client_side.cc | 57 ++++++++++++++++++++++++++++++++++++++++++++-- src/client_side.h | 7 ++++++ src/forward.cc | 1 + 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/client_side.cc b/src/client_side.cc index d7038b2a77..40fe8709e2 100644 --- 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 ¶ms = GetCommParams(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 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 10c456757a..4d87a8da99 100644 --- 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 5afce2b00e..e8cca4b1c3 100644 --- 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()) -- 2.47.2