Update the nameserver address store

And its tests

Fix one bug in zone entry on the way

git-svn-id: svn://bind10.isc.org/svn/bind10/branches/trac408@3720 e5f2f494-b856-4b98-b285-d166d9295462

src/lib/nsas/Makefile.am

@@ -14,7 +14,7 @@ libnsas_la_SOURCES += hash_deleter.h
 libnsas_la_SOURCES += hash_key.cc hash_key.h
 libnsas_la_SOURCES += hash_table.h
 libnsas_la_SOURCES += lru_list.h
-#libnsas_la_SOURCES += nameserver_address_store.cc nameserver_address_store.h
+libnsas_la_SOURCES += nameserver_address_store.cc nameserver_address_store.h
 libnsas_la_SOURCES += nameserver_address.h
 libnsas_la_SOURCES += nameserver_entry.cc nameserver_entry.h
 libnsas_la_SOURCES += nsas_entry_compare.h

src/lib/nsas/TODO

@@ -1,8 +1,9 @@
 NameserverEntry:
 * Reuse data on timeout/requery
 
-The NSAS itself:
-* Implement/recreate
+Global:
+* There are TODO notes in tests, some more tests should be added to stress
+  and test it more.
 
 Long term:
 * Make a mechanism the cache (which does not exist at the time of writing this

src/lib/nsas/nameserver_address_store.cc

@@ -21,11 +21,14 @@
 #include <config.h>
 #include <dns/rdataclass.h>
 
+#include "hash_table.h"
+#include "lru_list.h"
 #include "hash_deleter.h"
 #include "nsas_entry_compare.h"
 #include "nameserver_entry.h"
 #include "nameserver_address_store.h"
 #include "zone_entry.h"
+#include "address_request_callback.h"
 
 using namespace isc::dns;
 using namespace std;
@@ -39,183 +42,54 @@ namespace nsas {
 // The LRU lists are set equal to three times the size of the respective
 // hash table, on the assumption that three elements is the longest linear
 // search we want to do when looking up names in the hash table.
-NameserverAddressStore::NameserverAddressStore(ResolverInterface& resolver,
-    uint32_t zonehashsize, uint32_t nshashsize) :
-    zone_hash_(new NsasEntryCompare<ZoneEntry>, zonehashsize),
-    nameserver_hash_(new NsasEntryCompare<NameserverEntry>, nshashsize),
-    zone_lru_((3 * zonehashsize), new HashDeleter<ZoneEntry>(zone_hash_)),
-    nameserver_lru_((3 * nshashsize), new HashDeleter<NameserverEntry>(
-        nameserver_hash_)),
-    resolver_(resolver),
-    callback_(*this)
-{
-}
+NameserverAddressStore::NameserverAddressStore(
+    shared_ptr<ResolverInterface> resolver, uint32_t zonehashsize,
+    uint32_t nshashsize) :
+    zone_hash_(new HashTable<ZoneEntry>(new NsasEntryCompare<ZoneEntry>,
+        zonehashsize)),
+    nameserver_hash_(new HashTable<NameserverEntry>(
+        new NsasEntryCompare<NameserverEntry>, nshashsize)),
+    zone_lru_(new LruList<ZoneEntry>((3 * zonehashsize),
+        new HashDeleter<ZoneEntry>(*zone_hash_))),
+    nameserver_lru_(new LruList<NameserverEntry>((3 * nshashsize),
+        new HashDeleter<NameserverEntry>(*nameserver_hash_))),
+    resolver_(resolver)
+{ }
 
 namespace {
 
-// Often used types
-typedef shared_ptr<ZoneEntry> ZonePtr;
-typedef shared_ptr<NameserverEntry> NameserverPtr;
-typedef shared_ptr<AddressRequestCallback> CallbackPtr;
-
 /*
- * Create a zone entry.
- * It is called inside the mutex so it is called and filled in attomically.
- * Pointers are used instead of references, because with references,
- * boost::bind copyes.
+ * We use pointers here so there's no call to any copy constructor.
+ * It is easier for the compiler to inline it and prove that there's
+ * no need to copy anything. In that case, the bind should not be
+ * called at all to create the object, just call the function.
  */
-ZonePtr
-newZone(const std::string* zone, uint16_t class_code,
-   const AbstractRRset* authority, const vector<AbstractRRset>* additional,
-   HashTable<NameserverEntry>* ns_hash, LruList<NameserverEntry>* ns_lru)
+shared_ptr<ZoneEntry>
+newZone(const shared_ptr<ResolverInterface>* resolver, const string* zone,
+    const RRClass* class_code,
+    const shared_ptr<HashTable<NameserverEntry> >* ns_hash,
+    const shared_ptr<LruList<NameserverEntry> >* ns_lru)
 {
-    ZonePtr zone_ptr(new ZoneEntry(*zone, class_code));
-    // Sanitize the authority section and put the data there
-    if (authority->getClass().getCode() != class_code) {
-        isc_throw(InconsistentZone,
-            "Authority section is for different class, expected: " <<
-            RRClass(class_code).toText() << ", got: " <<
-            authority->getClass().toText());
-    }
-    if (authority->getName() != Name(*zone)) {
-        isc_throw(InconsistentZone,
-            "Authority section is for different zone, expected: " <<
-            *zone << ", got: " << authority->getName().toText());
-    }
-    if (authority->getType() != RRType::NS()) {
-        isc_throw(NotNS, "Authority section with non-NS RR type: " <<
-            authority->getType().toText());
-    }
-    // Make sure the name servers exist
-    RdataIteratorPtr ns(authority->getRdataIterator());
-    // TODO Remove the call to first on merge with #410
-    for (ns->first(); !ns->isLast(); ns->next()) {
-        Name ns_name(dynamic_cast<const rdata::generic::NS&>(
-                    ns->getCurrent()).getNSName());
-        string ns_name_str(ns_name.toText());
-        pair<bool, NameserverPtr> ns_lookup(
-                ns_hash->getOrAdd(HashKey(ns_name_str, class_code),
-                bind(newNs, &ns_name_str, class_code, additional)));
-        if (ns_lookup.first) { // Is it a new nameserver?
-            ns_lru->add(ns_lookup.second);
-        } else {
-            ns_lru->touch(ns_lookup.second);
-        }
-        zone_ptr->nameserverAdd(ns_lookup.second);
-    }
-
-    return zone_ptr;
+    shared_ptr<ZoneEntry> result(new ZoneEntry(*resolver, *zone, *class_code,
+        *ns_hash, *ns_lru));
+    return (result);
 }
 
 }
 
 void
-NameserverAddressStore::lookup(const std::string& zone, uint16_t class_code,
-    const AbstractRRset& authority, const vector<AbstractRRset>& additional,
-    CallbackPtr callback)
+NameserverAddressStore::lookup(const string& zone, const RRClass& class_code,
+    shared_ptr<AddressRequestCallback> callback, AddressFamily family)
 {
-    // Try to look up the entry, or create and fill it with initial data
-    pair<bool, ZonePtr> zone_lookup(
-        zone_hash_.getOrAdd(HashKey(zone, class_code),
-        bind(newZone, &zone, class_code, &authority, &additional,
+    pair<bool, shared_ptr<ZoneEntry> > zone_obj(zone_hash_->getOrAdd(HashKey(
+        zone, class_code), boost::bind(newZone, &resolver_, &zone, &class_code,
         &nameserver_hash_, &nameserver_lru_)));
-    ZonePtr zone_ptr(zone_lookup.second);
-    if (zone_lookup.first) { // New value
-        zone_lru_.add(zone_ptr);
-    } else { // Was already here
-        // FIXME Check the TTL, delete and retry if outdated
-        zone_lru_.touch(zone_ptr);
-    }
-    zone_ptr->addCallback(callback);
-    // TODO Do this only when there are no callbacks currently
-    // if there are, it means this will be just added as well
-    processZone(zone_ptr);
-}
-
-namespace {
-}
-
-// TODO Pass a nameserver that is responsible for this, as it is not
-// checked for TTL (might be 0)
-// TODO Move this to the zone
-void NameserverAddressStore::processZone(ZonePtr zone) {
-    // Addresses of existing nameservers
-    NameserverEntry::AddressVector addresses;
-    // Current state
-    Fetchable::State state;
-    {
-        // Nameservers we can still ask for their IP address
-        vector<NameserverPtr> not_asked;
-        // Are there any in-progress nameservers?
-        bool pending(false);
-        /**
-         * This is inside a block so we can unlock the zone with nameservers
-         * after we get all information from it. We do not need to keep the
-         * lock on the nameservers when we run callbacks.
-         */
-        ZoneEntry::Lock lock(zone->getLock());
-
-        if (zone->getState() != Fetchable::UNREACHABLE) {
-            // Look into all the nameservers
-
-            // FIXME This completely ignores TTLs, something shoud be done with them.
-            // Maybe turn all outdated nameservers into NOT_ASKED?
-
-            BOOST_FOREACH(NameserverPtr ns, *zone) {
-                switch (ns->getState()) {
-                    case Fetchable::NOT_ASKED:
-                        not_asked.push_back(ns);
-                        break;
-                    case Fetchable::READY:
-                        ns->getAddresses(addresses);
-                        break;
-                    case Fetchable::IN_PROGRESS:
-                        pending = true;
-                        ns->ensureHasCallback(zone, callback_);
-                        break;
-                    case Fetchable::UNREACHABLE:
-                        // Nothing. We do not care about it.
-                        break;
-                }
-            }
-        }
-
-        /*
-         * If there is no data, noone to ask and noone to expect answer from
-         * then bad luck, but this zone can't be reached.
-         */
-        if (not_asked.empty() && addresses.empty() && !pending) {
-            zone->setState(Fetchable::UNREACHABLE);
-        }
-
-        // Pick up to two nameservers and try to resolve them
-        // TODO Any better way to choose one?
-        for (int i(0); i < 2 && !not_asked.empty(); ++ i) {
-            size_t index(randIndex(not_asked.size()));
-            not_asked[index]->askIP(resolver_, zone, callback_,
-                not_asked[index]);
-            // Remove from the vector so we do not choose it again
-            not_asked.erase(not_asked.begin() + index);
-        }
-
-        state = zone->getState();
-    } // Release the lock (and some other resources as a bonus)
-
-    // If we are unreachable, tell everyone
-    if (state == Fetchable::UNREACHABLE) {
-        while (zone->hasCallbacks()) {
-            zone->popCallback()->unreachable();
-        }
-    } else if (!addresses.empty()) { // Give everyone an address
-        while (zone->hasCallbacks()) {
-            // Get the address first, so the callback is removed after we are
-            // sure there's no exception
-            asiolink::IOAddress address(chooseAddress(addresses));
-            zone->popCallback()->success(address);
-        }
+    if (zone_obj.first) {
+        zone_lru_->add(zone_obj.second);
+    } else {
+        zone_lru_->touch(zone_obj.second);
     }
-    // Otherwise, we are still waiting for more info to come, let the
-    // callbacks rest
+    zone_obj.second->addCallback(callback, family);
 }
 
 } // namespace nsas

src/lib/nsas/nameserver_address_store.h

@@ -20,44 +20,25 @@
 #include <string>
 #include <vector>
 
-#include <dns/rrset.h>
-
-#include "address_request_callback.h"
-#include "hash_table.h"
-#include "nameserver_entry.h"
-#include "lru_list.h"
-#include "zone_entry.h"
-#include "resolver_interface.h"
+#include <boost/shared_ptr.hpp>
+
 #include "nsas_types.h"
 
 namespace isc {
-namespace nsas {
+// Some forward declarations, so we do not need to include so many headers
 
-/**
- * \short Invalid referral information passed.
- *
- * This is thrown if the referral passed to NameserverAddressStore::lookup is
- * wrong. Has subexceptions for specific conditions.
- */
-struct InvalidReferral : public isc::BadValue {
-    InvalidReferral(const char *file, size_t line, const char *what) :
-        BadValue(file, line, what)
-    { }
-};
+namespace dns {
+class RRClass;
+}
 
-/// \short The referral is not for this zone.
-struct InconsistentZone : public InvalidReferral {
-    InconsistentZone(const char *file, size_t line, const char *what) :
-        InvalidReferral(file, line, what)
-    { }
-};
+namespace nsas {
 
-/// \short The authority zone contains something else than NS
-struct NotNS : public InvalidReferral {
-    NotNS(const char *file, size_t line, const char *what) :
-        InvalidReferral(file, line, what)
-    { }
-};
+class ResolverInterface;
+template<class T> class HashTable;
+template<class T> class LruList;
+class ZoneEntry;
+class NameserverEntry;
+class AddressRequestCallback;
 
 /// \brief Nameserver Address Store
 ///
@@ -80,7 +61,7 @@ public:
     /// \param zonehashsize Size of the zone hash table.  The default value of
     /// 1009 is the first prime number above 1000.
     /// \param nshash size Size of the nameserver hash table.  The default
-    /// value of 2003 is the first prime number over 2000, and by implication,
+    /// value of 3001 is the first prime number over 3000, and by implication,
     /// there is an assumption that there will be more nameservers than zones
     /// in the store.
     NameserverAddressStore(boost::shared_ptr<ResolverInterface> resolver,
@@ -92,22 +73,18 @@ public:
     virtual ~NameserverAddressStore()
     {}
 
-    // TODO Drop zone and class code, they can be found out from the authority
     /// \brief Lookup Address for a Zone
     ///
     /// Looks up the address of a nameserver in the zone.
     ///
     /// \param zone Name of zone for which an address is required.
     /// \param class_code Class of the zone.
-    /// \param authority Authority RRset from the referral containing the
-    /// nameservers that serve the zone.
     /// \param callback Callback object used to pass the result back to the
     /// caller.
-    /// \param request Which address is requested.
-    void lookup(const std::string& zone, uint16_t class_code,
-        const isc::dns::AbstractRRset& authority,
-        boost::shared_ptr<AddressRequestCallback> callback, AddressRequest
-        request = ANY_OK);
+    /// \param family Which address is requested.
+    void lookup(const std::string& zone, const dns::RRClass& class_code,
+        boost::shared_ptr<AddressRequestCallback> callback, AddressFamily
+        family = ANY_OK);
 
     /// \brief Protected Members
     ///
@@ -122,15 +99,16 @@ public:
     //@{
 protected:
     // Zone and nameserver hash tables
-    HashTable<ZoneEntry>        zone_hash_;
-    HashTable<NameserverEntry>  nameserver_hash_;
+    boost::shared_ptr<HashTable<ZoneEntry> > zone_hash_;
+    boost::shared_ptr<HashTable<NameserverEntry> > nameserver_hash_;
 
     // ... and the LRU lists
-    LruList<ZoneEntry>          zone_lru_;
-    LruList<NameserverEntry>    nameserver_lru_;
-    //}@
+    boost::shared_ptr<LruList<ZoneEntry> > zone_lru_;
+    boost::shared_ptr<LruList<NameserverEntry> > nameserver_lru_;
+    // The resolver we use
 private:
     boost::shared_ptr<ResolverInterface> resolver_;
+    //}@
 };
 
 } // namespace nsas

src/lib/nsas/tests/Makefile.am

@@ -24,7 +24,7 @@ run_unittests_SOURCES += hash_table_unittest.cc
 run_unittests_SOURCES += hash_unittest.cc
 run_unittests_SOURCES += lru_list_unittest.cc
 run_unittests_SOURCES += nameserver_address_unittest.cc
-#run_unittests_SOURCES += nameserver_address_store_unittest.cc
+run_unittests_SOURCES += nameserver_address_store_unittest.cc
 run_unittests_SOURCES += nameserver_entry_unittest.cc
 run_unittests_SOURCES += nsas_entry_compare_unittest.cc
 run_unittests_SOURCES += nsas_test.h

src/lib/nsas/tests/nameserver_address_store_unittest.cc

@@ -22,6 +22,7 @@
 
 #include <dns/rrttl.h>
 #include <dns/rdataclass.h>
+#include <dns/rrclass.h>
 
 #include <gtest/gtest.h>
 #include <boost/shared_ptr.hpp>
@@ -35,6 +36,7 @@
 #include "../nsas_entry_compare.h"
 #include "../nameserver_entry.h"
 #include "../zone_entry.h"
+#include "../address_request_callback.h"
 #include "nsas_test.h"
 
 using namespace isc::dns;
@@ -55,9 +57,10 @@ public:
     ///
     /// \param hashsize Size of the zone hash table
     /// \param lrusize Size of the zone hash table
-    DerivedNsas(ResolverInterface& resolver, uint32_t hashsize,
+    DerivedNsas(shared_ptr<TestResolver> resolver, uint32_t hashsize,
         uint32_t lrusize) :
-        NameserverAddressStore(resolver, hashsize, lrusize)
+        NameserverAddressStore(resolver, hashsize, lrusize),
+        resolver_(resolver)
     {}
 
     /// \brief Virtual Destructor
@@ -67,16 +70,39 @@ public:
     /// \brief Add Nameserver Entry to Hash and LRU Tables
     void AddNameserverEntry(boost::shared_ptr<NameserverEntry>& entry) {
         HashKey h = entry->hashKey();
-        nameserver_hash_.add(entry, h);
-        nameserver_lru_.add(entry);
+        nameserver_hash_->add(entry, h);
+        nameserver_lru_->add(entry);
     }
 
     /// \brief Add Zone Entry to Hash and LRU Tables
     void AddZoneEntry(boost::shared_ptr<ZoneEntry>& entry) {
         HashKey h = entry->hashKey();
-        zone_hash_.add(entry, h);
-        zone_lru_.add(entry);
+        zone_hash_->add(entry, h);
+        zone_lru_->add(entry);
     }
+    /**
+     * \short Just wraps the common lookup
+     *
+     * It calls the lookup and provides the authority section
+     * if it is asked for by the resolver.
+     */
+    void lookupAndAnswer(const string& name, const RRClass& class_code,
+        shared_ptr<AbstractRRset> authority,
+        shared_ptr<AddressRequestCallback> callback)
+    {
+        size_t size(resolver_->requests.size());
+        NameserverAddressStore::lookup(name, class_code, callback, ANY_OK);
+        // It asked something, the only thing it can ask is the NS list
+        if (size < resolver_->requests.size()) {
+            resolver_->provideNS(size, authority);
+            // Once answered, drop the request so noone else sees it
+            resolver_->requests.erase(resolver_->requests.begin() + size);
+        } else {
+            ADD_FAILURE() << "Not asked for NS";
+        }
+    }
+private:
+    shared_ptr<TestResolver> resolver_;
 };
 
 
@@ -89,22 +115,31 @@ protected:
         authority_(new RRset(Name("example.net."), RRClass::IN(), RRType::NS(),
             RRTTL(128))),
         empty_authority_(new RRset(Name("example.net."), RRClass::IN(),
-            RRType::NS(), RRTTL(128)))
+            RRType::NS(), RRTTL(128))),
+        resolver_(new TestResolver)
     {
-        // Constructor - initialize a set of nameserver and zone objects.  For convenience,
-        // these are stored in vectors.
+        // Constructor - initialize a set of nameserver and zone objects.  For
+        // convenience, these are stored in vectors.
         for (int i = 1; i <= 9; ++i) {
-            std::string name = "nameserver" + boost::lexical_cast<std::string>(i);
-            nameservers_.push_back(boost::shared_ptr<NameserverEntry>(new NameserverEntry(name, (40 + i))));
+            std::string name = "nameserver" + boost::lexical_cast<std::string>(
+                i);
+            nameservers_.push_back(boost::shared_ptr<NameserverEntry>(
+                new NameserverEntry(name, RRClass(40 + i))));
         }
 
+        // Some zones. They will not use the tables in this test, so it can be
+        // empty
         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(name, (40 + i))));
+            zones_.push_back(boost::shared_ptr<ZoneEntry>(new ZoneEntry(
+                resolver_, name, RRClass(40 + i),
+                shared_ptr<HashTable<NameserverEntry> >(),
+                shared_ptr<LruList<NameserverEntry> >())));
         }
 
         // A nameserver serving data
-        authority_->addRdata(ConstRdataPtr(new rdata::generic::NS(Name("ns.example.com."))));
+        authority_->addRdata(ConstRdataPtr(new rdata::generic::NS(Name(
+            "ns.example.com."))));
 
         // This is reused because of convenience, clear it just in case
         NSASCallback::results.clear();
@@ -116,7 +151,7 @@ protected:
 
     RRsetPtr authority_, empty_authority_;
 
-    TestResolver defaultTestResolver;
+    shared_ptr<TestResolver> resolver_;
 
     class NSASCallback : public AddressRequestCallback {
         public:
@@ -148,7 +183,7 @@ TEST_F(NameserverAddressStoreTest, ZoneDeletionCheck) {
 
     // Create a NSAS with a hash size of three and a LRU size of 9 (both zone and
     // nameserver tables).
-    DerivedNsas nsas(defaultTestResolver, 2, 2);
+    DerivedNsas nsas(resolver_, 2, 2);
 
     // Add six entries to the tables.  After addition the reference count of each element
     // should be 3 - one for the entry in the zones_ vector, and one each for the entries
@@ -178,7 +213,7 @@ TEST_F(NameserverAddressStoreTest, NameserverDeletionCheck) {
 
     // Create a NSAS with a hash size of three and a LRU size of 9 (both zone and
     // nameserver tables).
-    DerivedNsas nsas(defaultTestResolver, 2, 2);
+    DerivedNsas nsas(resolver_, 2, 2);
 
     // Add six entries to the tables.  After addition the reference count of each element
     // should be 3 - one for the entry in the nameservers_ vector, and one each for the entries
@@ -205,37 +240,29 @@ TEST_F(NameserverAddressStoreTest, NameserverDeletionCheck) {
  * Check if it asks correct questions and it keeps correct internal state.
  */
 TEST_F(NameserverAddressStoreTest, emptyLookup) {
-    DerivedNsas nsas(defaultTestResolver, 10, 10);
+    DerivedNsas nsas(resolver_, 10, 10);
     // Ask it a question
-    nsas.lookup("example.net.", RRClass::IN().getCode(), *authority_,
-        vector<AbstractRRset>(), getCallback());
-    // It should ask for IP addresses for example.com.
-    ASSERT_EQ(2, defaultTestResolver.requests.size());
-    defaultTestResolver.asksIPs(Name("ns.example.com."), 0, 1);
+    nsas.lookupAndAnswer("example.net.", RRClass::IN(), authority_,
+        getCallback());
+    // It should ask for IP addresses for ns.example.com.
+    resolver_->asksIPs(Name("ns.example.com."), 0, 1);
 
     // Ask another question for the same zone
-    nsas.lookup("example.net.", RRClass::IN().getCode(), *authority_,
-        vector<AbstractRRset>(), getCallback());
+    nsas.lookup("example.net.", RRClass::IN(), getCallback());
     // It should ask no more questions now
-    EXPECT_EQ(2, defaultTestResolver.requests.size());
+    EXPECT_EQ(2, resolver_->requests.size());
 
     // Ask another question with different zone but the same nameserver
     authority_->setName(Name("example.com."));
-    nsas.lookup("example.com.", RRClass::IN().getCode(), *authority_,
-        vector<AbstractRRset>(), getCallback());
+    nsas.lookupAndAnswer("example.com.", RRClass::IN(), authority_,
+        getCallback());
     // It still should ask nothing
-    EXPECT_EQ(2, defaultTestResolver.requests.size());
+    EXPECT_EQ(2, resolver_->requests.size());
 
     // We provide IP address of one nameserver, it should generate all the
     // results
-    RRsetPtr answer(new RRset(Name("example.com."), RRClass::IN(), RRType::A(),
-        RRTTL(100)));
-    answer->addRdata(rdata::in::A("192.0.2.1"));
-    Message address(Message::RENDER); // Not able to create different one
-    address.addRRset(Section::ANSWER(), answer);
-    address.addRRset(Section::AUTHORITY(), authority_);
-    address.addQuestion(defaultTestResolver[0]);
-    defaultTestResolver.requests[0].second->success(address);
+    resolver_->answer(0, Name("ns.example.com."), RRType::A(),
+        rdata::in::A("192.0.2.1"));
     EXPECT_EQ(3, NSASCallback::results.size());
     BOOST_FOREACH(const NSASCallback::Result& result, NSASCallback::results) {
         EXPECT_TRUE(result.first);
@@ -249,12 +276,12 @@ TEST_F(NameserverAddressStoreTest, emptyLookup) {
  * It should not ask anything and say it is unreachable right away.
  */
 TEST_F(NameserverAddressStoreTest, zoneWithoutNameservers) {
-    DerivedNsas nsas(defaultTestResolver, 10, 10);
+    DerivedNsas nsas(resolver_, 10, 10);
     // Ask it a question
-    nsas.lookup("example.net.", RRClass::IN().getCode(), *empty_authority_,
-        vector<AbstractRRset>(), getCallback());
+    nsas.lookupAndAnswer("example.net.", RRClass::IN(), empty_authority_,
+        getCallback());
     // There should be no questions, because there's nothing to ask
-    EXPECT_EQ(0, defaultTestResolver.requests.size());
+    EXPECT_EQ(0, resolver_->requests.size());
     // And there should be one „unreachable“ answer for the query
     ASSERT_EQ(1, NSASCallback::results.size());
     EXPECT_FALSE(NSASCallback::results[0].first);
@@ -268,35 +295,33 @@ TEST_F(NameserverAddressStoreTest, zoneWithoutNameservers) {
  * without further asking.
  */
 TEST_F(NameserverAddressStoreTest, unreachableNS) {
-    DerivedNsas nsas(defaultTestResolver, 10, 10);
+    DerivedNsas nsas(resolver_, 10, 10);
     // Ask it a question
-    nsas.lookup("example.net.", RRClass::IN().getCode(), *authority_,
-        vector<AbstractRRset>(), getCallback());
+    nsas.lookupAndAnswer("example.net.", RRClass::IN(), authority_,
+        getCallback());
     // It should ask for IP addresses for example.com.
-    ASSERT_EQ(2, defaultTestResolver.requests.size());
-    defaultTestResolver.asksIPs(Name("ns.example.com."), 0, 1);
+    ASSERT_EQ(2, resolver_->requests.size());
+    resolver_->asksIPs(Name("ns.example.com."), 0, 1);
 
     // Ask another question with different zone but the same nameserver
     authority_->setName(Name("example.com."));
-    nsas.lookup("example.com.", RRClass::IN().getCode(), *authority_,
-        vector<AbstractRRset>(), getCallback());
+    nsas.lookupAndAnswer("example.com.", RRClass::IN(), authority_,
+        getCallback());
     // It should ask nothing more now
-    EXPECT_EQ(2, defaultTestResolver.requests.size());
+    EXPECT_EQ(2, resolver_->requests.size());
 
     // We say there are no addresses
-    defaultTestResolver.requests[0].second->failure();
-    defaultTestResolver.requests[1].second->failure();
+    resolver_->requests[0].second->failure();
+    resolver_->requests[1].second->failure();
 
     // We should have 2 answers now
     EXPECT_EQ(2, NSASCallback::results.size());
     // When we ask one same and one other zone with the same nameserver,
     // it should generate no questions and answer right away
-    authority_->setName(Name("example.net."));
-    nsas.lookup("example.net.", RRClass::IN().getCode(), *authority_,
-        vector<AbstractRRset>(), getCallback());
+    nsas.lookup("example.net.", RRClass::IN(), getCallback());
     authority_->setName(Name("example.org."));
-    nsas.lookup("example.org.", RRClass::IN().getCode(), *authority_,
-        vector<AbstractRRset>(), getCallback());
+    nsas.lookupAndAnswer("example.org.", RRClass::IN(), authority_,
+        getCallback());
     // There should be 4 negative answers now
     EXPECT_EQ(4, NSASCallback::results.size());
     BOOST_FOREACH(const NSASCallback::Result& result, NSASCallback::results) {
@@ -304,27 +329,11 @@ TEST_F(NameserverAddressStoreTest, unreachableNS) {
     }
 }
 
-/// \short Test invalid authority section.
-TEST_F(NameserverAddressStoreTest, invalidAuthority) {
-    DerivedNsas nsas(defaultTestResolver, 2, 2);
-    EXPECT_THROW(nsas.lookup("example.net.", RRClass::CH().getCode(),
-        *authority_, vector<AbstractRRset>(), getCallback()),
-        InconsistentZone);
-    EXPECT_EQ(0, defaultTestResolver.requests.size());
-    EXPECT_EQ(0, NSASCallback::results.size());
-    EXPECT_THROW(nsas.lookup("example.com.", RRClass::IN().getCode(),
-        *authority_, vector<AbstractRRset>(), getCallback()),
-        InconsistentZone);
-    EXPECT_EQ(0, defaultTestResolver.requests.size());
-    EXPECT_EQ(0, NSASCallback::results.size());
-    BasicRRset aAuthority(Name("example.net."), RRClass::IN(), RRType::A(),
-            RRTTL(128));
-    EXPECT_THROW(nsas.lookup("example.net.", RRClass::IN().getCode(),
-        aAuthority, vector<AbstractRRset>(),
-        getCallback()), NotNS);
-    EXPECT_EQ(0, defaultTestResolver.requests.size());
-    EXPECT_EQ(0, NSASCallback::results.size());
-}
+/*
+ * TODO: More tests. Some eviction combined with lookups would make sense.
+ * Stressing the entries that some nameservers for zone are there and some
+ * are not, etc.
+ */
 
 } // namespace nsas
 } // namespace isc

src/lib/nsas/tests/nsas_test.h

@@ -289,7 +289,9 @@ class TestResolver : public isc::nsas::ResolverInterface {
             requests[index].second->success(set);
         }
 
-        void provideNS(size_t index, RRsetPtr nameservers) {
+        void provideNS(size_t index,
+            boost::shared_ptr<AbstractRRset> nameservers)
+        {
             if (index >= requests.size()) {
                 throw NoSuchRequest();
             }

src/lib/nsas/tests/zone_entry_unittest.cc

@@ -279,6 +279,8 @@ TEST_F(ZoneEntryTest, CallbackTwoNS) {
  *   (provide it with some kind of cache-like thing, preconfigure answer, so
  *   the things start recursing).
  * - Combine this with some timeouting.
+ * - Look what happens when the nameservers are already in some different
+ *   states and not just newly created.
  */
 
 }   // namespace

src/lib/nsas/zone_entry.cc

@@ -79,6 +79,8 @@ class ZoneEntry::ResolverCallback : public ResolverInterface::Callback {
             } else {
                 // Store the current ones so we can keep them
                 map<string, NameserverPtr> old;
+                set<NameserverPtr> old_not_asked;
+                old_not_asked.swap(entry_->nameservers_not_asked_);
                 BOOST_FOREACH(const NameserverPtr& ptr, entry_->nameservers_) {
                     old[ptr->getName()] = ptr;
                 }
@@ -102,16 +104,29 @@ class ZoneEntry::ResolverCallback : public ResolverInterface::Callback {
                                 entry_->nameserver_table_->getOrAdd(HashKey(
                                 ns_name_str, entry_->class_code_), bind(
                                 newNs, &ns_name_str, &entry_->class_code_)));
-                            // Touch it if it is not newly created
-                            if (!from_hash.first) {
+                            // Make it at the front of the list
+                            if (from_hash.first) {
+                                entry_->nameserver_lru_->add(from_hash.second);
+                            } else {
                                 entry_->nameserver_lru_->touch(
                                     from_hash.second);
                             }
                             // And add it at last
                             entry_->nameservers_.push_back(from_hash.second);
+                            entry_->nameservers_not_asked_.insert(
+                                from_hash.second);
                         } else {
                             // We have it, so just use it
                             entry_->nameservers_.push_back(old_ns->second);
+                            // Did we ask it already? If not, it is still not
+                            // asked (the one designing std interface must
+                            // have been mad)
+                            if (old_not_asked.find(old_ns->second) !=
+                                old_not_asked.end())
+                            {
+                                entry_->nameservers_not_asked_.insert(
+                                    old_ns->second);
+                            }
                         }
                     }
                     // OK, we skip this one it is not NS (log?)
@@ -330,6 +345,13 @@ ZoneEntry::process(CallbackPtr callback, AddressFamily family,
                     switch (ns_state) {
                         case IN_PROGRESS:
                             pending = true;
+                            // Someone asked it, but not us, we don't have
+                            // callback
+                            if (nameservers_not_asked_.find(ns) !=
+                                nameservers_not_asked_.end())
+                            {
+                                to_ask.push_back(ns);
+                            }
                             break;
                         case NOT_ASKED:
                         case EXPIRED:
@@ -344,16 +366,11 @@ ZoneEntry::process(CallbackPtr callback, AddressFamily family,
 
                 // We have someone to ask, so do it
                 if (!to_ask.empty()) {
+                    // We ask everything that makes sense now
+                    nameservers_not_asked_.clear();
                     // We should not be locked, because this function can
                     // be called directly from the askIP again
                     lock->unlock();
-                    shared_ptr<NameserverCallback> ns_callbacks[ADDR_REQ_MAX];;
-                    ns_callbacks[ANY_OK].reset(new NameserverCallback(
-                        shared_from_this(), ANY_OK));
-                    ns_callbacks[V4_ONLY].reset(new NameserverCallback(
-                        shared_from_this(), V4_ONLY));
-                    ns_callbacks[V6_ONLY].reset(new NameserverCallback(
-                        shared_from_this(), V6_ONLY));
                     /*
                      * TODO: Possible place for an optimisation. We now ask
                      * everything we can. We should limit this to something like
@@ -367,9 +384,7 @@ ZoneEntry::process(CallbackPtr callback, AddressFamily family,
                         // callback for different one.
                         // If they recurse back to us (call directly), we kill
                         // it by the in_process_
-                        ns->askIP(resolver_, ns_callbacks[V4_ONLY], V4_ONLY);
-                        ns->askIP(resolver_, ns_callbacks[V6_ONLY], V6_ONLY);
-                        ns->askIP(resolver_, ns_callbacks[ANY_OK], ANY_OK);
+                        insertCallback(ns, ADDR_REQ_MAX);
                     }
                     // Retry with all the data that might have arrived
                     in_process_[family] = false;
@@ -401,5 +416,18 @@ ZoneEntry::process(CallbackPtr callback, AddressFamily family,
     }
 }
 
+void
+ZoneEntry::insertCallback(NameserverPtr ns, AddressFamily family) {
+    if (family == ADDR_REQ_MAX) {
+        insertCallback(ns, ANY_OK);
+        insertCallback(ns, V4_ONLY);
+        insertCallback(ns, V6_ONLY);
+    } else {
+        shared_ptr<NameserverCallback> callback(new NameserverCallback(
+            shared_from_this(), family));
+        ns->askIP(resolver_, callback, family);
+    }
+}
+
 }; // namespace nsas
 }; // namespace isc

src/lib/nsas/zone_entry.h

@@ -19,6 +19,7 @@
 
 #include <string>
 #include <vector>
+#include <set>
 #include <boost/thread.hpp>
 #include <boost/shared_ptr.hpp>
 #include <boost/enable_shared_from_this.hpp>
@@ -105,6 +106,8 @@ protected:
     typedef boost::shared_ptr<NameserverEntry> NameserverPtr;
     typedef std::vector<NameserverPtr> NameserverVector;
     NameserverVector nameservers_; ///< Nameservers
+    // Which nameservers didn't have any of our callbacks yet
+    std::set<NameserverPtr> nameservers_not_asked_;
     /*
      * Callbacks. For each fimily type one vector, so we can process
      * them separately.
@@ -153,6 +156,9 @@ private:
     // The lock is mandatory
     void dispatchFailures(AddressFamily family,
         boost::shared_ptr<boost::mutex::scoped_lock> lock);
+    // Put a callback into the nameserver entry. Same ADDR_REQ_MAX means for
+    // all families
+    void insertCallback(NameserverPtr nameserver, AddressFamily family);
 };
 
 } // namespace nsas