From: bert hubert <bert.hubert@netherlabs.nl>
Date: Sun, 7 Dec 2014 20:30:22 +0000 (+0100)
Subject: make sure we lock the cache shards while we clean them, closing #1910. Plus add regre... 
X-Git-Tag: rec-3.7.0-rc1~144
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=20ac38f9dad041149ca6727b981c3d5c244bbfc5;p=pdns

make sure we lock the cache shards while we clean them, closing #1910. Plus add regression test that pretty reliably
detects us not locking.
---

diff --git a/pdns/packetcache.cc b/pdns/packetcache.cc
index 8347c4be1..5e35fc7ab 100644
--- a/pdns/packetcache.cc
+++ b/pdns/packetcache.cc
@@ -337,7 +337,6 @@ int PacketCache::size()
 /** readlock for figuring out which iterators to delete, upgrade to writelock when actually cleaning */
 void PacketCache::cleanup()
 {
-
   *d_statnumentries=AtomicCounter(0);
   BOOST_FOREACH(MapCombo& mc, d_maps) {
     ReadLock l(&mc.d_mut);
@@ -364,7 +363,9 @@ void PacketCache::cleanup()
   //  cerr<<"cacheSize: "<<cacheSize<<", lookAt: "<<lookAt<<", toTrim: "<<toTrim<<endl;
   time_t now=time(0);
   DLOG(L<<"Starting cache clean"<<endl);
+  //unsigned int totErased=0;
   BOOST_FOREACH(MapCombo& mc, d_maps) {
+    WriteLock wl(&mc.d_mut);
     typedef cmap_t::nth_index<1>::type sequence_t;
     sequence_t& sidx=mc.d_map.get<1>();
     unsigned int erased=0, lookedAt=0;
@@ -373,8 +374,9 @@ void PacketCache::cleanup()
 	sidx.erase(i++);
 	erased++;
       }
-      else
+      else {
 	++i;
+      }
 
       if(toTrim && erased > toTrim / d_maps.size())
 	break;
@@ -382,9 +384,11 @@ void PacketCache::cleanup()
       if(lookedAt > lookAt / d_maps.size())
 	break;
     }
+    //totErased += erased;
   }
-  //  cerr<<"erased: "<<erased<<endl;
-
+  //  if(totErased)
+  //  cerr<<"erased: "<<totErased<<endl;
+  
   *d_statnumentries=AtomicCounter(0);
   BOOST_FOREACH(MapCombo& mc, d_maps) {
     ReadLock l(&mc.d_mut);
diff --git a/pdns/test-packetcache_cc.cc b/pdns/test-packetcache_cc.cc
index dbb65d064..80075566d 100644
--- a/pdns/test-packetcache_cc.cc
+++ b/pdns/test-packetcache_cc.cc
@@ -101,6 +101,7 @@ catch(PDNSException& e) {
 }
 
 
+
 BOOST_AUTO_TEST_CASE(test_PacketCacheThreaded) {
   try {
     PacketCache PC;
@@ -120,9 +121,10 @@ BOOST_AUTO_TEST_CASE(test_PacketCacheThreaded) {
     for(int i=0; i < 4 ; ++i)
       pthread_join(tid[i], &res);
 
-    BOOST_CHECK_EQUAL(S.read("deferred-cache-inserts"), g_missing);
-    BOOST_CHECK_EQUAL(S.read("deferred-cache-lookup"), 0);
+    BOOST_CHECK(S.read("deferred-cache-inserts") + S.read("deferred-cache-lookup") >= g_missing);
+    //    BOOST_CHECK_EQUAL(S.read("deferred-cache-lookup"), 0); // cache cleaning invalidates this
 
+  
   }
   catch(PDNSException& e) {
     cerr<<"Had error: "<<e.reason<<endl;
@@ -131,5 +133,53 @@ BOOST_AUTO_TEST_CASE(test_PacketCacheThreaded) {
   
 }
 
+bool g_stopCleaning;
+static void *cacheCleaner(void*)
+try
+{
+  while(!g_stopCleaning) {
+    g_PC->cleanup();
+  }
+
+  return 0;
+}
+catch(PDNSException& e) {
+  cerr<<"Had error in threadReader: "<<e.reason<<endl;
+  throw;
+}
+
+BOOST_AUTO_TEST_CASE(test_PacketCacheClean) {
+  try {
+    PacketCache PC;
+
+    for(unsigned int counter = 0; counter < 1000000; ++counter) {
+      PC.insert("hello "+boost::lexical_cast<string>(counter), QType(QType::A), PacketCache::QUERYCACHE, "something", 1, 1);
+    }
+
+    sleep(1);
+    
+    g_PC=&PC;
+    pthread_t tid[4];
+
+    ::arg().set("max-cache-entries")="10000";
+
+    pthread_create(&tid[0], 0, threadReader, (void*)(0*1000000UL));
+    pthread_create(&tid[1], 0, threadReader, (void*)(1*1000000UL));
+    pthread_create(&tid[2], 0, threadReader, (void*)(2*1000000UL));
+    //    pthread_create(&tid[2], 0, threadMangler, (void*)(0*1000000UL));
+    pthread_create(&tid[3], 0, cacheCleaner, 0);
+
+    void *res;
+    for(int i=0; i < 3 ; ++i)
+      pthread_join(tid[i], &res);
+    g_stopCleaning=true;
+    pthread_join(tid[3], &res);
+  }
+  catch(PDNSException& e) {
+    cerr<<"Had error in threadReader: "<<e.reason<<endl;
+    throw;
+  }
+}
+  
 
 BOOST_AUTO_TEST_SUITE_END()