Bug 3462: Delay Pools and ICAP

Allow StoreEntry::bytesWanted() API to ignore delay pools. Use that
feature when shoveling adapted response body from ICAP/eCAP BodyPipe
into Store.

If we do not ignore delay pools in
ServerStateData::handleMoreAdaptedBodyAvailable() context, and our pool
quota is exhausted, we will stop reading from the adaptation BodyPipe,
but there is no code/API to notify a BodyPipe reader when the quota
becomes available again.  With no reads from the pipe, the ICAP service
stops sending more adapted body data and possibly stops reading the
virgin body data from Squid, resulting in a stuck ICAP RESPMOD and the
master HTTP transaction.

We do need to call StoreEntry::bytesWanted() in this context because
otherwise Squid may run out of RAM (Squid bug #2619). The fix for that
problem inadvertently created this bug when delay pools were enabled.

Long-term, a "kick BodyPipe consumer when there is quota" code should be
added, and delay pools should be enabled for ICAP responses, but with
enough knobs for admins to disable ICAP pooling where needed. Most ICAP
environments will probably want to disable bandwidth controls because
ICAP service traffic is usually "local".

Removed StoreEntry::bytesWanted() TRY_FWD_HDR_WAIT guard that disabled
delay pool application until the final HTTP response headers are
received (HttpStateData::haveParsedReplyHeaders() is called). Nobody
could fully explain why that condition was really needed (and we
obviously want to limit bandwidth consumption at all response processing
stages, in general). The now-removed guard was bypassing delay pool
checks for virgin HTTP response (until the ICAP server responded with
adapted headers), causing bandwidth overuse.

Possibly fixes or helps with Squid bug #2606 as well.

diff --git a/src/MemObject.cc b/src/MemObject.cc
index b35d26cae619f7c153c521851950940a2ebce921..1cf74b8e942a70fc619131c8da06ce0985e4a0fa 100644 (file)
--- a/src/MemObject.cc
+++ b/src/MemObject.cc
@@ -415,16 +415,17 @@ MemObject::isContiguous() const
 }
 
 int
-MemObject::mostBytesWanted(int max) const
+MemObject::mostBytesWanted(int max, bool ignoreDelayPools) const
 {
 #if USE_DELAY_POOLS
-    /* identify delay id with largest allowance */
-    DelayId largestAllowance = mostBytesAllowed ();
-    return largestAllowance.bytesWanted(0, max);
-#else
+    if (!ignoreDelayPools) {
+        /* identify delay id with largest allowance */
+        DelayId largestAllowance = mostBytesAllowed ();
+        return largestAllowance.bytesWanted(0, max);
+    }
+#endif
 
     return max;
-#endif
 }
 
 void

diff --git a/src/MemObject.h b/src/MemObject.h
index fab4f0d517a5cad67d115bb2bd83726d61619c11..ee6fcc018c5848b1d5587aa99d8fdbad53022695 100644 (file)
--- a/src/MemObject.h
+++ b/src/MemObject.h
@@ -85,7 +85,7 @@ public:
     void trimSwappable();
     void trimUnSwappable();
     bool isContiguous() const;
-    int mostBytesWanted(int max) const;
+    int mostBytesWanted(int max, bool ignoreDelayPools) const;
     void setNoDelay(bool const newValue);
 #if USE_DELAY_POOLS
     DelayId mostBytesAllowed() const;

diff --git a/src/Server.cc b/src/Server.cc
index 16b533ea3549f0e2c31ece45a0aaa9993307f0a4..2d94160c21aeb8b4009437212bc5b266acf3953a 100644 (file)
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -734,7 +734,7 @@ ServerStateData::handleMoreAdaptedBodyAvailable()
     if (!contentSize)
         return; // XXX: bytesWanted asserts on zero-size ranges
 
-    const size_t spaceAvailable = entry->bytesWanted(Range<size_t>(0, contentSize));
+    const size_t spaceAvailable = entry->bytesWanted(Range<size_t>(0, contentSize), true);
 
     if (spaceAvailable < contentSize ) {
         // No or partial body data consuming

diff --git a/src/Store.h b/src/Store.h
index 331e58b84f449226f31ce2f383d0104803b7414e..afc9eb5e667352e23d93db350994245d2212217b 100644 (file)
--- a/src/Store.h
+++ b/src/Store.h
@@ -86,7 +86,7 @@ public:
     virtual void write (StoreIOBuffer);
     virtual _SQUID_INLINE_ bool isEmpty() const;
     virtual bool isAccepting() const;
-    virtual size_t bytesWanted(Range<size_t> const) const;
+    virtual size_t bytesWanted(Range<size_t> const aRange, bool ignoreDelayPool = false) const;
     virtual void complete();
     virtual store_client_t storeClientType() const;
     virtual char const *getSerialisedMetaData();
@@ -240,7 +240,7 @@ public:
 
     bool isEmpty () const {return true;}
 
-    virtual size_t bytesWanted(Range<size_t> const aRange) const { return aRange.end; }
+    virtual size_t bytesWanted(Range<size_t> const aRange, bool ignoreDelayPool = false) const { return aRange.end; }
 
     void operator delete(void *address);
     void complete() {}

diff --git a/src/store.cc b/src/store.cc
index 3511708e127b7416dd0e6f02c135a95b0bddf6d4..f7ad5839e8b9a4c13fd62a298816bb6575e67c2c 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -272,7 +272,7 @@ StoreEntry::delayAwareRead(const Comm::ConnectionPointer &conn, char *buf, int l
 }
 
 size_t
-StoreEntry::bytesWanted (Range<size_t> const aRange) const
+StoreEntry::bytesWanted (Range<size_t> const aRange, bool ignoreDelayPools) const
 {
     if (mem_obj == NULL)
         return aRange.end;
@@ -283,14 +283,10 @@ StoreEntry::bytesWanted (Range<size_t> const aRange) const
 
 #endif
 
-    /* Always read *something* here - we haven't got the header yet */
-    if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT))
-        return aRange.end;
-
     if (!mem_obj->readAheadPolicyCanRead())
         return 0;
 
-    return mem_obj->mostBytesWanted(aRange.end);
+    return mem_obj->mostBytesWanted(aRange.end, ignoreDelayPools);
 }
 
 bool

diff --git a/src/tests/stub_MemObject.cc b/src/tests/stub_MemObject.cc
index 51d7c26d7ca184a5738fd6ef36003382491516f5..f19309e91de6aa91ad07a5fbda1f8737d34cd3e8 100644 (file)
--- a/src/tests/stub_MemObject.cc
+++ b/src/tests/stub_MemObject.cc
@@ -106,12 +106,7 @@ MemObject::~MemObject()
     fatal ("Not implemented");
 }
 
-int
-MemObject::mostBytesWanted(int max) const
-{
-    fatal ("Not implemented");
-    return -1;
-}
+int MemObject::mostBytesWanted(int max, bool ignoreDelayPools) const STUB_RETVAL(-1)
 
 #if USE_DELAY_POOLS
 DelayId

diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc
index 98f674dff8c45527141c87ef7e6d6da7b5bf6934..d58703fad53d06d5a320b8e64d84e6bceac3853f 100644 (file)
--- a/src/tests/stub_store.cc
+++ b/src/tests/stub_store.cc
@@ -21,7 +21,7 @@ StoreEntry::~StoreEntry() STUB
 HttpReply const *StoreEntry::getReply() const STUB_RETVAL(NULL)
 void StoreEntry::write(StoreIOBuffer) STUB
 bool StoreEntry::isAccepting() const STUB_RETVAL(false)
-size_t StoreEntry::bytesWanted(Range<size_t> const) const STUB_RETVAL(0)
+size_t StoreEntry::bytesWanted(Range<size_t> const, bool) const STUB_RETVAL(0)
 void StoreEntry::complete() STUB
 store_client_t StoreEntry::storeClientType() const STUB_RETVAL(STORE_NON_CLIENT)
 char const *StoreEntry::getSerialisedMetaData() STUB_RETVAL(NULL)