From: Amos Jeffries <squid3@treenet.co.nz>
Date: Mon, 16 Jul 2012 06:28:20 +0000 (-0600)
Subject: Bug 3577: File Descriptors not properly closed in trunk r12185.
X-Git-Tag: SQUID_3_2_0_19~28
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2c0db7d92f8b5d6ebc459e6fc2c4fda500b7abb1;p=thirdparty%2Fsquid.git

Bug 3577: File Descriptors not properly closed in trunk r12185.
Bug 3583: Server connection stuck after TCP_REFRESH_UNMODIFIED.

These changes fix FD leaks and stuck connections under two conditions:

1) Client aborts while Squid's server-side establishes a connection

Bug 3577: When a client quits while ConnOpener is trying to open the
connection to the next hop, FwdState cancels its ConnOpener callback.
ConnOpener notices that when trying to connect again and quits before
establishing a connection. The ConnOpener cleanup code did not close the
temporary FD used for establishing the connection. It did call fd_close(),
but fd_close() does not close the FD, naturally.

ConnOpener was probably leaking the temporary FD in other error handling
cases as well. It was never closed unless the connection was successful.


2) Client aborts after Squid's server-side established a connection:

Bug 3583: When a client aborts the store entry after receiving an HTTP 304 Not
Modified reply in response to a cache refreshing IMS request, HttpStateData
notices an aborted Store entry (after writing the reply to store), but does
virtually nothing, often resulting in a stuck server connection, leaking a
descriptor.  Now we abort the server-side transaction in this case.

Bug 3577: Similarly, when a client disconnects after Squid started talking to
the origin server but before Squid received a [complete] server response,
HttpStateData notices an aborted Store entry (during the next read from the
origin server), but does virtually nothing, often resulting in a stuck server
connection, leaking a descriptor. Now we abort the server-side transaction in
this case.

FwdState now also closes the server-side connection, if any, when the client
aborts the store entry and FwdState::abort() callback is called. This helps
reduce the number of concurrent server-side connections when clients abort
connections rapidly as Squid no longer has to wait for the server-side I/O to
notice that the entry is gone. The code to close the connection was temporary
removed in trunk r10057.1.51.
---

diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc
index f8d464fbe8..27f0476902 100644
--- a/src/comm/ConnOpener.cc
+++ b/src/comm/ConnOpener.cc
@@ -131,6 +131,7 @@ Comm::ConnOpener::doneConnecting(comm_err_t status, int xerrno)
     }
 
     if (temporaryFd_ >= 0) {
+        debugs(5, 4, HERE << conn_ << " closing temp FD " << temporaryFd_);
         // it never reached fully open, so cleanup the FD handlers
         // Note that comm_close() sequence does not happen for partially open FD
         Comm::SetSelect(temporaryFd_, COMM_SELECT_WRITE, NULL, NULL, 0);
@@ -141,6 +142,7 @@ Comm::ConnOpener::doneConnecting(comm_err_t status, int xerrno)
         }
         fd_table[temporaryFd_].timeoutHandler = NULL;
         fd_table[temporaryFd_].timeout = 0;
+        close(temporaryFd_);
         fd_close(temporaryFd_);
         temporaryFd_ = -1;
     }
diff --git a/src/forward.cc b/src/forward.cc
index d55cce5cd1..6961046a74 100644
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -84,6 +84,11 @@ FwdState::abort(void* d)
 
     if (Comm::IsConnOpen(fwd->serverConnection())) {
         comm_remove_close_handler(fwd->serverConnection()->fd, fwdServerClosedWrapper, fwd);
+        debugs(17, 3, HERE << "store entry aborted; closing " <<
+               fwd->serverConnection());
+        fwd->serverConnection()->close();
+    } else {
+        debugs(17, 7, HERE << "store entry aborted; no connection to close");
     }
     fwd->serverDestinations.clean();
     fwd->self = NULL;
diff --git a/src/http.cc b/src/http.cc
index edd96bb091..b9115be184 100644
--- a/src/http.cc
+++ b/src/http.cc
@@ -1067,7 +1067,7 @@ HttpStateData::readReply(const CommIoCbParams &io)
     }
 
     if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
-        // TODO: should we call abortTransaction() here?
+        abortTransaction("store entry aborted while reading reply");
         return;
     }
 
@@ -1346,12 +1346,9 @@ HttpStateData::processReplyBody()
     }
 
     if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
-        /*
-         * The above writeReplyBody() call could ABORT this entry,
-         * in that case, the server FD should already be closed.
-         * there's nothing for us to do.
-         */
-        (void) 0;
+        // The above writeReplyBody() call may have aborted the store entry.
+        abortTransaction("store entry aborted while storing reply");
+        return;
     } else
         switch (persistentConnStatus()) {
         case INCOMPLETE_MSG: {