From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Tue, 8 May 2012 18:14:08 +0000 (-0600)
Subject: Bug 3466: Adaptation stuck on last single-byte body piece
X-Git-Tag: BumpSslServerFirst.take08~7^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4dc2b072;p=thirdparty%2Fsquid.git

Bug 3466: Adaptation stuck on last single-byte body piece

Changed StoreEntry::bytesWanted(range) to return range.end when the entry can
accommodate range.end bytes. This makes it possible to use that method for
single-byte ranges. Old code returned zero for such ranges, which was
difficult to distinguish from situations where no bytes were wanted at all.

TODO: The StoreEntry::bytesWanted(range) API is left undocumented because it
seems to be slightly broken and/or inconsistent with callers and with the
DelayId::bytesWanted(min, max) API. AFAICT, we should convert
StoreEntry::bytesWanted API from range-based to min/max-based or even just
max-based.

Store Entry API does not use the lower end of the range (except for the
now-removed assertion that the range is not empty). I suspect that Store API
was meant to be used with (first, last+1) "byte position" parameters (returning
the number of bytes wanted) while the DelayId API was meant to be used with
(min, max) "number of bytes" parameters.  However, StoreEntry::bytesWanted
implementation does not follow this assumption so perhaps my speculation is
wrong and there are more problems, including this change.
---

diff --git a/src/Server.cc b/src/Server.cc
index beffab6a46..16b533ea35 100644
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -734,10 +734,7 @@ ServerStateData::handleMoreAdaptedBodyAvailable()
     if (!contentSize)
         return; // XXX: bytesWanted asserts on zero-size ranges
 
-    // XXX: entry->bytesWanted returns contentSize-1 if entry can accept data.
-    // We have to add 1 to avoid suspending forever.
-    const size_t bytesWanted = entry->bytesWanted(Range<size_t>(0, contentSize));
-    const size_t spaceAvailable = bytesWanted >  0 ? (bytesWanted + 1) : 0;
+    const size_t spaceAvailable = entry->bytesWanted(Range<size_t>(0, contentSize));
 
     if (spaceAvailable < contentSize ) {
         // No or partial body data consuming
@@ -747,8 +744,7 @@ ServerStateData::handleMoreAdaptedBodyAvailable()
         entry->deferProducer(call);
     }
 
-    // XXX: bytesWanted API does not allow us to write just one byte!
-    if (!spaceAvailable && contentSize > 1)  {
+    if (!spaceAvailable)  {
         debugs(11, 5, HERE << "NOT storing " << contentSize << " bytes of adapted " <<
                "response body at offset " << adaptedBodySource->consumedSize());
         return;
diff --git a/src/Store.h b/src/Store.h
index 38c51e2cc0..331e58b84f 100644
--- a/src/Store.h
+++ b/src/Store.h
@@ -240,7 +240,7 @@ public:
 
     bool isEmpty () const {return true;}
 
-    virtual size_t bytesWanted(Range<size_t> const aRange) const { assert (aRange.size()); return aRange.end - 1;}
+    virtual size_t bytesWanted(Range<size_t> const aRange) const { return aRange.end; }
 
     void operator delete(void *address);
     void complete() {}
diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 4fcca31076..7c738f45d1 100644
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1724,10 +1724,7 @@ ClientHttpRequest::noteMoreBodyDataAvailable(BodyPipe::Pointer)
     assert(adaptedBodySource != NULL);
 
     if (size_t contentSize = adaptedBodySource->buf().contentSize()) {
-        // XXX: entry->bytesWanted returns contentSize-1 if entry can accept data.
-        // We have to add 1 to avoid suspending forever.
-        const size_t bytesWanted = storeEntry()->bytesWanted(Range<size_t>(0,contentSize));
-        const size_t spaceAvailable = bytesWanted >  0 ? (bytesWanted + 1) : 0;
+        const size_t spaceAvailable = storeEntry()->bytesWanted(Range<size_t>(0,contentSize));
 
         if (spaceAvailable < contentSize ) {
             // No or partial body data consuming
@@ -1737,8 +1734,7 @@ ClientHttpRequest::noteMoreBodyDataAvailable(BodyPipe::Pointer)
             storeEntry()->deferProducer(call);
         }
 
-        // XXX: bytesWanted API does not allow us to write just one byte!
-        if (!spaceAvailable && contentSize > 1)
+        if (!spaceAvailable)
             return;
 
         if (spaceAvailable < contentSize )
diff --git a/src/store.cc b/src/store.cc
index eb55af882d..3511708e12 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -274,10 +274,8 @@ StoreEntry::delayAwareRead(const Comm::ConnectionPointer &conn, char *buf, int l
 size_t
 StoreEntry::bytesWanted (Range<size_t> const aRange) const
 {
-    assert (aRange.size());
-
     if (mem_obj == NULL)
-        return aRange.end - 1;
+        return aRange.end;
 
 #if URL_CHECKSUM_DEBUG
 
@@ -287,12 +285,12 @@ StoreEntry::bytesWanted (Range<size_t> const aRange) const
 
     /* Always read *something* here - we haven't got the header yet */
     if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT))
-        return aRange.end - 1;
+        return aRange.end;
 
     if (!mem_obj->readAheadPolicyCanRead())
         return 0;
 
-    return mem_obj->mostBytesWanted(aRange.end - 1);
+    return mem_obj->mostBytesWanted(aRange.end);
 }
 
 bool