------------------------------------------------------------ revno: 12627 revision-id: squid3@treenet.co.nz-20130929172800-vp3ul70gff41j4nj parent: squid3@treenet.co.nz-20130911015334-tu6v9uvjmlr4j9w9 author: Alex Rousskov committer: Amos Jeffries branch nick: 3.3 timestamp: Sun 2013-09-29 11:28:00 -0600 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20130929172800-vp3ul70gff41j4nj # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.3 # testament_sha1: 8da414a321a949fe1da5e1a62804916db186cfb7 # timestamp: 2013-09-29 17:54:58 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.3 # base_revision_id: squid3@treenet.co.nz-20130911015334-\ # tu6v9uvjmlr4j9w9 # # Begin patch === modified file 'src/client_side.cc' --- src/client_side.cc 2013-09-11 01:05:03 +0000 +++ src/client_side.cc 2013-09-29 17:28:00 +0000 @@ -4425,7 +4425,7 @@ 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 @@ 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 @@ 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 === modified file 'src/client_side.h' --- src/client_side.h 2013-01-28 04:11:25 +0000 +++ src/client_side.h 2013-09-29 17:28:00 +0000 @@ -265,6 +265,7 @@ 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 @@ /// 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 @@ 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); === modified file 'src/forward.cc' --- src/forward.cc 2013-06-29 08:20:01 +0000 +++ src/forward.cc 2013-09-29 17:28:00 +0000 @@ -975,6 +975,7 @@ else serverConn = NULL; if (Comm::IsConnOpen(serverConn)) { + pinned_connection->stopPinnedConnectionMonitoring(); flags.connected_okay = true; #if 0 if (!serverConn->getPeer())