Bug 5309: frequent "lowestOffset () <= target_offset" assertion (#1561)

Recent commit 122a6e3 left store_client::readOffset() unchanged but
should have adjusted it to match changed copyInto.offset semantics:
Starting with that commit, storeClientCopy() callers supply HTTP
response _body_ offset rather than HTTP response offset.

This bug decreased readOffset() values (by the size of stored HTTP
response headers), effectively telling Store that we are not yet done
with some of the MemObject/mem_hdr bytes. This bug could cause slightly
higher transaction memory usage because the same response bytes are
trimmed later. This bug should not have caused any assertions.

However, the old mem_hdr::freeDataUpto() code that uses readOffset() is
also broken -- the assertion in that method only "works" when
readOffset() returns values matching a memory node boundary. The smaller
values returned by buggy readOffset() triggered buggy assertions.

This minimal fix removes the recent store_client::readOffset() bug
described above. We will address old mem_hdr problems separately.

diff --git a/src/MemObject.cc b/src/MemObject.cc
index effbb2c4cd8f94681bb9664e8bb3f83c8d757292..d8f6687fe7f2ead83ccf4b4b61a91da8a10093c4 100644 (file)
--- a/src/MemObject.cc
+++ b/src/MemObject.cc
@@ -167,7 +167,7 @@ struct LowestMemReader : public unary_function<store_client, void> {
 
     void operator() (store_client const &x) {
         if (x.getType() == STORE_MEM_CLIENT)
-            current = std::min(current, x.readOffset());
+            current = std::min(current, x.discardableHttpEnd());
     }
 
     int64_t current;

diff --git a/src/StoreClient.h b/src/StoreClient.h
index 13f718ffe75da4d7cceb313aac9b8e63e83f687b..4bd6965b0e91fe73eec167b5e34c1471147d5a28 100644 (file)
--- a/src/StoreClient.h
+++ b/src/StoreClient.h
@@ -74,15 +74,8 @@ public:
     explicit store_client(StoreEntry *);
     ~store_client();
 
-    /// An offset into the stored response bytes, including the HTTP response
-    /// headers (if any). Note that this offset does not include Store entry
-    /// metadata, because it is not a part of the stored response.
-    /// \retval 0 means the client wants to read HTTP response headers.
-    /// \retval +N the response byte that the client wants to read next.
-    /// \retval -N should not occur.
-    // TODO: Callers do not expect negative offset. Verify that the return
-    // value cannot be negative and convert to unsigned in this case.
-    int64_t readOffset() const { return copyInto.offset; }
+    /// the client will not use HTTP response bytes with lower offsets (if any)
+    auto discardableHttpEnd() const { return discardableHttpEnd_; }
 
     int getType() const;
 
@@ -175,8 +168,16 @@ private:
 
     /// Storage and metadata associated with the current copy() request. Ought
     /// to be ignored when not answering a copy() request.
+    /// * copyInto.offset is the requested HTTP response body offset;
+    /// * copyInto.data is the client-owned, client-provided result buffer;
+    /// * copyInto.length is the size of the .data result buffer;
+    /// * copyInto.flags are unused by this class.
     StoreIOBuffer copyInto;
 
+    // TODO: Convert to uint64_t after fixing mem_hdr::endOffset() and friends.
+    /// \copydoc discardableHttpEnd()
+    int64_t discardableHttpEnd_ = 0;
+
     /// the total number of finishCallback() calls
     uint64_t answers;
 

diff --git a/src/store_client.cc b/src/store_client.cc
index 7fedc24ebda719f5f6b3f193b33ee05c508e2cba..9253c2253f084c727dde30e2f4bae886caa9abdd 100644 (file)
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -163,6 +163,16 @@ store_client::finishCallback()
         result = parsingBuffer->packBack();
     result.flags.error = object_ok ? 0 : 1;
 
+    // TODO: Move object_ok handling above into this `if` statement.
+    if (object_ok) {
+        // works for zero hdr_sz cases as well; see also: nextHttpReadOffset()
+        discardableHttpEnd_ = NaturalSum<int64_t>(entry->mem().baseReply().hdr_sz, result.offset, result.length).value();
+    } else {
+        // object_ok is sticky, so we will not be able to use any response bytes
+        discardableHttpEnd_ = entry->mem().endOffset();
+    }
+    debugs(90, 7, "with " << result << "; discardableHttpEnd_=" << discardableHttpEnd_);
+
     // no HTTP headers and no body bytes (but not because there was no space)
     atEof_ = !sendingHttpHeaders() && !result.length && copyInto.length;
 
@@ -265,6 +275,9 @@ store_client::copy(StoreEntry * anEntry,
 
     parsingBuffer.emplace(copyInto);
 
+    discardableHttpEnd_ = nextHttpReadOffset();
+    debugs(90, 7, "discardableHttpEnd_=" << discardableHttpEnd_);
+
     static bool copying (false);
     assert (!copying);
     copying = true;