[master] Merge branch 'trac2211'

with fixing Conflicts to:
	src/bin/auth/auth_srv.cc
	src/bin/auth/auth_srv.h
	src/bin/auth/datasrc_clients_mgr.h
	src/bin/auth/main.cc
	src/bin/auth/tests/auth_srv_unittest.cc
	src/bin/auth/tests/command_unittest.cc

src/bin/auth/auth_messages.mes

@@ -121,11 +121,10 @@ The separate thread for maintaining data source clients has been stopped.
 % AUTH_DATASRC_CLIENTS_SHUTDOWN_ERROR error on waiting for data source builder thread: %1
 This indicates that the separate thread for maintaining data source
 clients had been terminated due to an uncaught exception, and the
-manager notices that at its own termination.  There should have been
-AUTH_DATASRC_CLIENTS_BUILDER_FAILED or
-AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED error messages in past
-logs.  If this message appears, the maintenance of the data source
-clients hasn't been working properly for some time.
+manager notices that at its own termination.  This is not an expected
+event, because the thread is implemented so it catches all exceptions
+internally.  So, if this message is logged it's most likely some internal
+bug, and it would be nice to file a bug report.
 
 % AUTH_DATASRC_CLIENTS_SHUTDOWN_UNEXPECTED_ERROR Unexpected error on waiting for data source builder thread
 Some exception happens while waiting for the termination of the

src/bin/auth/auth_srv.cc

@@ -26,7 +26,6 @@
 #include <exceptions/exceptions.h>
 
 #include <util/buffer.h>
-#include <util/threads/sync.h>
 
 #include <dns/edns.h>
 #include <dns/exceptions.h>
@@ -53,6 +52,7 @@
 #include <auth/query.h>
 #include <auth/statistics.h>
 #include <auth/auth_log.h>
+#include <auth/datasrc_clients_mgr.h>
 
 #include <boost/bind.hpp>
 #include <boost/lexical_cast.hpp>
@@ -268,24 +268,8 @@ public:
     /// The TSIG keyring
     const shared_ptr<TSIGKeyRing>* keyring_;
 
-    /// The data source client list
-    ClientListMapPtr datasrc_client_lists_;
-
-    shared_ptr<ConfigurableClientList> getDataSrcClientList(
-        const RRClass& rrclass)
-    {
-        // TODO: Debug-build only check
-        if (!mutex_.locked()) {
-            isc_throw(isc::Unexpected, "Not locked!");
-        }
-        const std::map<RRClass, shared_ptr<ConfigurableClientList> >::
-            const_iterator it(datasrc_client_lists_->find(rrclass));
-        if (it == datasrc_client_lists_->end()) {
-            return (shared_ptr<ConfigurableClientList>());
-        } else {
-            return (it->second);
-        }
-    }
+    /// The data source client list manager
+    auth::DataSrcClientsMgr datasrc_clients_mgr_;
 
     /// Bind the ModuleSpec object in config_session_ with
     /// isc:config::ModuleSpec::validateStatistics.
@@ -315,8 +299,6 @@ public:
                       isc::dns::Message& message,
                       bool done);
 
-    mutable util::thread::Mutex mutex_;
-
 private:
     bool xfrout_connected_;
     AbstractXfroutClient& xfrout_client_;
@@ -336,8 +318,6 @@ AuthSrvImpl::AuthSrvImpl(AbstractXfroutClient& xfrout_client,
     xfrin_session_(NULL),
     counters_(),
     keyring_(NULL),
-    datasrc_client_lists_(new std::map<RRClass,
-                          shared_ptr<ConfigurableClientList> >()),
     ddns_base_forwarder_(ddns_forwarder),
     ddns_forwarder_(NULL),
     xfrout_connected_(false),
@@ -488,6 +468,11 @@ AuthSrv::getIOService() {
     return (impl_->io_service_);
 }
 
+isc::auth::DataSrcClientsMgr&
+AuthSrv::getDataSrcClientsMgr() {
+    return (impl_->datasrc_clients_mgr_);
+}
+
 void
 AuthSrv::setXfrinSession(AbstractSession* xfrin_session) {
     impl_->xfrin_session_ = xfrin_session;
@@ -646,15 +631,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 the
+    // 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();
@@ -933,26 +918,6 @@ AuthSrv::destroyDDNSForwarder() {
     }
 }
 
-ClientListMapPtr
-AuthSrv::swapDataSrcClientLists(ClientListMapPtr new_lists) {
-    // TODO: Debug-build only check
-    if (!impl_->mutex_.locked()) {
-        isc_throw(isc::Unexpected, "Not locked!");
-    }
-    std::swap(new_lists, impl_->datasrc_client_lists_);
-    return (new_lists);
-}
-
-shared_ptr<ConfigurableClientList>
-AuthSrv::getDataSrcClientList(const RRClass& rrclass) {
-    return (impl_->getDataSrcClientList(rrclass));
-}
-
-util::thread::Mutex&
-AuthSrv::getDataSrcClientListMutex() const {
-    return (impl_->mutex_);
-}
-
 void
 AuthSrv::setTCPRecvTimeout(size_t timeout) {
     dnss_->setTCPRecvTimeout(timeout);

src/bin/auth/auth_srv.h

@@ -35,7 +35,9 @@
 
 #include <asiolink/asiolink.h>
 #include <server_common/portconfig.h>
+
 #include <auth/statistics.h>
+#include <auth/datasrc_clients_mgr.h>
 
 #include <boost/shared_ptr.hpp>
 
@@ -194,6 +196,11 @@ public:
     /// \brief Return pointer to the Checkin callback function
     isc::asiolink::SimpleCallback* getCheckinProvider() const { return (checkin_); }
 
+    /// \brief Return data source clients manager.
+    ///
+    /// \throw None
+    isc::auth::DataSrcClientsMgr& getDataSrcClientsMgr();
+
     /// \brief Set the communication session with a separate process for
     /// outgoing zone transfers.
     ///
@@ -303,77 +310,10 @@ public:
     /// If there was no forwarder yet, this method does nothing.
     void destroyDDNSForwarder();
 
-    /// \brief Swap the currently used set of data source client lists with
-    /// given one.
-    ///
-    /// The "set" of lists is actually given in the form of map from
-    /// RRClasses to shared pointers to isc::datasrc::ConfigurableClientList.
-    ///
-    /// This method returns the swapped set of lists, which was previously
-    /// used by the server.
-    ///
-    /// This method is intended to be used by a separate method to update
-    /// the data source configuration "at once".  The caller must hold
-    /// a lock for the mutex object returned by  \c getDataSrcClientListMutex()
-    /// before calling this method.
-    ///
-    /// The ownership of the returned pointer is transferred to the caller.
-    /// The caller is generally expected to release the resources used in
-    /// the old lists.  Note that it could take longer time if some of the
-    /// data source clients contain a large size of in-memory data.
-    ///
-    /// The caller can pass a NULL pointer.  This effectively disables
-    /// any data source for the server.
-    ///
-    /// \param new_lists Shared pointer to a new set of data source client
-    /// lists.
-    /// \return The previous set of lists.  It can be NULL.
-    isc::datasrc::ClientListMapPtr
-        swapDataSrcClientLists(isc::datasrc::ClientListMapPtr new_lists);
-
-    /// \brief Returns the currently used client list for the class.
-    ///
-    /// \param rrclass The class for which to get the list.
-    /// \return The list, or NULL if no list is set for the class.
-    boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-        getDataSrcClientList(const isc::dns::RRClass& rrclass);
-
-    /// \brief Return a mutex for the client lists.
-    ///
-    /// Background loading of data uses threads. Therefore we need to protect
-    /// the client lists by a mutex, so they don't change (or get destroyed)
-    /// during query processing. Get (and lock) this mutex whenever you do
-    /// something with the lists and keep it locked until you finish. This
-    /// is correct:
-    /// \code
-    /// {
-    ///  Mutex::Locker locker(auth->getDataSrcClientListMutex());
-    ///  boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-    ///    list(auth->getDataSrcClientList(RRClass::IN()));
-    ///  // Do some processing here
-    /// }
-    /// \endcode
-    ///
-    /// But this is not (it releases the mutex too soon):
-    /// \code
-    /// boost::shared_ptr<isc::datasrc::ConfigurableClientList> list;
-    /// {
-    ///     Mutex::Locker locker(auth->getDataSrcClientListMutex());
-    ///     list = auth->getDataSrcClientList(RRClass::IN()));
-    /// }
-    /// // Do some processing here
-    /// \endcode
-    ///
-    /// \note This method is const even if you are allowed to modify
-    ///    (lock) the mutex. It's because locking of the mutex is not really
-    ///    a modification of the server object and it is needed to protect the
-    ///    lists even on read-only operations.
-    isc::util::thread::Mutex& getDataSrcClientListMutex() const;
-
     /// \brief Sets the timeout for incoming TCP connections
     ///
     /// Incoming TCP connections that have not sent their data
-    /// withing this time are dropped.
+    /// within this time are dropped.
     ///
     /// \param timeout The timeout (in milliseconds). If se to
     /// zero, no timeouts are used, and the connection will remain

src/bin/auth/benchmarks/query_bench.cc

@@ -18,7 +18,6 @@
 #include <bench/benchmark_util.h>
 
 #include <util/buffer.h>
-#include <util/threads/sync.h>
 
 #include <dns/message.h>
 #include <dns/name.h>
@@ -33,6 +32,7 @@
 #include <auth/auth_srv.h>
 #include <auth/auth_config.h>
 #include <auth/datasrc_config.h>
+#include <auth/datasrc_clients_mgr.h>
 #include <auth/query.h>
 
 #include <asiodns/asiodns.h>
@@ -127,9 +127,9 @@ public:
                           OutputBuffer& buffer) :
         QueryBenchMark(queries, query_message, buffer)
     {
-        isc::util::thread::Mutex::Locker locker(
-                server_->getDataSrcClientListMutex());
-        server_->swapDataSrcClientLists(
+        // Note: setDataSrcClientLists() may be deprecated, but until then
+        // we use it because we want to be synchronized with the server.
+        server_->getDataSrcClientsMgr().setDataSrcClientLists(
             configureDataSource(
                 Element::fromJSON("{\"IN\":"
                                   "  [{\"type\": \"sqlite3\","
@@ -148,9 +148,7 @@ public:
                          OutputBuffer& buffer) :
         QueryBenchMark(queries, query_message, buffer)
     {
-        isc::util::thread::Mutex::Locker locker(
-                server_->getDataSrcClientListMutex());
-        server_->swapDataSrcClientLists(
+        server_->getDataSrcClientsMgr().setDataSrcClientLists(
             configureDataSource(
                 Element::fromJSON("{\"IN\":"
                                   "  [{\"type\": \"MasterFiles\","

src/bin/auth/command.cc

@@ -15,13 +15,13 @@
 #include <auth/command.h>
 #include <auth/auth_log.h>
 #include <auth/auth_srv.h>
+#include <auth/datasrc_clients_mgr.h>
 
 #include <cc/data.h>
 #include <datasrc/client_list.h>
 #include <config/ccsession.h>
 #include <exceptions/exceptions.h>
 #include <dns/rrclass.h>
-#include <util/threads/sync.h>
 
 #include <string>
 
@@ -188,14 +188,11 @@ public:
         if (!origin_elem) {
             isc_throw(AuthCommandError, "Zone origin is missing");
         }
-        Name origin(origin_elem->stringValue());
+        const Name origin(origin_elem->stringValue());
 
-        // We're going to work with the client lists. They may be used
-        // from a different thread too, protect them.
-        isc::util::thread::Mutex::Locker locker(
-            server.getDataSrcClientListMutex());
-        const boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-            list(server.getDataSrcClientList(zone_class));
+        DataSrcClientsMgr::Holder holder(server.getDataSrcClientsMgr());
+        boost::shared_ptr<ConfigurableClientList> list =
+            holder.findClientList(zone_class);
 
         if (!list) {
             isc_throw(AuthCommandError, "There's no client list for "

src/bin/auth/datasrc_clients_mgr.h

@@ -21,16 +21,20 @@
 #include <log/logger_support.h>
 #include <log/log_dbglevels.h>
 
-#include <auth/datasrc_config.h>
+#include <dns/rrclass.h>
+
 #include <cc/data.h>
+
+#include <datasrc/data_source.h>
 #include <datasrc/client_list.h>
-#include <dns/rrclass.h>
 
 #include <auth/auth_log.h>
+#include <auth/datasrc_config.h>
 
 #include <boost/array.hpp>
 #include <boost/bind.hpp>
 #include <boost/shared_ptr.hpp>
+#include <boost/noncopyable.hpp>
 
 #include <list>
 #include <utility>
@@ -84,8 +88,63 @@ typedef std::pair<CommandID, data::ConstElementPtr> Command;
 /// \c DataSrcClientsMgr.
 template <typename ThreadType, typename BuilderType, typename MutexType,
           typename CondVarType>
-class DataSrcClientsMgrBase {
+class DataSrcClientsMgrBase : boost::noncopyable {
+private:
+    typedef std::map<dns::RRClass,
+                     boost::shared_ptr<datasrc::ConfigurableClientList> >
+    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.
+        ///
+        /// \note Since the ownership isn't transferred the return value
+        /// could be a bare pointer (and it's probably better in several
+        /// points).  Unfortunately, some unit tests currently don't work
+        /// unless this method effectively shares the ownership with the
+        /// tests.  That's the only reason why we return a shared pointer
+        /// for now.  We should eventually fix it and change the return value
+        /// type (see Trac ticket #2395).  Other applications must not
+        /// assume the ownership is actually shared.
+        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 (boost::shared_ptr<datasrc::ConfigurableClientList>());
+            } else {
+                return (it->second);
+            }
+        }
+    private:
+        DataSrcClientsMgrBase& mgr_;
+        typename MutexType::Locker locker_;
+    };
+
     /// \brief Constructor.
     ///
     /// It internally invokes a separate thread and waits for further
@@ -99,6 +158,7 @@ public:
     /// \throw std::bad_alloc internal memory allocation failure.
     /// \throw isc::Unexpected general unexpected system errors.
     DataSrcClientsMgrBase() :
+        clients_map_(new ClientListsMap),
         builder_(&command_queue_, &cond_, &queue_mutex_, &clients_map_,
                  &map_mutex_),
         builder_thread_(boost::bind(&BuilderType::run, &builder_))
@@ -144,6 +204,35 @@ public:
         cleanup();              // see below
     }
 
+    /// \brief Handle new full configuration for data source clients.
+    ///
+    /// This method simply passes the new configuration to the builder
+    /// and immediately returns.  This method is basically exception free
+    /// as long as the caller passes a non NULL value for \c config_arg;
+    /// it doesn't validate the argument further.
+    ///
+    /// \brief isc::InvalidParameter config_arg is NULL.
+    /// \brief std::bad_alloc
+    ///
+    /// \param config_arg The new data source configuration.  Must not be NULL.
+    void reconfigure(data::ConstElementPtr config_arg) {
+        if (!config_arg) {
+            isc_throw(InvalidParameter, "Invalid null config argument");
+        }
+        sendCommand(datasrc_clientmgr_internal::RECONFIGURE, config_arg);
+        reconfigureHook();      // for test's customization
+    }
+
+    /// \brief Set the underlying data source client lists to new lists.
+    ///
+    /// This is provided only for some existing tests until we support a
+    /// cleaner way to use faked data source clients.  Non test code or
+    /// newer tests must not use this.
+    void setDataSrcClientLists(datasrc::ClientListMapPtr new_lists) {
+        typename MutexType::Locker locker(map_mutex_);
+        clients_map_ = new_lists;
+    }
+
 private:
     // This is expected to be called at the end of the destructor.  It
     // actually does nothing, but provides a customization point for
@@ -151,14 +240,18 @@ private:
     // state of the class.
     void cleanup() {}
 
+    // same as cleanup(), for reconfigure().
+    void reconfigureHook() {}
+
     void sendCommand(datasrc_clientmgr_internal::CommandID command,
                      data::ConstElementPtr arg)
     {
-        {
-            typename MutexType::Locker locker(queue_mutex_);
-            command_queue_.push_back(
-                datasrc_clientmgr_internal::Command(command, arg));
-        }
+        // The lock will be held until the end of this method.  Only
+        // push_back has to be protected, but we can avoid having an extra
+        // block this way.
+        typename MutexType::Locker locker(queue_mutex_);
+        command_queue_.push_back(
+            datasrc_clientmgr_internal::Command(command, arg));
         cond_.signal();
     }
 
@@ -195,7 +288,7 @@ namespace datasrc_clientmgr_internal {
 /// This class is templated so that we can test it without involving actual
 /// threads or locks.
 template <typename MutexType, typename CondVarType>
-class DataSrcClientsBuilderBase {
+class DataSrcClientsBuilderBase : boost::noncopyable {
 public:
     /// \brief Constructor.
     ///
@@ -294,13 +387,12 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::run() {
             std::list<Command> current_commands;
             {
                 // Move all new commands to local queue under the protection of
-                // queue_mutex_.  Note that list::splice() should never throw.
+                // queue_mutex_.
                 typename MutexType::Locker locker(*queue_mutex_);
                 while (command_queue_->empty()) {
                     cond_->wait(*queue_mutex_);
                 }
-                current_commands.splice(current_commands.end(),
-                                        *command_queue_);
+                current_commands.swap(*command_queue_);
             } // the lock is released here.
 
             while (keep_running && !current_commands.empty()) {
@@ -312,12 +404,12 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::run() {
         LOG_INFO(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_STOPPED);
     } catch (const std::exception& ex) {
         // We explicitly catch exceptions so we can log it as soon as possible.
-        LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED).
+        LOG_FATAL(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED).
             arg(ex.what());
-        throw;
+        assert(false);
     } catch (...) {
-        LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED);
-        throw;
+        LOG_FATAL(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED);
+        assert(false);
     }
 }
 

src/bin/auth/datasrc_config.h

@@ -15,8 +15,6 @@
 #ifndef DATASRC_CONFIG_H
 #define DATASRC_CONFIG_H
 
-#include "auth_srv.h"
-
 #include <cc/data.h>
 #include <datasrc/client_list.h>
 

src/bin/auth/main.cc

@@ -18,7 +18,6 @@
 
 #include <util/buffer.h>
 #include <util/io/socketsession.h>
-#include <util/threads/sync.h>
 
 #include <dns/message.h>
 #include <dns/messagerenderer.h>
@@ -95,32 +94,23 @@ datasrcConfigHandler(AuthSrv* server, bool* first_time,
                      const isc::config::ConfigData&)
 {
     assert(server != NULL);
-    if (config->contains("classes")) {
-        isc::datasrc::ClientListMapPtr lists;
-
-        if (*first_time) {
-            // HACK: The default is not passed to the handler in the first
-            // callback. This one will get the default (or, current value).
-            // Further updates will work the usual way.
-            assert(config_session != NULL);
-            *first_time = false;
-            lists = configureDataSource(
-                config_session->getRemoteConfigValue("data_sources",
-                                                     "classes"));
-        } else {
-            lists = configureDataSource(config->get("classes"));
-        }
 
-        // Replace the server's lists.  The returned lists will be stored
-        // in a local variable 'lists', and will be destroyed outside of
-        // the temporary block for the lock scope.  That way we can minimize
-        // the range of the critical section.
-        {
-            isc::util::thread::Mutex::Locker locker(
-                server->getDataSrcClientListMutex());
-            lists = server->swapDataSrcClientLists(lists);
-        }
-        // The previous lists are destroyed here.
+    // Note: remote config handler is requested to be exception free.
+    // While the code below is not 100% exception free, such an exception
+    // is really fatal and the server should actually stop.  So we don't
+    // bother to catch them; the exception would be propagated to the
+    // top level of the server and terminate it.
+
+    if (*first_time) {
+        // HACK: The default is not passed to the handler in the first
+        // callback. This one will get the default (or, current value).
+        // Further updates will work the usual way.
+        assert(config_session != NULL);
+        *first_time = false;
+        server->getDataSrcClientsMgr().reconfigure(
+            config_session->getRemoteConfigValue("data_sources", "classes"));
+    } else if (config->contains("classes")) {
+        server->getDataSrcClientsMgr().reconfigure(config->get("classes"));
     }
 }
 
@@ -232,10 +222,6 @@ main(int argc, char* argv[]) {
         isc::server_common::initKeyring(*config_session);
         auth_server->setTSIGKeyRing(&isc::server_common::keyring);
 
-        // Instantiate the data source clients manager.  At the moment
-        // just so we actually create it in system tests.
-        DataSrcClientsMgr datasrc_clients_mgr;
-
         // Start the data source configuration.  We pass first_time and
         // config_session for the hack described in datasrcConfigHandler.
         bool first_time = true;

src/bin/auth/tests/auth_srv_unittest.cc

@@ -39,7 +39,6 @@
 #include <auth/datasrc_config.h>
 
 #include <util/unittests/mock_socketsession.h>
-#include <util/threads/sync.h>
 #include <dns/tests/unittest_util.h>
 #include <testutils/dnsmessage_test.h>
 #include <testutils/srv_test.h>
@@ -70,6 +69,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 +726,11 @@ TEST_F(AuthSrvTest, notifyWithSessionMessageError) {
 }
 
 void
-installDataSrcClientLists(AuthSrv& server,
-                          ClientListMapPtr lists)
-{
-    thread::Mutex::Locker locker(server.getDataSrcClientListMutex());
-    server.swapDataSrcClientLists(lists);
+installDataSrcClientLists(AuthSrv& server, ClientListMapPtr 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().setDataSrcClientLists(lists);
 }
 
 void
@@ -1438,16 +1438,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));
-        ClientListMapPtr 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));
     }
+    ClientListMapPtr lists(new std::map<RRClass, ListPtr>);
+    lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
+    server.getDataSrcClientsMgr().setDataSrcClientLists(lists);
 
     createDataFromFile("nsec3query_nodnssec_fromWire.wire");
     server.processMessage(*io_message, *parse_message, *response_obuffer,
@@ -1470,14 +1470,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));
+    }
     ClientListMapPtr lists(new std::map<RRClass, ListPtr>);
     lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
-    server.swapDataSrcClientLists(lists);
+    mgr.setDataSrcClientLists(lists);
 }
 
 TEST_F(AuthSrvTest,
@@ -1784,57 +1786,4 @@ 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());
-
-    ClientListMapPtr 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());
-    // TODO: Once we have non-debug build, this one will not work, since
-    // we currently use the fact that we can't lock twice from the same
-    // thread. In the non-debug mode, this would deadlock.
-    // Skip then.
-    EXPECT_THROW({
-        isc::util::thread::Mutex::Locker l2(
-            server.getDataSrcClientListMutex());
-    }, isc::InvalidOperation);
-}
-
 }

src/bin/auth/tests/command_unittest.cc

@@ -16,8 +16,7 @@
 
 #include "datasrc_util.h"
 
-#include <util/threads/sync.h>
-
+#include <auth/auth_srv.h>
 #include <auth/command.h>
 #include <auth/datasrc_config.h>
 
@@ -56,6 +55,7 @@ using namespace isc::datasrc;
 using namespace isc::config;
 using namespace isc::util::unittests;
 using namespace isc::testutils;
+using namespace isc::auth;
 using namespace isc::auth::unittest;
 
 namespace {
@@ -174,27 +174,26 @@ TEST_F(AuthCommandTest, shutdownIncorrectPID) {
 // zones, and checks the zones are correctly loaded.
 void
 zoneChecks(AuthSrv& server) {
-    isc::util::thread::Mutex::Locker locker(
-        server.getDataSrcClientListMutex());
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
-              find(Name("ns.test1.example")).finder_->
-              find(Name("ns.test1.example"), RRType::A())->code);
-    EXPECT_EQ(ZoneFinder::NXRRSET, server.getDataSrcClientList(RRClass::IN())->
-              find(Name("ns.test1.example")).finder_->
-              find(Name("ns.test1.example"), RRType::AAAA())->code);
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
-              find(Name("ns.test2.example")).finder_->
-              find(Name("ns.test2.example"), RRType::A())->code);
-    EXPECT_EQ(ZoneFinder::NXRRSET, server.getDataSrcClientList(RRClass::IN())->
-              find(Name("ns.test2.example")).finder_->
-              find(Name("ns.test2.example"), RRType::AAAA())->code);
+    const RRClass rrclass(RRClass::IN());
+
+    DataSrcClientsMgr::Holder holder(server.getDataSrcClientsMgr());
+    EXPECT_EQ(ZoneFinder::SUCCESS,
+              holder.findClientList(rrclass)->find(Name("ns.test1.example"))
+              .finder_->find(Name("ns.test1.example"), RRType::A())->code);
+    EXPECT_EQ(ZoneFinder::NXRRSET,
+              holder.findClientList(rrclass)->find(Name("ns.test1.example")).
+              finder_->find(Name("ns.test1.example"), RRType::AAAA())->code);
+    EXPECT_EQ(ZoneFinder::SUCCESS,
+              holder.findClientList(rrclass)->find(Name("ns.test2.example")).
+              finder_->find(Name("ns.test2.example"), RRType::A())->code);
+    EXPECT_EQ(ZoneFinder::NXRRSET,
+              holder.findClientList(rrclass)->find(Name("ns.test2.example")).
+              finder_->find(Name("ns.test2.example"), RRType::AAAA())->code);
 }
 
 void
 installDataSrcClientLists(AuthSrv& server, ClientListMapPtr lists) {
-    isc::util::thread::Mutex::Locker locker(
-        server.getDataSrcClientListMutex());
-    server.swapDataSrcClientLists(lists);
+    server.getDataSrcClientsMgr().setDataSrcClientLists(lists);
 }
 
 void
@@ -223,22 +222,24 @@ configureZones(AuthSrv& server) {
 
 void
 newZoneChecks(AuthSrv& server) {
-    isc::util::thread::Mutex::Locker locker(
-        server.getDataSrcClientListMutex());
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
+    const RRClass rrclass(RRClass::IN());
+
+    DataSrcClientsMgr::Holder holder(server.getDataSrcClientsMgr());
+    EXPECT_EQ(ZoneFinder::SUCCESS, holder.findClientList(rrclass)->
               find(Name("ns.test1.example")).finder_->
               find(Name("ns.test1.example"), RRType::A())->code);
+
     // now test1.example should have ns/AAAA
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
+    EXPECT_EQ(ZoneFinder::SUCCESS, holder.findClientList(rrclass)->
               find(Name("ns.test1.example")).finder_->
               find(Name("ns.test1.example"), RRType::AAAA())->code);
 
     // test2.example shouldn't change
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
+    EXPECT_EQ(ZoneFinder::SUCCESS, holder.findClientList(rrclass)->
               find(Name("ns.test2.example")).finder_->
               find(Name("ns.test2.example"), RRType::A())->code);
     EXPECT_EQ(ZoneFinder::NXRRSET,
-              server.getDataSrcClientList(RRClass::IN())->
+              holder.findClientList(rrclass)->
               find(Name("ns.test2.example")).finder_->
               find(Name("ns.test2.example"), RRType::AAAA())->code);
 }
@@ -284,11 +285,11 @@ TEST_F(AuthCommandTest,
     installDataSrcClientLists(server_, configureDataSource(config));
 
     {
-        isc::util::thread::Mutex::Locker locker(
-            server_.getDataSrcClientListMutex());
+        DataSrcClientsMgr::Holder holder(server_.getDataSrcClientsMgr());
+
         // Check that the A record at www.example.org does not exist
         EXPECT_EQ(ZoneFinder::NXDOMAIN,
-                  server_.getDataSrcClientList(RRClass::IN())->
+                  holder.findClientList(RRClass::IN())->
                   find(Name("example.org")).finder_->
                   find(Name("www.example.org"), RRType::A())->code);
 
@@ -309,7 +310,7 @@ TEST_F(AuthCommandTest,
         sql_updater->commit();
 
         EXPECT_EQ(ZoneFinder::NXDOMAIN,
-                  server_.getDataSrcClientList(RRClass::IN())->
+                  holder.findClientList(RRClass::IN())->
                   find(Name("example.org")).finder_->
                   find(Name("www.example.org"), RRType::A())->code);
     }
@@ -321,11 +322,10 @@ TEST_F(AuthCommandTest,
     checkAnswer(0, "Successful load");
 
     {
-        isc::util::thread::Mutex::Locker locker(
-            server_.getDataSrcClientListMutex());
+        DataSrcClientsMgr::Holder holder(server_.getDataSrcClientsMgr());
         // And now it should be present too.
         EXPECT_EQ(ZoneFinder::SUCCESS,
-                  server_.getDataSrcClientList(RRClass::IN())->
+                  holder.findClientList(RRClass::IN())->
                   find(Name("example.org")).finder_->
                   find(Name("www.example.org"), RRType::A())->code);
     }
@@ -336,11 +336,10 @@ TEST_F(AuthCommandTest,
     checkAnswer(1, "example.com");
 
     {
-        isc::util::thread::Mutex::Locker locker(
-            server_.getDataSrcClientListMutex());
+        DataSrcClientsMgr::Holder holder(server_.getDataSrcClientsMgr());
         // The previous zone is not hurt in any way
         EXPECT_EQ(ZoneFinder::SUCCESS,
-                  server_.getDataSrcClientList(RRClass::IN())->
+                  holder.findClientList(RRClass::IN())->
                   find(Name("example.org")).finder_->
                   find(Name("example.org"), RRType::SOA())->code);
     }
@@ -359,11 +358,10 @@ TEST_F(AuthCommandTest,
         Element::fromJSON("{\"origin\": \"example.com\"}"));
     checkAnswer(1, "Unreadable");
 
-    isc::util::thread::Mutex::Locker locker(
-        server_.getDataSrcClientListMutex());
+    DataSrcClientsMgr::Holder holder(server_.getDataSrcClientsMgr());
     // The previous zone is not hurt in any way
     EXPECT_EQ(ZoneFinder::SUCCESS,
-              server_.getDataSrcClientList(RRClass::IN())->
+              holder.findClientList(RRClass::IN())->
               find(Name("example.org")).finder_->
               find(Name("example.org"), RRType::SOA())->code);
 }

src/bin/auth/tests/datasrc_clients_builder_unittest.cc

@@ -71,14 +71,16 @@ TEST_F(DataSrcClientsBuilderTest, runMultiCommands) {
 
 TEST_F(DataSrcClientsBuilderTest, exception) {
     // Let the noop command handler throw exceptions and see if we can see
-    // them.
+    // them.  Right now, we simply abort to prevent the system from running
+    // with half-broken state.  Eventually we should introduce a better
+    // error handling.
     command_queue.push_back(noop_cmd);
     queue_mutex.throw_from_noop = TestMutex::EXCLASS;
-    EXPECT_THROW(builder.run(), isc::Exception);
+    EXPECT_DEATH_IF_SUPPORTED({builder.run();}, "");
 
     command_queue.push_back(noop_cmd);
     queue_mutex.throw_from_noop = TestMutex::INTEGER;
-    EXPECT_THROW(builder.run(), int);
+    EXPECT_DEATH_IF_SUPPORTED({builder.run();}, "");
 }
 
 TEST_F(DataSrcClientsBuilderTest, condWait) {

src/bin/auth/tests/datasrc_clients_mgr_unittest.cc

@@ -12,8 +12,14 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+#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"
 
@@ -21,6 +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;
 
@@ -41,6 +50,30 @@ shutdownCheck() {
     EXPECT_TRUE(FakeDataSrcClientsBuilder::thread_waited);
 }
 
+// Commonly used pattern of checking member variables shared between the
+// manager and builder.
+void
+checkSharedMembers(size_t expected_queue_lock_count,
+                   size_t expected_queue_unlock_count,
+                   size_t expected_map_lock_count,
+                   size_t expected_map_unlock_count,
+                   size_t expected_cond_signal_count,
+                   size_t expected_command_queue_size)
+{
+    EXPECT_EQ(expected_queue_lock_count,
+              FakeDataSrcClientsBuilder::queue_mutex->lock_count);
+    EXPECT_EQ(expected_queue_unlock_count,
+              FakeDataSrcClientsBuilder::queue_mutex->unlock_count);
+    EXPECT_EQ(expected_map_lock_count,
+              FakeDataSrcClientsBuilder::map_mutex->lock_count);
+    EXPECT_EQ(expected_map_unlock_count,
+              FakeDataSrcClientsBuilder::map_mutex->unlock_count);
+    EXPECT_EQ(expected_cond_signal_count,
+              FakeDataSrcClientsBuilder::cond->signal_count);
+    EXPECT_EQ(expected_command_queue_size,
+              FakeDataSrcClientsBuilder::command_queue->size());
+}
+
 TEST(DataSrcClientsMgrTest, start) {
     // When we create a manager, builder's run() method should be called.
     FakeDataSrcClientsBuilder::started = false;
@@ -77,6 +110,92 @@ TEST(DataSrcClientsMgrTest, shutdownWithException) {
         });
 }
 
+TEST(DataSrcClientsMgrTest, reconfigure) {
+    TestDataSrcClientsMgr mgr;
+
+    // Check pre-command condition
+    checkSharedMembers(0, 0, 0, 0, 0, 0);
+
+    // A valid reconfigure argument
+    ConstElementPtr reconfigure_arg = Element::fromJSON(
+        "{""\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
+        "             \"cache-enable\": true}]}");
+
+    // On reconfigure(), it just send the RECONFIGURE command to the builder
+    // thread with the given argument intact.
+    mgr.reconfigure(reconfigure_arg);
+
+    // The manager should have acquired the queue lock, send RECONFIGURE
+    // command with the arg, wake up the builder thread by signal.  It doesn't
+    // touch or refer to the map, so it shouldn't acquire the map lock.
+    checkSharedMembers(1, 1, 0, 0, 1, 1);
+    const Command& cmd1 = FakeDataSrcClientsBuilder::command_queue->front();
+    EXPECT_EQ(RECONFIGURE, cmd1.first);
+    EXPECT_EQ(reconfigure_arg, cmd1.second);
+
+    // Non-null, but semantically invalid argument.  The manager doesn't do
+    // this check, so it should result in the same effect.
+    FakeDataSrcClientsBuilder::command_queue->clear();
+    reconfigure_arg = isc::data::Element::create("{ \"foo\": \"bar\" }");
+    mgr.reconfigure(reconfigure_arg);
+    checkSharedMembers(2, 2, 0, 0, 2, 1);
+    const Command& cmd2 = FakeDataSrcClientsBuilder::command_queue->front();
+    EXPECT_EQ(RECONFIGURE, cmd2.first);
+    EXPECT_EQ(reconfigure_arg, cmd2.second);
+
+    // Passing NULL argument is immediately rejected
+    EXPECT_THROW(mgr.reconfigure(ConstElementPtr()), isc::InvalidParameter);
+    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_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);
+    }
+    // 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_TRUE(holder.findClientList(RRClass::IN()));
+        EXPECT_TRUE(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_TRUE(holder.findClientList(RRClass::IN()));
+        EXPECT_FALSE(holder.findClientList(RRClass::CH()));
+    }
+
+    // Duplicate lock acquisition is prohibited (only test mgr can detect
+    // this reliably, so this test may not be that useful)
+    TestDataSrcClientsMgr::Holder holder1(mgr);
+    EXPECT_THROW(TestDataSrcClientsMgr::Holder holder2(mgr), isc::Unexpected);
+}
+
 TEST(DataSrcClientsMgrTest, realThread) {
     // Using the non-test definition with a real thread.  Just checking
     // no disruption happens.

src/bin/auth/tests/datasrc_config_unittest.cc

@@ -16,7 +16,6 @@
 
 #include <config/tests/fake_session.h>
 #include <config/ccsession.h>
-#include <util/threads/sync.h>
 
 #include <gtest/gtest.h>
 
@@ -78,12 +77,8 @@ datasrcConfigHandler(DatasrcConfigTest* fake_server, const std::string&,
 
 class DatasrcConfigTest : public ::testing::Test {
 public:
-    // These pretend to be the server
-    isc::util::thread::Mutex& getDataSrcClientListMutex() const {
-        return (mutex_);
-    }
-    void swapDataSrcClientLists(shared_ptr<std::map<dns::RRClass, ListPtr> >
-                                new_lists)
+    void setDataSrcClientLists(shared_ptr<std::map<dns::RRClass, ListPtr> >
+                               new_lists)
     {
         lists_.clear();         // first empty it
 
@@ -156,7 +151,6 @@ protected:
     const string specfile;
     std::map<RRClass, ListPtr> lists_;
     string log_;
-    mutable isc::util::thread::Mutex mutex_;
 };
 
 void
@@ -167,7 +161,7 @@ testConfigureDataSource(DatasrcConfigTest& test,
     // possible to easily look that they were called.
     shared_ptr<std::map<dns::RRClass, ListPtr> > lists =
         configureDataSourceGeneric<FakeList>(config);
-    test.swapDataSrcClientLists(lists);
+    test.setDataSrcClientLists(lists);
 }
 
 // Push there a configuration with a single list.

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

@@ -51,6 +51,11 @@ public:
     class Locker {
     public:
         Locker(TestMutex& mutex) : mutex_(mutex) {
+            if (mutex.lock_count != mutex.unlock_count) {
+                isc_throw(Unexpected,
+                          "attempt of duplicate lock acquisition");
+            }
+
             ++mutex.lock_count;
             if (mutex.lock_count > 100) { // 100 is an arbitrary choice
                 isc_throw(Unexpected,
@@ -204,6 +209,9 @@ template<>
 void
 TestDataSrcClientsMgr::cleanup();
 
+template<>
+void
+TestDataSrcClientsMgr::reconfigureHook();
 } // namespace auth
 } // namespace isc