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 10a06bc800384b4e9d1fe8ef66b3d030b7b0afec..141b73702eb32105e2bb4483e3ed9ef69de655dc 100644 (file)
--- a/src/MemObject.cc
+++ b/src/MemObject.cc
@@ -462,6 +462,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 e49cec280ff0c68df3d66d97697869ffed343953..4600df0c562e9ec2b3adfafd0919be2dca07fd42 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 7f4b1140b3cbbceacf12322de87324e0b775238e..ec2fd42d3060f5215a1ba64795ab9b5937badfed 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
 {