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 e497edce9208dfc608d32bcf3f235acdc1876a96..4684332334e538dd3274edd168bb3b49504b2d23 100644 (file)
--- a/src/MemObject.cc
+++ b/src/MemObject.cc
@@ -376,16 +376,17 @@ MemObject::isContiguous() const
 }
 
 int
-MemObject::mostBytesWanted(int max) const
+MemObject::mostBytesWanted(int max, bool ignoreDelayPools) const
 {
 #if 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 5464ccb1972b9e5a935c02ffe916ebf5b78e5a61..dedf8f50d3b0e378dc3adbc0d3ebd62a47dd4cee 100644 (file)
--- a/src/MemObject.h
+++ b/src/MemObject.h
@@ -77,7 +77,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 DELAY_POOLS
 

diff --git a/src/Server.cc b/src/Server.cc
index 9c597a9abb8afe213a6a0ee1615bec4cb06cfe09..1c28d885b3f473e2e0a2e3f6ca14bbb53b3d8fc9 100644 (file)
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -721,7 +721,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 0b7ea9d0eaf01cf56349365c61ca49649ace9870..cd3d06601e363820bd17828eaa03c70489b22584 100644 (file)
--- a/src/Store.h
+++ b/src/Store.h
@@ -79,7 +79,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();
@@ -224,7 +224,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 ea54b6cb0cd1c098ae14cfb1f47d0d27d3de6220..c3eafb8b323b76e3ae10cf9a3ede08a3a0da75f4 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -257,7 +257,7 @@ StoreEntry::delayAwareRead(int fd, char *buf, int len, AsyncCall::Pointer callba
 }
 
 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;
@@ -268,14 +268,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 ff89a1aa59d52348ce1ff5e42058a70ffebc81fe..ab2c451a2c6086bd85f27db7aa52a21a01a5a78e 100644 (file)
--- a/src/tests/stub_MemObject.cc
+++ b/src/tests/stub_MemObject.cc
@@ -105,12 +105,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 DELAY_POOLS
 DelayId

diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc
index 80cd517ba5d75861a5de652e870b0ce61156feba..e22af2ffcc57c3bdd5e52bc72e15b65e296373fe 100644 (file)
--- a/src/tests/stub_store.cc
+++ b/src/tests/stub_store.cc
@@ -19,7 +19,7 @@ StoreEntry::StoreEntry(const char *url, const char *log_url) 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)