From 4c675a57e90c7cbc966ffeb3740fb3982ff8d40f Mon Sep 17 00:00:00 2001 From: Julien Pinon Date: Wed, 20 Jun 2012 18:39:01 -0600 Subject: [PATCH] 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. --- src/MemObject.cc | 13 +++++++------ src/MemObject.h | 2 +- src/Server.cc | 2 +- src/Store.h | 4 ++-- src/store.cc | 8 ++------ src/tests/stub_MemObject.cc | 7 +------ src/tests/stub_store.cc | 2 +- 7 files changed, 15 insertions(+), 23 deletions(-) diff --git a/src/MemObject.cc b/src/MemObject.cc index b35d26cae6..1cf74b8e94 100644 --- 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 fab4f0d517..ee6fcc018c 100644 --- 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 16b533ea35..2d94160c21 100644 --- 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(0, contentSize)); + const size_t spaceAvailable = entry->bytesWanted(Range(0, contentSize), true); if (spaceAvailable < contentSize ) { // No or partial body data consuming diff --git a/src/Store.h b/src/Store.h index 331e58b84f..afc9eb5e66 100644 --- 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 const) const; + virtual size_t bytesWanted(Range 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 const aRange) const { return aRange.end; } + virtual size_t bytesWanted(Range 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 3511708e12..f7ad5839e8 100644 --- 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 const aRange) const +StoreEntry::bytesWanted (Range const aRange, bool ignoreDelayPools) const { if (mem_obj == NULL) return aRange.end; @@ -283,14 +283,10 @@ StoreEntry::bytesWanted (Range 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 51d7c26d7c..f19309e91d 100644 --- 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 98f674dff8..d58703fad5 100644 --- 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 const) const STUB_RETVAL(0) +size_t StoreEntry::bytesWanted(Range 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) -- 2.47.2