Ensure StoreEntry presence during store_client lifetime (#1281)

Most likely, all store_client users already lock their StoreEntry
objects, so this change is unlikely to affect any current use cases.
However, objects like store_client that store StoreEntry objects for
long-term/asynchronous reuse should lock their entries rather than rely
on others to do that for them. Being able to rely on StoreEntry locking
also clarifies existing logic in several store_client methods.

diff --git a/src/StoreClient.h b/src/StoreClient.h
index 37f7d04a93a0238b5d38ae346c96f203994ee194..47106617d856b6649a8176f472606566762cdfdc 100644 (file)
--- a/src/StoreClient.h
+++ b/src/StoreClient.h
@@ -58,7 +58,7 @@ class store_client
     CBDATA_CLASS(store_client);
 
 public:
-    store_client(StoreEntry *);
+    explicit store_client(StoreEntry *);
     ~store_client();
 
     /// An offset into the stored response bytes, including the HTTP response

diff --git a/src/store_client.cc b/src/store_client.cc
index 1262685fd45def22e81b9407a243c484b73984ce..c6486dae1e8677049d454f9533f0020b49e2f946 100644 (file)
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -216,6 +216,9 @@ store_client::store_client(StoreEntry *e) :
     object_ok(true),
     copiedSize(0)
 {
+    Assure(entry);
+    entry->lock("store_client");
+
     flags.disk_io_pending = false;
     flags.store_copying = false;
     ++ entry->refcount;
@@ -228,7 +231,10 @@ store_client::store_client(StoreEntry *e) :
 }
 
 store_client::~store_client()
-{}
+{
+    assert(entry);
+    entry->unlock("store_client");
+}
 
 /* copy bytes requested by the client */
 void
@@ -527,7 +533,7 @@ store_client::readBody(const char *, ssize_t len)
 
     if (len > 0 && rep && entry->mem_obj->inmem_lo == 0 && entry->objectLen() <= (int64_t)Config.Store.maxInMemObjSize && Config.onoff.memory_cache_disk) {
         storeGetMemSpace(len);
-        // The above may start to free our object so we need to check again
+        // recheck for the above call may purge entry's data from the memory cache
         if (entry->mem_obj->inmem_lo == 0) {
             /* Copy read data back into memory.
              * copyInto.offset includes headers, which is what mem cache needs
@@ -717,15 +723,14 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data)
 
 #endif
 
+    // We must lock to safely dereference e below, after deleting sc and after
+    // calling CheckQuickAbortIsReasonable().
+    e->lock("storeUnregister");
+
     // XXX: We might be inside sc store_client method somewhere up the call
     // stack. TODO: Convert store_client to AsyncJob to make destruction async.
     delete sc;
 
-    assert(e->locked());
-    // An entry locked by others may be unlocked (and destructed) by others, so
-    // we must lock again to safely dereference e after CheckQuickAbortIsReasonable().
-    e->lock("storeUnregister");
-
     if (CheckQuickAbortIsReasonable(e))
         e->abort();
     else