[2211] use DataSrcClientsMgr for query processing

src/bin/auth/auth_srv.cc

@@ -654,15 +654,15 @@ AuthSrvImpl::processNormalQuery(const IOMessage& io_message, Message& message,
         local_edns->setUDPSize(AuthSrvImpl::DEFAULT_LOCAL_UDPSIZE);
         message.setEDNS(local_edns);
     }
-    // Lock the client lists and keep them under the lock until the processing
-    // and rendering is done (this is the same mutex as from
-    // AuthSrv::getDataSrcClientListMutex()).
-    isc::util::thread::Mutex::Locker locker(mutex_);
+    // Get access to data source client list through the holder and keep thek
+    // holder until the processing and rendering is done to avoid inter-thread
+    // race.
+    auth::DataSrcClientsMgr::Holder datasrc_holder(datasrc_clients_mgr_);
 
     try {
         const ConstQuestionPtr question = *message.beginQuestion();
         const shared_ptr<datasrc::ClientList>
-            list(getDataSrcClientList(question->getClass()));
+            list(datasrc_holder.findClientList(question->getClass()));
         if (list) {
             const RRType& qtype = question->getType();
             const Name& qname = question->getName();

src/bin/auth/command.cc

@@ -192,9 +192,10 @@ public:
         const Name origin(origin_elem->stringValue());
 
         DataSrcClientsMgr::Holder holder(server.getDataSrcClientsMgr());
-        ConfigurableClientList* list = holder.findClientList(zone_class);
+        boost::shared_ptr<ConfigurableClientList> list =
+            holder.findClientList(zone_class);
 
-        if (list == NULL) {
+        if (!list) {
             isc_throw(AuthCommandError, "There's no client list for "
                       "class " << zone_class);
         }

src/bin/auth/datasrc_clients_mgr.h

@@ -116,15 +116,15 @@ public:
         /// otherwise it returns NULL.  The manager keeps the ownership of
         /// the pointed object.  Also, it's not safe to get access to the
         /// object beyond the scope of the holder object.
-        datasrc::ConfigurableClientList* findClientList(
+        boost::shared_ptr<datasrc::ConfigurableClientList> findClientList(
             const dns::RRClass& rrclass)
         {
             const ClientListsMap::const_iterator
                 it = mgr_.clients_map_->find(rrclass);
             if (it == mgr_.clients_map_->end()) {
-                return (NULL);
+                return (boost::shared_ptr<datasrc::ConfigurableClientList>());
             } else {
-                return (it->second.get());
+                return (it->second);
             }
         }
     private:

src/bin/auth/tests/auth_srv_unittest.cc

@@ -70,6 +70,7 @@ using namespace isc::util::unittests;
 using namespace isc::dns::rdata;
 using namespace isc::data;
 using namespace isc::xfr;
+using namespace isc::auth;
 using namespace isc::asiodns;
 using namespace isc::asiolink;
 using namespace isc::testutils;
@@ -726,11 +727,11 @@ TEST_F(AuthSrvTest, notifyWithSessionMessageError) {
 }
 
 void
-installDataSrcClientLists(AuthSrv& server,
-                          DataSrcClientListsPtr lists)
-{
-    thread::Mutex::Locker locker(server.getDataSrcClientListMutex());
-    server.swapDataSrcClientLists(lists);
+installDataSrcClientLists(AuthSrv& server, DataSrcClientListsPtr lists) {
+    // For now, we use explicit swap than reconfigure() because the latter
+    // involves a separate thread and cannot guarantee the new config is
+    // available for the subsequent test.
+    server.getDataSrcClientsMgr().swapDataSrcClientLists(lists);
 }
 
 void
@@ -1438,16 +1439,16 @@ TEST_F(AuthSrvTest,
 {
     // Set real inmem client to proxy
     updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE);
+    boost::shared_ptr<isc::datasrc::ConfigurableClientList> list;
+    DataSrcClientsMgr& mgr = server.getDataSrcClientsMgr();
     {
-        isc::util::thread::Mutex::Locker locker(
-            server.getDataSrcClientListMutex());
-        boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-            list(new FakeList(server.getDataSrcClientList(RRClass::IN()),
-                              THROW_NEVER, false));
-        DataSrcClientListsPtr lists(new std::map<RRClass, ListPtr>);
-        lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
-        server.swapDataSrcClientLists(lists);
+        DataSrcClientsMgr::Holder holder(mgr);
+        list.reset(new FakeList(holder.findClientList(RRClass::IN()),
+                                THROW_NEVER, false));
     }
+    DataSrcClientListsPtr lists(new std::map<RRClass, ListPtr>);
+    lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
+    server.getDataSrcClientsMgr().swapDataSrcClientLists(lists);
 
     createDataFromFile("nsec3query_nodnssec_fromWire.wire");
     server.processMessage(*io_message, *parse_message, *response_obuffer,
@@ -1470,14 +1471,16 @@ setupThrow(AuthSrv& server, ThrowWhen throw_when, bool isc_exception,
 {
     updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE);
 
-    isc::util::thread::Mutex::Locker locker(
-        server.getDataSrcClientListMutex());
-    boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-        list(new FakeList(server.getDataSrcClientList(RRClass::IN()),
-                          throw_when, isc_exception, rrset));
+    boost::shared_ptr<isc::datasrc::ConfigurableClientList> list;
+    DataSrcClientsMgr& mgr = server.getDataSrcClientsMgr();
+    {           // we need to limit the scope so swap is outside of it
+        DataSrcClientsMgr::Holder holder(mgr);
+        list.reset(new FakeList(holder.findClientList(RRClass::IN()),
+                                throw_when, isc_exception, rrset));
+    }
     DataSrcClientListsPtr lists(new std::map<RRClass, ListPtr>);
     lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
-    server.swapDataSrcClientLists(lists);
+    mgr.swapDataSrcClientLists(lists);
 }
 
 TEST_F(AuthSrvTest,

src/bin/auth/tests/datasrc_clients_mgr_unittest.cc

@@ -154,10 +154,8 @@ TEST(DataSrcClientsMgrTest, holder) {
     {
         // Initially it's empty, so findClientList() will always return NULL
         TestDataSrcClientsMgr::Holder holder(mgr);
-        EXPECT_EQ(static_cast<ConfigurableClientList*>(NULL),
-                  holder.findClientList(RRClass::IN()));
-        EXPECT_EQ(static_cast<ConfigurableClientList*>(NULL),
-                  holder.findClientList(RRClass::CH()));
+        EXPECT_FALSE(holder.findClientList(RRClass::IN()));
+        EXPECT_FALSE(holder.findClientList(RRClass::CH()));
         // map should be protected here
         EXPECT_EQ(1, FakeDataSrcClientsBuilder::map_mutex->lock_count);
         EXPECT_EQ(0, FakeDataSrcClientsBuilder::map_mutex->unlock_count);
@@ -174,10 +172,8 @@ TEST(DataSrcClientsMgrTest, holder) {
     mgr.reconfigure(reconfigure_arg);
     {
         TestDataSrcClientsMgr::Holder holder(mgr);
-        EXPECT_NE(static_cast<ConfigurableClientList*>(NULL),
-                  holder.findClientList(RRClass::IN()));
-        EXPECT_NE(static_cast<ConfigurableClientList*>(NULL),
-                  holder.findClientList(RRClass::CH()));
+        EXPECT_TRUE(holder.findClientList(RRClass::IN()));
+        EXPECT_TRUE(holder.findClientList(RRClass::CH()));
     }
     // We need to clear command queue by hand
     FakeDataSrcClientsBuilder::command_queue->clear();
@@ -190,10 +186,8 @@ TEST(DataSrcClientsMgrTest, holder) {
     mgr.reconfigure(reconfigure_arg);
     {
         TestDataSrcClientsMgr::Holder holder(mgr);
-        EXPECT_NE(static_cast<ConfigurableClientList*>(NULL),
-                  holder.findClientList(RRClass::IN()));
-        EXPECT_EQ(static_cast<ConfigurableClientList*>(NULL),
-                  holder.findClientList(RRClass::CH()));
+        EXPECT_TRUE(holder.findClientList(RRClass::IN()));
+        EXPECT_FALSE(holder.findClientList(RRClass::CH()));
     }
 }