[2211] introduce DataSrcClientsMgr::Holder.

auth_srv test's clientList tests are now converted to clients manager's test.
("holder" tests).

src/bin/auth/datasrc_clients_mgr.h

@@ -90,6 +90,47 @@ private:
     ClientListsMap;
 
 public:
+    /// \brief Thread-safe accessor to the data source client lists.
+    ///
+    /// This class provides a simple wrapper for searching the client lists
+    /// stored in the DataSrcClientsMgr in a thread-safe manner.
+    /// It ensures the result of \c getClientList() can be used without
+    /// causing a race condition with other threads that can possibly use
+    /// the same manager throughout the lifetime of the holder object.
+    ///
+    /// This also means the holder object is expected to have a short lifetime.
+    /// The application shouldn't try to keep it unnecessarily long.
+    /// It's normally expected to create the holder object on the stack
+    /// of a small scope and automatically let it be destroyed at the end
+    /// of the scope.
+    class Holder {
+    public:
+        Holder(DataSrcClientsMgrBase& mgr) :
+            mgr_(mgr), locker_(mgr_.map_mutex_)
+        {}
+
+        /// \brief Find a data source client list of a specified RR class.
+        ///
+        /// It returns a pointer to the list stored in the manager if found,
+        /// 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(
+            const dns::RRClass& rrclass)
+        {
+            const ClientListsMap::const_iterator
+                it = mgr_.clients_map_->find(rrclass);
+            if (it == mgr_.clients_map_->end()) {
+                return (NULL);
+            } else {
+                return (it->second.get());
+            }
+        }
+    private:
+        DataSrcClientsMgrBase& mgr_;
+        typename MutexType::Locker locker_;
+    };
+
     /// \brief Constructor.
     ///
     /// It internally invokes a separate thread and waits for further
@@ -165,6 +206,7 @@ public:
             isc_throw(InvalidParameter, "Invalid null config argument");
         }
         sendCommand(datasrc_clientmgr_internal::RECONFIGURE, config_arg);
+        reconfigureHook();      // for test's customization
     }
 
 private:
@@ -174,6 +216,9 @@ private:
     // state of the class.
     void cleanup() {}
 
+    // same as cleanup(), for reconfigure().
+    void reconfigureHook() {}
+
     void sendCommand(datasrc_clientmgr_internal::CommandID command,
                      data::ConstElementPtr arg)
     {

src/bin/auth/tests/auth_srv_unittest.cc

@@ -1784,46 +1784,6 @@ TEST_F(AuthSrvTest, DDNSForwardCreateDestroy) {
                 Opcode::UPDATE().getCode(), QR_FLAG, 0, 0, 0, 0);
 }
 
-// Check the client list accessors
-TEST_F(AuthSrvTest, clientList) {
-    // We need to lock the mutex to make the (get|set)ClientList happy.
-    // There's a debug-build only check in them to make sure everything
-    // locks them and we call them directly here.
-    isc::util::thread::Mutex::Locker locker(
-        server.getDataSrcClientListMutex());
-
-    DataSrcClientListsPtr lists; // initially empty
-
-    // The lists don't exist. Therefore, the list of RRClasses is empty.
-    EXPECT_TRUE(server.swapDataSrcClientLists(lists)->empty());
-
-    // Put something in.
-    const ListPtr list(new ConfigurableClientList(RRClass::IN()));
-    const ListPtr list2(new ConfigurableClientList(RRClass::CH()));
-
-    lists.reset(new std::map<RRClass, ListPtr>);
-    lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
-    lists->insert(pair<RRClass, ListPtr>(RRClass::CH(), list2));
-    server.swapDataSrcClientLists(lists);
-
-    // And the lists can be retrieved.
-    EXPECT_EQ(list, server.getDataSrcClientList(RRClass::IN()));
-    EXPECT_EQ(list2, server.getDataSrcClientList(RRClass::CH()));
-
-    // Replace the lists with new lists containing only one list.
-    lists.reset(new std::map<RRClass, ListPtr>);
-    lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
-    lists = server.swapDataSrcClientLists(lists);
-
-    // Old one had two lists.  That confirms our swap for IN and CH classes
-    // (i.e., no other entries were there).
-    EXPECT_EQ(2, lists->size());
-
-    // The CH list really got deleted.
-    EXPECT_EQ(list, server.getDataSrcClientList(RRClass::IN()));
-    EXPECT_FALSE(server.getDataSrcClientList(RRClass::CH()));
-}
-
 // We just test the mutex can be locked (exactly once).
 TEST_F(AuthSrvTest, mutex) {
     isc::util::thread::Mutex::Locker l1(server.getDataSrcClientListMutex());

src/bin/auth/tests/datasrc_clients_mgr_unittest.cc

@@ -14,8 +14,12 @@
 
 #include <exceptions/exceptions.h>
 
+#include <dns/rrclass.h>
+
 #include <cc/data.h>
 
+#include <datasrc/client_list.h>
+
 #include <auth/datasrc_clients_mgr.h>
 #include "test_datasrc_clients_mgr.h"
 
@@ -23,7 +27,9 @@
 
 #include <boost/function.hpp>
 
+using namespace isc::dns;
 using namespace isc::data;
+using namespace isc::datasrc;
 using namespace isc::auth;
 using namespace isc::auth::datasrc_clientmgr_internal;
 
@@ -142,6 +148,56 @@ TEST(DataSrcClientsMgrTest, reconfigure) {
     checkSharedMembers(2, 2, 0, 0, 2, 1); // no state change
 }
 
+TEST(DataSrcClientsMgrTest, holder) {
+    TestDataSrcClientsMgr mgr;
+
+    {
+        // 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()));
+        // map should be protected here
+        EXPECT_EQ(1, FakeDataSrcClientsBuilder::map_mutex->lock_count);
+        EXPECT_EQ(0, FakeDataSrcClientsBuilder::map_mutex->unlock_count);
+    }
+    // map lock has been released
+    EXPECT_EQ(1, FakeDataSrcClientsBuilder::map_mutex->unlock_count);
+
+    // Put something in, that should become visible.
+    ConstElementPtr reconfigure_arg = Element::fromJSON(
+        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
+        "              \"cache-enable\": true}],"
+        "\"CH\": [{\"type\": \"MasterFiles\", \"params\": {},"
+        "              \"cache-enable\": true}]}");
+    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()));
+    }
+    // We need to clear command queue by hand
+    FakeDataSrcClientsBuilder::command_queue->clear();
+
+    // Replace the lists with new lists containing only one list.
+    // The CH will disappear again.
+    reconfigure_arg = Element::fromJSON(
+        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
+        "              \"cache-enable\": true}]}");
+    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()));
+    }
+}
+
+
 TEST(DataSrcClientsMgrTest, realThread) {
     // Using the non-test definition with a real thread.  Just checking
     // no disruption happens.

src/bin/auth/tests/test_datasrc_clients_mgr.cc

@@ -17,6 +17,8 @@
 
 #include "test_datasrc_clients_mgr.h"
 
+#include <cassert>
+
 namespace isc {
 namespace auth {
 namespace datasrc_clientmgr_internal {
@@ -71,6 +73,18 @@ TestDataSrcClientsMgr::cleanup() {
         &FakeDataSrcClientsBuilder::cond_copy;
 }
 
+template<>
+void
+TestDataSrcClientsMgr::reconfigureHook() {
+    using namespace datasrc_clientmgr_internal;
+
+    // Simply replace the local map, ignoring bogus config value.
+    assert(command_queue_.front().first == RECONFIGURE);
+    try {
+        clients_map_ = configureDataSource(command_queue_.front().second);
+    } catch (...) {}
+}
+
 } // namespace auth
 } // namespace isc
 

src/bin/auth/tests/test_datasrc_clients_mgr.h

@@ -204,6 +204,9 @@ template<>
 void
 TestDataSrcClientsMgr::cleanup();
 
+template<>
+void
+TestDataSrcClientsMgr::reconfigureHook();
 } // namespace auth
 } // namespace isc