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 556742e7ca0ff5c114a9d044e91da90421ade9e2..b5e2d7a14427a8e25e674c3f8eed635900dec871 100644 (file)
--- 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 d55cce5cd194639d2a479880b2c079e7ddc617b1..6961046a747096b5b716e796513f4edb020f8bc1 100644 (file)
--- 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 77dd1d7e5881ad468929da744440de0324398020..dd7bac4b7af42a438a42861277949d98606ad222 100644 (file)
--- 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: {