[trac661] Touch or remove the entry in rrset cache properly when doing lookup or update.

src/lib/cache/rrset_cache.cc

@@ -42,45 +42,33 @@ RRsetCache::lookup(const isc::dns::Name& qname,
 {
     const string entry_name = genCacheEntryName(qname, qtype);
     RRsetEntryPtr entry_ptr = rrset_table_.get(HashKey(entry_name, RRClass(class_)));
-
-    //If the rrset entry has expired, return NULL.
-    if(entry_ptr && (time(NULL) > entry_ptr->getExpireTime())) {
-        return (RRsetEntryPtr());
+    if (entry_ptr) {
+        rrset_lru_.touch(entry_ptr);
     }
+
     return (entry_ptr);
 }
 
 RRsetEntryPtr
 RRsetCache::update(const isc::dns::RRset& rrset, const RRsetTrustLevel& level) {
     // TODO: If the RRset is an NS, we should update the NSAS as well
-    
     // lookup first
     RRsetEntryPtr entry_ptr = lookup(rrset.getName(), rrset.getType());
-    if(!entry_ptr) {
-        // rrset entry doesn't exist, create one rrset entry for the rrset
-        // and add it directly.
-        entry_ptr.reset(new RRsetEntry(rrset, level));
-        // Replace the expired rrset entry if it exists.
-        rrset_table_.add(entry_ptr, entry_ptr->hashKey(), true);
-        //TODO , lru list touch.
-        return (entry_ptr);
-    } else {
-        // there is one rrset entry in the cache, need to check whether
-        // the new rrset is more authoritative.
+    if (entry_ptr) {
         if (entry_ptr->getTrustLevel() > level) {
-            // existed rrset entry is more authoritative, do nothing,
-            // just return it.
-            //TODO, lru list touch
+            // existed rrset entry is more authoritative, touch and return it
+            rrset_lru_.touch(entry_ptr);
             return (entry_ptr);
         } else {
-            HashKey key = entry_ptr->hashKey();
-            entry_ptr.reset(new RRsetEntry(rrset, level));
-            //TODO, lru list touch.
-            // Replace the expired rrset entry if it exists.
-            rrset_table_.add(entry_ptr, entry_ptr->hashKey(), true);
-            return (entry_ptr);
+            // Remove the old rrset entry from the lru list.
+            rrset_lru_.remove(entry_ptr);
         }
     }
+
+    entry_ptr.reset(new RRsetEntry(rrset, level));
+    rrset_table_.add(entry_ptr, entry_ptr->hashKey(), true);
+    rrset_lru_.add(entry_ptr);
+    return (entry_ptr);
 }
 
 #if 0

src/lib/cache/tests/message_cache_unittest.cc

@@ -70,8 +70,8 @@ public:
     {
         uint16_t class_ = RRClass::IN().getCode();
         rrset_cache_.reset(new DerivedRRsetCache(RRSET_CACHE_DEFAULT_SIZE, class_));
-        message_cache_.reset(new DerivedMessageCache(rrset_cache_,
-                                          MESSAGE_CACHE_DEFAULT_SIZE, class_ ));
+        // Set the message cache size to 1, make it easy for unittest.
+        message_cache_.reset(new DerivedMessageCache(rrset_cache_, 1, class_ ));
     }
 
 protected:
@@ -118,5 +118,31 @@ TEST_F(MessageCacheTest, testUpdate) {
     EXPECT_TRUE(new_msg_render.getHeaderFlag(Message::HEADERFLAG_AA));
 }
 
+void
+updateMessageCache(const char* message_file,
+                   boost::shared_ptr<DerivedMessageCache> cache)
+{
+    Message msg(Message::PARSE);
+    messageFromFile(msg, message_file);
+    cache->update(msg);
+}
+
+TEST_F(MessageCacheTest, testCacheLruBehavior) {
+    // qname = "test.example.com.", qtype = A
+    updateMessageCache("message_fromWire1", message_cache_);
+    // qname = "test.example.net.", qtype = A
+    updateMessageCache("message_fromWire2", message_cache_);
+    // qname = "example.com.", qtype = SOA
+    updateMessageCache("message_fromWire4", message_cache_);
+
+    Name qname_net("test.example.net.");
+    EXPECT_TRUE(message_cache_->lookup(qname_net, RRType::A(), message_render));
+
+    // qname = "a.example.com.", qtype = A
+    updateMessageCache("message_fromWire5", message_cache_);
+    Name qname_com("test.example.com.");
+    EXPECT_FALSE(message_cache_->lookup(qname_com, RRType::A(), message_render));
+}
+
 }   // namespace
 

src/lib/cache/tests/rrset_cache_unittest.cc

@@ -34,50 +34,87 @@ namespace {
 class RRsetCacheTest : public testing::Test {
 protected:
     RRsetCacheTest():
-        cache(RRSET_CACHE_DEFAULT_SIZE, RRClass::IN().getCode()),
-        name("example.com"),
-        rrset1(name, RRClass::IN(), RRType::A(), RRTTL(20)),
-        rrset2(name, RRClass::IN(), RRType::A(), RRTTL(10)),
-        rrset_entry1(rrset1, RRSET_TRUST_ADDITIONAL_AA),
-        rrset_entry2(rrset2, RRSET_TRUST_PRIM_ZONE_NONGLUE)
+        cache_(1, RRClass::IN().getCode()),
+        name_("example.com"),
+        rrset1_(name_, RRClass::IN(), RRType::A(), RRTTL(20)),
+        rrset2_(name_, RRClass::IN(), RRType::A(), RRTTL(10)),
+        rrset_entry1_(rrset1_, RRSET_TRUST_ADDITIONAL_AA),
+        rrset_entry2_(rrset2_, RRSET_TRUST_PRIM_ZONE_NONGLUE)
     {
     }
 
-    RRsetCache cache;
-    Name name;
-    RRset rrset1;
-    RRset rrset2;
-    RRsetEntry rrset_entry1;
-    RRsetEntry rrset_entry2;
+    RRsetCache cache_;
+    Name name_;
+    RRset rrset1_;
+    RRset rrset2_;
+    RRsetEntry rrset_entry1_;
+    RRsetEntry rrset_entry2_;
 };
 
 TEST_F(RRsetCacheTest, lookup) {
     const RRType& type = RRType::A();
-    EXPECT_TRUE(cache.lookup(name, type) == NULL);
-
-    cache.update(rrset1, rrset_entry1.getTrustLevel());
-    RRsetEntryPtr rrset_entry_ptr = cache.lookup(name, type);
-    EXPECT_EQ(rrset_entry_ptr->getTrustLevel(), rrset_entry1.getTrustLevel());
-    EXPECT_EQ(rrset_entry_ptr->getRRset()->getName(), rrset_entry1.getRRset()->getName());
-    EXPECT_EQ(rrset_entry_ptr->getRRset()->getType(), rrset_entry1.getRRset()->getType());
-    EXPECT_EQ(rrset_entry_ptr->getRRset()->getClass(), rrset_entry1.getRRset()->getClass());
+    EXPECT_TRUE(cache_.lookup(name_, type) == NULL);
+
+    cache_.update(rrset1_, rrset_entry1_.getTrustLevel());
+    RRsetEntryPtr rrset_entry_ptr = cache_.lookup(name_, type);
+    EXPECT_EQ(rrset_entry_ptr->getTrustLevel(), rrset_entry1_.getTrustLevel());
+    EXPECT_EQ(rrset_entry_ptr->getRRset()->getName(), rrset_entry1_.getRRset()->getName());
+    EXPECT_EQ(rrset_entry_ptr->getRRset()->getType(), rrset_entry1_.getRRset()->getType());
+    EXPECT_EQ(rrset_entry_ptr->getRRset()->getClass(), rrset_entry1_.getRRset()->getClass());
 }
 
 TEST_F(RRsetCacheTest, update) {
     const RRType& type = RRType::A();
 
-    cache.update(rrset1, rrset_entry1.getTrustLevel());
-    RRsetEntryPtr rrset_entry_ptr = cache.lookup(name, type);
-    EXPECT_EQ(rrset_entry_ptr->getTrustLevel(), rrset_entry1.getTrustLevel());
+    cache_.update(rrset1_, rrset_entry1_.getTrustLevel());
+    RRsetEntryPtr rrset_entry_ptr = cache_.lookup(name_, type);
+    EXPECT_EQ(rrset_entry_ptr->getTrustLevel(), rrset_entry1_.getTrustLevel());
 
-    cache.update(rrset2, rrset_entry2.getTrustLevel());
-    rrset_entry_ptr = cache.lookup(name, type);
+    cache_.update(rrset2_, rrset_entry2_.getTrustLevel());
+    rrset_entry_ptr = cache_.lookup(name_, type);
     // The trust level should be updated
-    EXPECT_EQ(rrset_entry_ptr->getTrustLevel(), rrset_entry2.getTrustLevel());
+    EXPECT_EQ(rrset_entry_ptr->getTrustLevel(), rrset_entry2_.getTrustLevel());
 
-    cache.update(rrset1, rrset_entry1.getTrustLevel());
+    cache_.update(rrset1_, rrset_entry1_.getTrustLevel());
     // The trust level should not be updated
-    EXPECT_EQ(rrset_entry_ptr->getTrustLevel(), rrset_entry2.getTrustLevel());
+    EXPECT_EQ(rrset_entry_ptr->getTrustLevel(), rrset_entry2_.getTrustLevel());
+}
+
+void
+updateRRsetCache(RRsetCache& cache, Name& rrset_name,
+                 RRsetTrustLevel level=RRSET_TRUST_ADDITIONAL_AA)
+{
+    RRset rrset(rrset_name, RRClass::IN(), RRType::A(), RRTTL(20));
+    cache.update(rrset, level);
+}
+
+// Test whether the lru list in rrset cache works as expected.
+TEST_F(RRsetCacheTest, cacheLruBehavior) {
+    Name name1("1.example.com.");
+    Name name2("2.example.com.");
+    Name name3("3.example.com.");
+    Name name4("4.example.com.");
+
+    updateRRsetCache(cache_, name1);
+    updateRRsetCache(cache_, name2);
+    updateRRsetCache(cache_, name3);
+
+    EXPECT_TRUE(cache_.lookup(name1, RRType::A()));
+
+    // Now update the fourth rrset, rrset with name "2.example.com."
+    // should has been removed from cache.
+    updateRRsetCache(cache_, name4);
+    EXPECT_FALSE(cache_.lookup(name2, RRType::A()));
+
+    // Test Update rrset with higher trust level
+    updateRRsetCache(cache_, name1, RRSET_TRUST_PRIM_GLUE);
+    // Test update rrset with lower trust level.
+    updateRRsetCache(cache_, name3, RRSET_TRUST_ADDITIONAL_NONAA);
+
+    // When add rrset with name2, rrset with name4
+    // has been removed from the cache.
+    updateRRsetCache(cache_, name2);
+    EXPECT_FALSE(cache_.lookup(name4, RRType::A()));
 }
 
 }