Prevent Rock Store map entries being locked for reading forever.

The map entries were locked to serve a hit but the client side carelessly
discarded the attached StoreEntry object when the hit had to be ignored.
Discarded StoreEntry object was left idle because it was never [un]locked.

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 2fcbf316af3cb18be904ac9d6362aa1d0aa72b4d..7995e5837e2cd0888af2169582dfff481619cff9 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -1547,6 +1547,23 @@ clientReplyContext::cloneReply()
     buildReplyHeader();
 }
 
+/// Safely disposes of an entry pointing to a cache hit that we do not want.
+/// We cannot just ignore the entry because it may be locking or otherwise
+/// holding an associated cache resource of some sort.
+void
+clientReplyContext::forgetHit()
+{
+    StoreEntry *e = http->storeEntry();
+    assert(e); // or we are not dealing with a hit
+    // We probably have not locked the entry earlier, unfortunately. We lock it
+    // now so that we can unlock two lines later (and trigger cleanup).
+    // Ideally, ClientHttpRequest::storeEntry() should lock/unlock, but it is
+    // used so inconsistently that simply adding locking there leads to bugs.
+    e->lock();
+    http->storeEntry(NULL);
+    e->unlock(); // may delete e (and release resources associated with it)
+}
+
 void
 clientReplyContext::identifyStoreObject()
 {
@@ -1624,7 +1641,7 @@ clientReplyContext::identifyFoundObject(StoreEntry *newEntry)
     if (http->redirect.status) {
         /** \li If redirection status is True force this to be a MISS */
         debugs(85, 3, HERE << "REDIRECT status forced StoreEntry to NULL (no body on 3XX responses)");
-        http->storeEntry(NULL);
+        forgetHit();
         http->logType = LOG_TCP_REDIRECT;
         doGetMoreData();
         return;
@@ -1632,7 +1649,7 @@ clientReplyContext::identifyFoundObject(StoreEntry *newEntry)
 
     if (!e->validToSend()) {
         debugs(85, 3, "clientProcessRequest2: !storeEntryValidToSend MISS" );
-        http->storeEntry(NULL);
+        forgetHit();
         http->logType = LOG_TCP_MISS;
         doGetMoreData();
         return;
@@ -1648,7 +1665,7 @@ clientReplyContext::identifyFoundObject(StoreEntry *newEntry)
 
     if (r->flags.noCache) {
         debugs(85, 3, "clientProcessRequest2: no-cache REFRESH MISS");
-        http->storeEntry(NULL);
+        forgetHit();
         http->logType = LOG_TCP_CLIENT_REFRESH_MISS;
         doGetMoreData();
         return;

diff --git a/src/client_side_reply.h b/src/client_side_reply.h
index 50ece1136938c53e0a7bd0590da34d41f323db30..a4b9286d4c0422d5f507ba84a2a2f0774aaa9cca 100644 (file)
--- a/src/client_side_reply.h
+++ b/src/client_side_reply.h
@@ -144,6 +144,7 @@ private:
     void triggerInitialStoreRead();
     void sendClientOldEntry();
     void purgeAllCached();
+    void forgetHit();
 
     void sendBodyTooLargeError();
     void sendPreconditionFailedError();