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 92c02ba5f8ad7b9676830bb21960c3034cd022e9..9c597a9abb8afe213a6a0ee1615bec4cb06cfe09 100644 (file)
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -721,10 +721,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
@@ -734,8 +731,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 043273c423fd96476dbbf91eb74ae3ac8e30c4a6..0b7ea9d0eaf01cf56349365c61ca49649ace9870 100644 (file)
--- a/src/Store.h
+++ b/src/Store.h
@@ -224,7 +224,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 eb5aeda7809480279894a61a1cfbaf87c176f65f..e18d6a40daef2fe570dfb2dbfbfdb8cab7c711ab 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1504,10 +1504,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
@@ -1517,8 +1514,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 7c0968991c72e8e0ef31f9b4e59f0bdc7ac6d7ff..ea54b6cb0cd1c098ae14cfb1f47d0d27d3de6220 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -259,10 +259,8 @@ StoreEntry::delayAwareRead(int fd, char *buf, int len, AsyncCall::Pointer callba
 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
 
@@ -272,12 +270,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