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 beffab6a461875de617b311f77316b3607b38758..16b533ea3549f0e2c31ece45a0aaa9993307f0a4 100644 (file)
--- 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 38c51e2cc0f2672d83befb24537e1854bb6084a0..331e58b84f449226f31ce2f383d0104803b7414e 100644 (file)
--- 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 4fcca310768055f7a376cec07a21d1fc7f6ef4be..7c738f45d105a49ac288affe3bd7ba2c19f767cc 100644 (file)
--- 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 eb55af882d7520798b18abb4d6857f24b7bea3e0..3511708e127b7416dd0e6f02c135a95b0bddf6d4 100644 (file)
--- 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