Bug 4616: store_client.cc:92: "mem" assertion (#50)

This bug was probably caused by Bug 2833 feature/fix (1a210de).

The primary fix here is limited to clientReplyContext::processExpired():
Collapsed forwarding code must ensure StoreEntry::mem_obj existence. It
was missing for cache hits purged from (or never admitted into) the
memory cache. Most storeClientListAdd() callers either have similar code
or call storeCreateEntry() which also creates StoreEntry::mem_obj.

Also avoided clobbering known StoreEntry URIs/method in some cases. The
known effect of this change is fixed store.log URI and method fields
when a hit transaction did not match the stored entry exactly (e.g., a
HEAD hit for a GET cached entry), but this improvement may have even
more important consequences: The original method is used by possibly
still-running entry filling code (e.g., determining the end of the
incoming response, validating the entry length, finding vary markers,
etc.). Changing the method affects those actions, essentially corrupting
the entry state. The same argument may apply to store ID and log URI.

We even tried to make URIs/method constant, but that is impractical w/o
addressing an XXX in MemStore::get(), which is outside this issue scope.
To facilitate that future fix, the code now distinguishes these cases:

* createMemObject(void): Buggy callers that create a new memory object
  but do not know what URIs/method the hosting StoreEntry was based on.
  Once these callers are fixed, we can make the URIs/method constant.

* createMemObject(trio): Callers that create a new memory object with
  URIs/method that match the hosting StoreEntry.

* ensureMemObject(trio): Callers that are not sure whether StoreEntry
  has a memory object but have URIs/method to create one if needed.

diff --git a/src/MemObject.cc b/src/MemObject.cc
index d608a21c53b635a0e2324209240b494102f38362..53cf1c53b93b705ec7466fe842236b8e7fd6a2ed 100644 (file)
--- a/src/MemObject.cc
+++ b/src/MemObject.cc
@@ -76,7 +76,11 @@ MemObject::hasUris() const
 void
 MemObject::setUris(char const *aStoreId, char const *aLogUri, const HttpRequestMethod &aMethod)
 {
+    if (hasUris())
+        return;
+
     storeId_ = aStoreId;
+    debugs(88, 3, this << " storeId: " << storeId_);
 
     // fast pointer comparison for a common storeCreateEntry(url,url,...) case
     if (!aLogUri || aLogUri == aStoreId)

diff --git a/src/MemObject.h b/src/MemObject.h
index 23d716ef2246fe301f3a08d6e2771d763a64eb90..35b1376157934be38bae95c3a296ed46d5b74034 100644 (file)
--- a/src/MemObject.h
+++ b/src/MemObject.h
@@ -39,7 +39,12 @@ public:
     MemObject();
     ~MemObject();
 
-    /// sets store ID, log URI, and request method; TODO: find a better name
+    /// Sets store ID, log URI, and request method (unless already set). Does
+    /// not clobber the method so that, say, a HEAD hit for a GET entry keeps
+    /// the GET method that matches the entry key. Same for the other parts of
+    /// the trio because the entry filling code may expect them to be constant.
+    /// XXX: Avoid this method. We plan to remove it and make the trio constant
+    /// after addressing the XXX in MemStore::get().
     void setUris(char const *aStoreId, char const *aLogUri, const HttpRequestMethod &aMethod);
 
     /// whether setUris() has been called

diff --git a/src/MemStore.cc b/src/MemStore.cc
index ad65db20c9fd866cad2b93f549e77aadb7715f6d..49eb2112fb403c6f1525d93d3f630792455b7c09 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -323,7 +323,7 @@ MemStore::get(const cache_key *key)
     // XXX: We do not know the URLs yet, only the key, but we need to parse and
     // store the response for the Root().get() callers to be happy because they
     // expect IN_MEMORY entries to already have the response headers and body.
-    e->makeMemObject();
+    e->createMemObject();
 
     anchorEntry(*e, index, *slot);
 

diff --git a/src/Store.h b/src/Store.h
index cb31b5c33dcd29303054fd0892d0da108d7166ab..a1f6ef97208488df262b008d661b107ce7aac672 100644 (file)
--- a/src/Store.h
+++ b/src/Store.h
@@ -115,12 +115,16 @@ public:
     int validToSend() const;
     bool memoryCachable(); ///< checkCachable() and can be cached in memory
 
-    /// if needed, initialize mem_obj member w/o URI-related information
-    MemObject *makeMemObject();
+    /// initialize mem_obj; assert if mem_obj already exists
+    /// avoid this method in favor of createMemObject(trio)!
+    void createMemObject();
 
-    /// initialize mem_obj member (if needed) and supply URI-related info
+    /// initialize mem_obj with URIs/method; assert if mem_obj already exists
     void createMemObject(const char *storeId, const char *logUri, const HttpRequestMethod &aMethod);
 
+    /// initialize mem_obj (if needed) and set URIs/method (if missing)
+    void ensureMemObject(const char *storeId, const char *logUri, const HttpRequestMethod &aMethod);
+
     void dump(int debug_lvl) const;
     void hashDelete();
     void hashInsert(const cache_key *);

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 5abfe917abe49434314c243f983289211a190029..f6ae6ea4be3f4c236451ecec060f64f9adb35268 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -308,6 +308,7 @@ clientReplyContext::processExpired()
     }
 
     if (entry) {
+        entry->ensureMemObject(url, http->log_uri, http->request->method);
         debugs(88, 5, "collapsed on existing revalidation entry: " << *entry);
         collapsedRevalidation = crSlave;
     } else {
@@ -993,7 +994,7 @@ clientReplyContext::purgeFoundObject(StoreEntry *entry)
     http->storeEntry(entry);
 
     http->storeEntry()->lock("clientReplyContext::purgeFoundObject");
-    http->storeEntry()->createMemObject(storeId(), http->log_uri,
+    http->storeEntry()->ensureMemObject(storeId(), http->log_uri,
                                         http->request->method);
 
     sc = storeClientListAdd(http->storeEntry(), this);
@@ -1845,22 +1846,7 @@ clientReplyContext::doGetMoreData()
 
         http->storeEntry()->lock("clientReplyContext::doGetMoreData");
 
-        MemObject *mem_obj = http->storeEntry()->makeMemObject();
-        if (!mem_obj->hasUris()) {
-            /*
-             * This if-block exists because we don't want to clobber
-             * a preexiting mem_obj->method value if the mem_obj
-             * already exists.  For example, when a HEAD request
-             * is a cache hit for a GET response, we want to keep
-             * the method as GET.
-             */
-            mem_obj->setUris(storeId(), http->log_uri, http->request->method);
-            /**
-             * Here we can see if the object was
-             * created using URL or alternative StoreID from helper.
-             */
-            debugs(88, 3, "storeId: " << http->storeEntry()->mem_obj->storeId());
-        }
+        http->storeEntry()->ensureMemObject(storeId(), http->log_uri, http->request->method);
 
         sc = storeClientListAdd(http->storeEntry(), this);
 #if USE_DELAY_POOLS

diff --git a/src/store.cc b/src/store.cc
index 9e4190fb95c6b516e10c55207b964c51fa6fc34f..2ffde12c8c5fed5cd18f30d94a7729d46799687c 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -779,8 +779,7 @@ storeCreatePureEntry(const char *url, const char *log_url, const RequestFlags &f
     debugs(20, 3, "storeCreateEntry: '" << url << "'");
 
     e = new StoreEntry();
-    e->makeMemObject();
-    e->mem_obj->setUris(url, log_url, method);
+    e->createMemObject(url, log_url, method);
 
     if (flags.cachable) {
         EBIT_CLR(e->flags, RELEASE_REQUEST);
@@ -1678,18 +1677,25 @@ StoreEntry::url() const
         return mem_obj->storeId();
 }
 
-MemObject *
-StoreEntry::makeMemObject()
+void
+StoreEntry::createMemObject()
 {
-    if (!mem_obj)
-        mem_obj = new MemObject();
-    return mem_obj;
+    assert(!mem_obj);
+    mem_obj = new MemObject();
 }
 
 void
 StoreEntry::createMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod)
 {
-    makeMemObject();
+    assert(!mem_obj);
+    ensureMemObject(aUrl, aLogUrl, aMethod);
+}
+
+void
+StoreEntry::ensureMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod)
+{
+    if (!mem_obj)
+        mem_obj = new MemObject();
     mem_obj->setUris(aUrl, aLogUrl, aMethod);
 }
 

diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc
index 5c6c38f128cb955095e78db58281629ef0d752d9..9ca29a359334710d1b485b6b1a1b4dedb11bf55a 100644 (file)
--- a/src/tests/stub_store.cc
+++ b/src/tests/stub_store.cc
@@ -54,8 +54,9 @@ int StoreEntry::checkNegativeHit() const STUB_RETVAL(0)
 int StoreEntry::locked() const STUB_RETVAL(0)
 int StoreEntry::validToSend() const STUB_RETVAL(0)
 bool StoreEntry::memoryCachable() STUB_RETVAL(false)
-MemObject *StoreEntry::makeMemObject() STUB_RETVAL(NULL)
+void StoreEntry::createMemObject() STUB
 void StoreEntry::createMemObject(const char *, const char *, const HttpRequestMethod &aMethod) STUB
+void StoreEntry::ensureMemObject(const char *, const char *, const HttpRequestMethod &) STUB
 void StoreEntry::dump(int debug_lvl) const STUB
 void StoreEntry::hashDelete() STUB
 void StoreEntry::hashInsert(const cache_key *) STUB

diff --git a/src/tests/testStoreController.cc b/src/tests/testStoreController.cc
index 116a50d409b55346f5a51c1728df51b94aa68ac5..abc478d4c13679926861ca629dba5ebe363fd791 100644 (file)
--- a/src/tests/testStoreController.cc
+++ b/src/tests/testStoreController.cc
@@ -31,8 +31,7 @@ testStoreController::testStats()
 {
     Store::Init();
     StoreEntry *logEntry = new StoreEntry;
-    logEntry->makeMemObject();
-    logEntry->mem_obj->setUris("dummy_storeId", NULL, HttpRequestMethod());
+    logEntry->createMemObject("dummy_storeId", NULL, HttpRequestMethod());
     logEntry->store_status = STORE_PENDING;
     TestSwapDirPointer aStore (new TestSwapDir);
     TestSwapDirPointer aStore2 (new TestSwapDir);
@@ -73,8 +72,7 @@ testStoreController::testMaxSize()
 {
     commonInit();
     StoreEntry *logEntry = new StoreEntry;
-    logEntry->makeMemObject();
-    logEntry->mem_obj->setUris("dummy_storeId", NULL, HttpRequestMethod());
+    logEntry->createMemObject("dummy_storeId", NULL, HttpRequestMethod());
     logEntry->store_status = STORE_PENDING;
     Store::Init();
     TestSwapDirPointer aStore (new TestSwapDir);

diff --git a/src/tests/testStoreHashIndex.cc b/src/tests/testStoreHashIndex.cc
index 96ea8bdad94e698474f7f44bfa8f8b32acd70673..6c861d190e46fe82c6527cf2c88b99c676e8c946 100644 (file)
--- a/src/tests/testStoreHashIndex.cc
+++ b/src/tests/testStoreHashIndex.cc
@@ -30,8 +30,7 @@ void
 testStoreHashIndex::testStats()
 {
     StoreEntry *logEntry = new StoreEntry;
-    logEntry->makeMemObject();
-    logEntry->mem_obj->setUris("dummy_storeId", NULL, HttpRequestMethod());
+    logEntry->createMemObject("dummy_storeId", NULL, HttpRequestMethod());
     logEntry->store_status = STORE_PENDING;
     Store::Init();
     TestSwapDirPointer aStore (new TestSwapDir);
@@ -51,8 +50,7 @@ void
 testStoreHashIndex::testMaxSize()
 {
     StoreEntry *logEntry = new StoreEntry;
-    logEntry->makeMemObject();
-    logEntry->mem_obj->setUris("dummy_storeId", NULL, HttpRequestMethod());
+    logEntry->createMemObject("dummy_storeId", NULL, HttpRequestMethod());
     logEntry->store_status = STORE_PENDING;
     Store::Init();
     TestSwapDirPointer aStore (new TestSwapDir);