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 0435cd26ede694ac3e51904a1fc75612d0fac5c8..182384e104c4789b61604977275eeaeab1b1a87d 100644 (file)
--- a/src/MemObject.cc
+++ b/src/MemObject.cc
@@ -77,7 +77,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 4f958e8c749097317ff0d094c2e77b21b8d9496c..53a750b1561f8c6470f0f566ea183dcaf02bd37c 100644 (file)
--- a/src/MemObject.h
+++ b/src/MemObject.h
@@ -42,7 +42,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 86a86e7a048bc57d3d6b32ab1e43e5dbdb84bd93..016adc7858b5fec3414b61f510fa450368dffbb5 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 5bd311cc56a31a4cf35f3ca27dccd71f95fff380..09206f946e7e717593b14d928cd69ce00ee7fec5 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 5ae8aed556ebd680a634309064a9d7d4345ba877..d4c54e2069b36df6e0a49d2a15a934f8c59ec5ec 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);
@@ -1844,22 +1845,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 1da265b034bc548e70cef1b3a61de1d73f1aab6c..9dc6981d2866ffb9dde449c6f2d4f2a99b96a8fa 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);