Fix two read-ahead problems related to delay pools (or lack of thereof).

1. Honor EOF on Squid-to-server connections with full read ahead buffers
   and no clients when --enable-delay-pools is used without any delay
   pools configured in squid.conf.

Since trunk r6150.

Squid delays reading from the server after buffering read_ahead_gap
bytes that are not yet sent to the client. A delayed read is normally
resumed after Squid sends more buffered bytes to the client. See
readAheadPolicyCanRead() and kickReads().

However, Squid was not resuming the delayed read after all Store clients
were gone. If quick_abort prevents Squid from immediately closing the
corresponding Squid-to-server connection, then the connection gets stuck
until read_timeout (15m), even if the server closes much sooner, --
without reading from the server, Squid cannot detect the connection
closure. The affected connections enter the CLOSE_WAIT state.

Kicking delayed read when the last client leaves fixes the problem. The
removal of any client, including the last one, may change
readAheadPolicyCanRead() answer and, hence, deserves a kickReads() call.

Why "without any delay pools configured in squid.conf"? When classic
(i.e., delay_pool_*) delay pools are configured, Squid kicks all delayed
reads every second. That periodic kicking is an old design bug, but it
resumes stuck reads when all Store clients are gone. Without classic
delay pools, there is no periodic kicking. This fix does not address
that old bug but removes Squid hidden dependence on its side effect.

Note that the Squid-to-server connections with full read-ahead buffers
still remain "stuck" if there are non-reading clients. There is nothing
Squid can do about them because we cannot reliably detect EOF without
reading at least one byte and such reading is not allowed by the read
ahead gap. In other words, non-reading clients still stall server
connections.

While fixing this, I moved all CheckQuickAbort() tests into
CheckQuickAbortIsReasonable() because we need a boolean function to
avoid kicking aborted entries and because the old separation was rather
awkward -- CheckQuickAbort() contained "reasonable" tests that were not
in CheckQuickAbortIsReasonable(). All the aborting tests and their order
were preserved during this move. The moved tests gained debugging.

According to the existing test order in CheckQuickAbortIsReasonable(),
the above problem can be caused by:

* non-private responses with a known content length
* non-private responses with unknown content length, having quick_abort_min
  set to -1 KB.

2. Honor read_ahead_gap with --disable-delay-pools.

Since trunk r13954.

This fix also addresses "Perhaps these two calls should both live
in MemObject" comment and eliminates existing code duplication.

diff --git a/src/MemObject.cc b/src/MemObject.cc
index 69961aa19d21736d9312e0c8dfb477042bcb4d2f..67eb5ed3bc02f31509aa2dc39bfa14fd2885660e 100644 (file)
--- a/src/MemObject.cc
+++ b/src/MemObject.cc
@@ -437,6 +437,14 @@ MemObject::setNoDelay(bool const newValue)
 void
 MemObject::delayRead(DeferredRead const &aRead)
 {
+#if USE_DELAY_POOLS
+        if (readAheadPolicyCanRead()) {
+            if (DelayId mostAllowedId = mostBytesAllowed()) {
+                mostAllowedId.delayRead(aRead);
+                return;
+            }
+        }
+#endif
     deferredReads.delayRead(aRead);
 }
 

diff --git a/src/http.cc b/src/http.cc
index 7c1281c04fcf874406cdd75cce78d6707f6fc9c2..10273c0d13d8b4b315f5f7b9003ffd5b2bd3a30e 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1128,7 +1128,6 @@ HttpStateData::persistentConnStatus() const
     return statusIfComplete();
 }
 
-#if USE_DELAY_POOLS
 static void
 readDelayed(void *context, CommRead const &)
 {
@@ -1136,7 +1135,6 @@ readDelayed(void *context, CommRead const &)
     state->flags.do_next_read = true;
     state->maybeReadVirginBody();
 }
-#endif
 
 void
 HttpStateData::readReply(const CommIoCbParams &io)
@@ -1171,23 +1169,13 @@ HttpStateData::readReply(const CommIoCbParams &io)
     CommIoCbParams rd(this); // will be expanded with ReadNow results
     rd.conn = io.conn;
     rd.size = entry->bytesWanted(Range<size_t>(0, inBuf.spaceSize()));
-#if USE_DELAY_POOLS
-    if (rd.size < 1) {
-        assert(entry->mem_obj);
 
-        /* read ahead limit */
-        /* Perhaps these two calls should both live in MemObject */
+    if (rd.size <= 0) {
+        assert(entry->mem_obj);
         AsyncCall::Pointer nilCall;
-        if (!entry->mem_obj->readAheadPolicyCanRead()) {
-            entry->mem_obj->delayRead(DeferredRead(readDelayed, this, CommRead(io.conn, NULL, 0, nilCall)));
-            return;
-        }
-
-        /* delay id limit */
-        entry->mem_obj->mostBytesAllowed().delayRead(DeferredRead(readDelayed, this, CommRead(io.conn, NULL, 0, nilCall)));
+        entry->mem_obj->delayRead(DeferredRead(readDelayed, this, CommRead(io.conn, NULL, 0, nilCall)));
         return;
     }
-#endif
 
     switch (Comm::ReadNow(rd, inBuf)) {
     case Comm::INPROGRESS:

diff --git a/src/store.cc b/src/store.cc
index d974882f3be3bb31197f7ba6403e49ce27b33e7e..2220725be881020f289e2ea86a55f515371117b0 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -196,24 +196,10 @@ StoreEntry::delayAwareRead(const Comm::ConnectionPointer &conn, char *buf, int l
      * ->deferRead (fd, buf, len, callback, DelayAwareRead, this)
      */
 
-    if (amountToRead == 0) {
+    if (amountToRead <= 0) {
         assert (mem_obj);
-        /* read ahead limit */
-        /* Perhaps these two calls should both live in MemObject */
-#if USE_DELAY_POOLS
-        if (!mem_obj->readAheadPolicyCanRead()) {
-#endif
-            mem_obj->delayRead(DeferredRead(DeferReader, this, CommRead(conn, buf, len, callback)));
-            return;
-#if USE_DELAY_POOLS
-        }
-
-        /* delay id limit */
-        mem_obj->mostBytesAllowed().delayRead(DeferredRead(DeferReader, this, CommRead(conn, buf, len, callback)));
+        mem_obj->delayRead(DeferredRead(DeferReader, this, CommRead(conn, buf, len, callback)));
         return;
-
-#endif
-
     }
 
     if (fd_table[conn->fd].closing()) {

diff --git a/src/store_client.cc b/src/store_client.cc
index afd95932d798d6b3d41e80099a2c96723560bc4b..1d4b67eeca5a7311001616a2ae6e25610c920834 100644 (file)
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -41,7 +41,6 @@ static StoreIOState::STRCB storeClientReadHeader;
 static void storeClientCopy2(StoreEntry * e, store_client * sc);
 static EVH storeClientCopyEvent;
 static bool CheckQuickAbortIsReasonable(StoreEntry * entry);
-static void CheckQuickAbort(StoreEntry * entry);
 
 CBDATA_CLASS_INIT(store_client);
 
@@ -697,11 +696,11 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data)
 
     assert(e->locked());
     // An entry locked by others may be unlocked (and destructed) by others, so
-    // we must lock again to safely dereference e after CheckQuickAbort().
+    // we must lock again to safely dereference e after CheckQuickAbortIsReasonable().
     e->lock("storeUnregister");
 
-    if (mem->nclients == 0)
-        CheckQuickAbort(e);
+    if (CheckQuickAbortIsReasonable(e))
+        e->abort();
     else
         mem->kickReads();
 
@@ -760,9 +759,32 @@ storePendingNClients(const StoreEntry * e)
 static bool
 CheckQuickAbortIsReasonable(StoreEntry * entry)
 {
+    assert(entry);
+    debugs(90, 3, "entry=" << *entry);
+
+    if (storePendingNClients(entry) > 0) {
+        debugs(90, 3, "quick-abort? NO storePendingNClients() > 0");
+        return false;
+    }
+
+    if (!shutting_down && Store::Root().transientReaders(*entry)) {
+        debugs(90, 3, "quick-abort? NO still have one or more transient readers");
+        return false;
+    }
+
+    if (entry->store_status != STORE_PENDING) {
+        debugs(90, 3, "quick-abort? NO store_status != STORE_PENDING");
+        return false;
+    }
+
+    if (EBIT_TEST(entry->flags, ENTRY_SPECIAL)) {
+        debugs(90, 3, "quick-abort? NO ENTRY_SPECIAL");
+        return false;
+    }
+
     MemObject * const mem = entry->mem_obj;
     assert(mem);
-    debugs(90, 3, "entry=" << entry << ", mem=" << mem);
+    debugs(90, 3, "mem=" << mem);
 
     if (mem->request && !mem->request->flags.cachable) {
         debugs(90, 3, "quick-abort? YES !mem->request->flags.cachable");
@@ -824,31 +846,6 @@ CheckQuickAbortIsReasonable(StoreEntry * entry)
     return true;
 }
 
-/// Aborts a swapping-out entry if nobody needs it any more _and_
-/// continuing swap out is not reasonable per CheckQuickAbortIsReasonable().
-static void
-CheckQuickAbort(StoreEntry * entry)
-{
-    assert (entry);
-
-    if (storePendingNClients(entry) > 0)
-        return;
-
-    if (!shutting_down && Store::Root().transientReaders(*entry))
-        return;
-
-    if (entry->store_status != STORE_PENDING)
-        return;
-
-    if (EBIT_TEST(entry->flags, ENTRY_SPECIAL))
-        return;
-
-    if (!CheckQuickAbortIsReasonable(entry))
-        return;
-
-    entry->abort();
-}
-
 void
 store_client::dumpStats(MemBuf * output, int clientNumber) const
 {