[trac641] First part of tackling memory leaks

In the NSAS, store a pointer to the resolver as a "raw" pointer,
not a shared pointer.  The NSAS is part of the resolver, although
it can call back into the resolver.  If both store a shared pointer
to each other we can have the case where the reference counts can
never drop to zero.

src/lib/nsas/nameserver_address_store.cc

@@ -53,7 +53,7 @@ NameserverAddressStore::NameserverAddressStore(
         new HashDeleter<ZoneEntry>(*zone_hash_))),
     nameserver_lru_(new LruList<NameserverEntry>((3 * nshashsize),
         new HashDeleter<NameserverEntry>(*nameserver_hash_))),
-    resolver_(resolver)
+    resolver_(resolver.get())
 { }
 
 namespace {
@@ -66,7 +66,7 @@ namespace {
  */
 boost::shared_ptr<ZoneEntry>
 newZone(
-    const boost::shared_ptr<isc::resolve::ResolverInterface>* resolver,
+    isc::resolve::ResolverInterface** const resolver,
     const string* zone, const RRClass* class_code,
     const boost::shared_ptr<HashTable<NameserverEntry> >* ns_hash,
     const boost::shared_ptr<LruList<NameserverEntry> >* ns_lru)

src/lib/nsas/nameserver_address_store.h

@@ -115,7 +115,7 @@ protected:
     boost::shared_ptr<LruList<NameserverEntry> > nameserver_lru_;
     // The resolver we use
 private:
-    boost::shared_ptr<isc::resolve::ResolverInterface> resolver_;
+    isc::resolve::ResolverInterface* resolver_;
     //}@
 };
 

src/lib/nsas/nameserver_entry.cc

@@ -380,8 +380,7 @@ class NameserverEntry::ResolverCallback :
 };
 
 void
-NameserverEntry::askIP(
-    boost::shared_ptr<isc::resolve::ResolverInterface> resolver,
+NameserverEntry::askIP(isc::resolve::ResolverInterface* resolver,
     const RRType& type, AddressFamily family)
 {
     QuestionPtr question(new Question(Name(getName()), RRClass(getClass()),
@@ -392,8 +391,7 @@ NameserverEntry::askIP(
 }
 
 void
-NameserverEntry::askIP(
-    boost::shared_ptr<isc::resolve::ResolverInterface> resolver,
+NameserverEntry::askIP(isc::resolve::ResolverInterface* resolver,
     boost::shared_ptr<Callback> callback, AddressFamily family)
 {
     Lock lock(mutex_);

src/lib/nsas/nameserver_entry.h

@@ -241,7 +241,7 @@ public:
      *     even when there are addresses, if there are no addresses for this
      *     family.
      */
-    void askIP(boost::shared_ptr<isc::resolve::ResolverInterface> resolver,
+    void askIP(isc::resolve::ResolverInterface* resolver,
         boost::shared_ptr<Callback> callback, AddressFamily family);
     //@}
 
@@ -273,7 +273,7 @@ private:
     /// \short Private version that does the actual asking of one address type
     ///
     /// Call unlocked.
-    void askIP(boost::shared_ptr<isc::resolve::ResolverInterface> resolver,
+    void askIP(isc::resolve::ResolverInterface* resolver,
         const isc::dns::RRType&, AddressFamily);
 };
 

src/lib/nsas/tests/nameserver_address_store_unittest.cc

@@ -131,7 +131,7 @@ protected:
         for (int i = 1; i <= 9; ++i) {
             std::string name = "zone" + boost::lexical_cast<std::string>(i);
             zones_.push_back(boost::shared_ptr<ZoneEntry>(new ZoneEntry(
-                resolver_, name, RRClass(40 + i),
+                resolver_.get(), name, RRClass(40 + i),
                 boost::shared_ptr<HashTable<NameserverEntry> >(),
                 boost::shared_ptr<LruList<NameserverEntry> >())));
         }

src/lib/nsas/tests/nameserver_address_unittest.cc

@@ -39,7 +39,9 @@ class NameserverEntrySample {
 public:
     NameserverEntrySample():
         name_("example.org"),
-        rrv4_(new RRset(name_, RRClass::IN(), RRType::A(), RRTTL(1200)))
+        rrv4_(new RRset(name_, RRClass::IN(), RRType::A(), RRTTL(1200))),
+        ns_(new NameserverEntry(name_.toText(), RRClass::IN())),
+        resolver_(new TestResolver())
     {
         // Add some sample A records
         rrv4_->addRdata(ConstRdataPtr(new RdataTest<A>("1.2.3.4")));
@@ -47,10 +49,9 @@ public:
         rrv4_->addRdata(ConstRdataPtr(new RdataTest<A>("9.10.11.12")));
 
         ns_.reset(new NameserverEntry(name_.toText(), RRClass::IN()));
-        boost::shared_ptr<TestResolver> resolver(new TestResolver);
-        ns_->askIP(resolver, boost::shared_ptr<Callback>(new Callback), ANY_OK);
-        resolver->asksIPs(name_, 0, 1);
-        resolver->requests[0].second->success(createResponseMessage(rrv4_));
+        ns_->askIP(resolver_.get(), boost::shared_ptr<Callback>(new Callback), ANY_OK);
+        resolver_->asksIPs(name_, 0, 1);
+        resolver_->requests[0].second->success(createResponseMessage(rrv4_));
     }
 
     // Return the sample NameserverEntry
@@ -75,6 +76,7 @@ private:
     Name name_;                             ///< Name of the sample
     RRsetPtr rrv4_;           ///< Standard RRSet - IN, A, lowercase name
     boost::shared_ptr<NameserverEntry> ns_; ///< Shared_ptr that points to a NameserverEntry object
+    boost::shared_ptr<TestResolver> resolver_;
 
     class Callback : public NameserverEntry::Callback {
         public:

src/lib/nsas/tests/nameserver_entry_unittest.cc

@@ -86,7 +86,7 @@ protected:
         boost::shared_ptr<TestResolver> resolver(new TestResolver);
         boost::shared_ptr<Callback> callback(new Callback);
         // Let it ask for data
-        entry->askIP(resolver, callback, ANY_OK);
+        entry->askIP(resolver.get(), callback, ANY_OK);
         // Check it really asked and sort the queries
         EXPECT_TRUE(resolver->asksIPs(Name(entry->getName()), 0, 1));
         // Respond with answers
@@ -266,7 +266,7 @@ TEST_F(NameserverEntryTest, IPCallbacks) {
     boost::shared_ptr<Callback> callback(new Callback);
     boost::shared_ptr<TestResolver> resolver(new TestResolver);
 
-    entry->askIP(resolver, callback, ANY_OK);
+    entry->askIP(resolver.get(), callback, ANY_OK);
     // Ensure it becomes IN_PROGRESS
     EXPECT_EQ(Fetchable::IN_PROGRESS, entry->getState());
     // Now, there should be two queries in the resolver
@@ -274,12 +274,12 @@ TEST_F(NameserverEntryTest, IPCallbacks) {
     ASSERT_TRUE(resolver->asksIPs(Name(EXAMPLE_CO_UK), 0, 1));
 
     // Another one might ask
-    entry->askIP(resolver, callback, V4_ONLY);
+    entry->askIP(resolver.get(), callback, V4_ONLY);
     // There should still be only two queries in the resolver
     ASSERT_EQ(2, resolver->requests.size());
 
     // Another one, with need of IPv6 address
-    entry->askIP(resolver, callback, V6_ONLY);
+    entry->askIP(resolver.get(), callback, V6_ONLY);
 
     // Answer one and see that the callbacks are called
     resolver->answer(0, Name(EXAMPLE_CO_UK), RRType::A(),
@@ -316,7 +316,7 @@ TEST_F(NameserverEntryTest, IPCallbacksUnreachable) {
     boost::shared_ptr<TestResolver> resolver(new TestResolver);
 
     // Ask for its IP
-    entry->askIP(resolver, callback, ANY_OK);
+    entry->askIP(resolver.get(), callback, ANY_OK);
     // Check it asks the resolver
     EXPECT_EQ(2, resolver->requests.size());
     ASSERT_TRUE(resolver->asksIPs(Name(EXAMPLE_CO_UK), 0, 1));
@@ -352,7 +352,7 @@ TEST_F(NameserverEntryTest, DirectAnswer) {
         RRType::AAAA()), RRsetPtr());
 
     // A successfull test first
-    entry->askIP(resolver, callback, ANY_OK);
+    entry->askIP(resolver.get(), callback, ANY_OK);
     EXPECT_EQ(0, resolver->requests.size());
     EXPECT_EQ(1, callback->count);
     NameserverEntry::AddressVector addresses;
@@ -362,7 +362,7 @@ TEST_F(NameserverEntryTest, DirectAnswer) {
     // An unsuccessfull test
     callback->count = 0;
     entry.reset(new NameserverEntry(EXAMPLE_NET, RRClass::IN()));
-    entry->askIP(resolver, callback, ANY_OK);
+    entry->askIP(resolver.get(), callback, ANY_OK);
     EXPECT_EQ(0, resolver->requests.size());
     EXPECT_EQ(1, callback->count);
     addresses.clear();
@@ -381,8 +381,8 @@ TEST_F(NameserverEntryTest, ChangedExpired) {
     boost::shared_ptr<TestResolver> resolver(new TestResolver);
 
     // Ask the first time
-    entry->askIP(resolver, callback, V4_ONLY);
-    entry->askIP(resolver, callback, V6_ONLY);
+    entry->askIP(resolver.get(), callback, V4_ONLY);
+    entry->askIP(resolver.get(), callback, V6_ONLY);
     EXPECT_TRUE(resolver->asksIPs(Name(EXAMPLE_CO_UK), 0, 1));
     EXPECT_EQ(Fetchable::IN_PROGRESS, entry->getState());
     resolver->answer(0, Name(EXAMPLE_CO_UK), RRType::A(),
@@ -402,8 +402,8 @@ TEST_F(NameserverEntryTest, ChangedExpired) {
 
     // Ask the second time. The callbacks should not fire right away and it
     // should request the addresses again
-    entry->askIP(resolver, callback, V4_ONLY);
-    entry->askIP(resolver, callback, V6_ONLY);
+    entry->askIP(resolver.get(), callback, V4_ONLY);
+    entry->askIP(resolver.get(), callback, V6_ONLY);
     EXPECT_EQ(2, callback->count);
     EXPECT_TRUE(resolver->asksIPs(Name(EXAMPLE_CO_UK), 2, 3));
     EXPECT_EQ(Fetchable::IN_PROGRESS, entry->getState());
@@ -431,8 +431,8 @@ TEST_F(NameserverEntryTest, KeepRTT) {
     boost::shared_ptr<TestResolver> resolver(new TestResolver);
 
     // Ask the first time
-    entry->askIP(resolver, callback, V4_ONLY);
-    entry->askIP(resolver, callback, V6_ONLY);
+    entry->askIP(resolver.get(), callback, V4_ONLY);
+    entry->askIP(resolver.get(), callback, V6_ONLY);
     EXPECT_TRUE(resolver->asksIPs(Name(EXAMPLE_CO_UK), 0, 1));
     EXPECT_EQ(Fetchable::IN_PROGRESS, entry->getState());
     resolver->answer(0, Name(EXAMPLE_CO_UK), RRType::A(),
@@ -455,8 +455,8 @@ TEST_F(NameserverEntryTest, KeepRTT) {
 
     // Ask the second time. The callbacks should not fire right away and it
     // should request the addresses again
-    entry->askIP(resolver, callback, V4_ONLY);
-    entry->askIP(resolver, callback, V6_ONLY);
+    entry->askIP(resolver.get(), callback, V4_ONLY);
+    entry->askIP(resolver.get(), callback, V6_ONLY);
     EXPECT_EQ(2, callback->count);
     EXPECT_TRUE(resolver->asksIPs(Name(EXAMPLE_CO_UK), 2, 3));
     EXPECT_EQ(Fetchable::IN_PROGRESS, entry->getState());

src/lib/nsas/tests/nsas_test.h

@@ -222,11 +222,6 @@ private:
 
 static const uint32_t HASHTABLE_DEFAULT_SIZE = 1009; ///< First prime above 1000
 
-} // namespace nsas
-} // namespace isc
-
-namespace {
-
 using namespace std;
 
 /*
@@ -420,6 +415,7 @@ protected:
     Name ns_name_;  ///< Nameserver name of ns.example.net
 };
 
-} // Empty namespace
+} // namespace nsas
+} // namespace isc
 
 #endif // __NSAS_TEST_H

src/lib/nsas/tests/zone_entry_unittest.cc

@@ -47,7 +47,7 @@ class InheritedZoneEntry : public ZoneEntry {
             const std::string& name, const RRClass& class_code,
             boost::shared_ptr<HashTable<NameserverEntry> > nameserver_table,
             boost::shared_ptr<LruList<NameserverEntry> > nameserver_lru) :
-            ZoneEntry(resolver, name, class_code, nameserver_table,
+            ZoneEntry(resolver.get(), name, class_code, nameserver_table,
                 nameserver_lru)
         { }
         NameserverVector& nameservers() { return nameservers_; }
@@ -569,7 +569,7 @@ TEST_F(ZoneEntryTest, NameserverEntryReady) {
     // Inject the entry
     boost::shared_ptr<NameserverEntry> nse(injectEntry());
     // Fill it with data
-    nse->askIP(resolver_, nseCallback(), ANY_OK);
+    nse->askIP(resolver_.get(), nseCallback(), ANY_OK);
     EXPECT_EQ(Fetchable::IN_PROGRESS, nse->getState());
     EXPECT_TRUE(resolver_->asksIPs(ns_name_, 0, 1));
     EXPECT_NO_THROW(resolver_->answer(0, ns_name_, RRType::A(),
@@ -594,7 +594,7 @@ TEST_F(ZoneEntryTest, NameserverEntryNotAsked) {
 TEST_F(ZoneEntryTest, NameserverEntryInProgress) {
     // Prepare the nameserver entry
     boost::shared_ptr<NameserverEntry> nse(injectEntry());
-    nse->askIP(resolver_, nseCallback(), ANY_OK);
+    nse->askIP(resolver_.get(), nseCallback(), ANY_OK);
     EXPECT_EQ(Fetchable::IN_PROGRESS, nse->getState());
     EXPECT_TRUE(resolver_->asksIPs(ns_name_, 0, 1));
 
@@ -604,7 +604,7 @@ TEST_F(ZoneEntryTest, NameserverEntryInProgress) {
 /// \short Check Zone's reaction to found expired nameserver
 TEST_F(ZoneEntryTest, NameserverEntryExpired) {
     boost::shared_ptr<NameserverEntry> nse(injectEntry());
-    nse->askIP(resolver_, nseCallback(), ANY_OK);
+    nse->askIP(resolver_.get(), nseCallback(), ANY_OK);
     EXPECT_EQ(Fetchable::IN_PROGRESS, nse->getState());
     EXPECT_TRUE(resolver_->asksIPs(ns_name_, 0, 1));
     EXPECT_NO_THROW(resolver_->answer(0, ns_name_, RRType::A(),
@@ -623,7 +623,7 @@ TEST_F(ZoneEntryTest, NameserverEntryExpired) {
 /// \short Check how it reacts to an unreachable zone already in the table
 TEST_F(ZoneEntryTest, NameserverEntryUnreachable) {
     boost::shared_ptr<NameserverEntry> nse(injectEntry());
-    nse->askIP(resolver_, nseCallback(), ANY_OK);
+    nse->askIP(resolver_.get(), nseCallback(), ANY_OK);
     ASSERT_EQ(2, resolver_->requests.size());
     resolver_->requests[0].second->failure();
     resolver_->requests[1].second->failure();

src/lib/nsas/zone_entry.cc

@@ -36,7 +36,7 @@ using namespace dns;
 namespace nsas {
 
 ZoneEntry::ZoneEntry(
-    boost::shared_ptr<isc::resolve::ResolverInterface> resolver,
+    isc::resolve::ResolverInterface* resolver,
     const std::string& name, const isc::dns::RRClass& class_code,
     boost::shared_ptr<HashTable<NameserverEntry> > nameserver_table,
     boost::shared_ptr<LruList<NameserverEntry> > nameserver_lru) :

src/lib/nsas/zone_entry.h

@@ -68,8 +68,7 @@ public:
      * \todo Move to cc file, include the lookup (if NSAS uses resolver for
      *     everything)
      */
-    ZoneEntry(
-        boost::shared_ptr<isc::resolve::ResolverInterface> resolver,
+    ZoneEntry(isc::resolve::ResolverInterface* resolver,
         const std::string& name, const isc::dns::RRClass& class_code,
         boost::shared_ptr<HashTable<NameserverEntry> > nameserver_table,
         boost::shared_ptr<LruList<NameserverEntry> > nameserver_lru);
@@ -153,7 +152,7 @@ private:
     void process(AddressFamily family,
         const boost::shared_ptr<NameserverEntry>& nameserver);
     // Resolver we use
-    boost::shared_ptr<isc::resolve::ResolverInterface> resolver_;
+    isc::resolve::ResolverInterface* resolver_;
     // We store the nameserver table and lru, so we can look up when there's
     // update
     boost::shared_ptr<HashTable<NameserverEntry> > nameserver_table_;