From: Alex Rousskov Date: Thu, 22 Aug 2013 18:39:41 +0000 (-0600) Subject: Close idle client connections associated with closed idle pinned connections. X-Git-Tag: SQUID_3_5_0_1~661 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7ac4092320177be991c3ed3f116fa7eaa6dcfccb;p=thirdparty%2Fsquid.git 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 cbb5c3a808..c7a38c334c 100644 --- 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 4f0f579e56..a731d38ec3 100644 --- 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 ¶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 89ad23fddb..ec95d11cc5 100644 --- 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);