Bug #2116 fix: Better handling of HTTP 206 Partial Content responses.

When Squid is not doing the ranges it may still forward a Range request to the
origin server and receive a range response. The old code was comparing
endOffset of memObject with the last expected offset according to the Range
header value. That comparison did not account for the header size in
endOffset.

I do not know if the header size should not have been there or
should have been accounted for in the comparison. Adding headers size made
the 206 problem more difficult to reproduce (but it was still there).

Instead of trying to figure out when the header should or should not be
counted, I used http->out.offset and reply->content_range->spec.length.  That
seems to work. This change, however, is in my Puzzle Area of Squid; I have low
confidence the fix is correct and cannot be improved.

diff --git a/src/client_side.cc b/src/client_side.cc
index 279ce06541680069741a7426a4c73b89efbb1dd1..85c2f0ec704291b37cedbe3149faa409c11b426f 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side.cc,v 1.767 2007/09/28 00:22:38 hno Exp $
+ * $Id: client_side.cc,v 1.768 2007/10/30 21:52:30 rousskov Exp $
  *
  * DEBUG: section 33    Client-side Routines
  * AUTHOR: Duane Wessels
@@ -1204,6 +1204,7 @@ ClientSocketContext::sendStartOfMessage(HttpReply * rep, StoreIOBuffer bodyData)
     }
 
     /* write */
+    debugs(33,7, HERE << "sendStartOfMessage schedules clientWriteComplete");
     comm_write_mbuf(fd(), mb, clientWriteComplete, this);
 
     delete mb;
@@ -1290,6 +1291,7 @@ clientSocketDetach(clientStreamNode * node, ClientHttpRequest * http)
 static void
 clientWriteBodyComplete(int fd, char *buf, size_t size, comm_err_t errflag, int xerrno, void *data)
 {
+    debugs(33,7, HERE << "clientWriteBodyComplete schedules clientWriteComplete");
     clientWriteComplete(fd, NULL, size, errflag, xerrno, data);
 }
 
@@ -1497,15 +1499,30 @@ ClientSocketContext::socketState()
                     return STREAM_UNPLANNED_COMPLETE;
             }
         } else if (reply && reply->content_range) {
-            /* reply has content-range, but request did not */
+            /* reply has content-range, but Squid is not managing ranges */
+            const int64_t &bytesSent = http->out.offset;
+            const int64_t &bytesExpected = reply->content_range->spec.length;
 
-            if (http->memObject()->endOffset() <=
-                    reply->content_range->spec.offset + reply->content_range->spec.length) {
+            debugs(33, 7, HERE << "body bytes sent vs. expected: " <<
+                bytesSent << " ? " << bytesExpected << " (+" <<
+                reply->content_range->spec.offset << ")");
+
+            // did we get at least what we expected, based on range specs?
+
+            if (bytesSent == bytesExpected) // got everything
+                return STREAM_COMPLETE;
+
+            // The logic below is not clear: If we got more than we
+            // expected why would persistency matter? Should not this
+            // always be an error?
+            if (bytesSent > bytesExpected) { // got extra
                 if (http->request->flags.proxy_keepalive)
                     return STREAM_COMPLETE;
                 else
                     return STREAM_UNPLANNED_COMPLETE;
             }
+
+            // did not get enough yet, expecting more
         }
 
         return STREAM_NONE;