From 26ea6b0e3045489e542b95d02052fc7565a4ca87 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Fri, 12 Oct 2012 19:44:18 -0700 Subject: [PATCH 01/22] [2205] added DataSrcClientsBuilder class, main part of configurator thread --- src/bin/auth/Makefile.am | 1 + src/bin/auth/datasrc_clients_mgr.h | 131 ++++++++++++++++++ src/bin/auth/tests/Makefile.am | 2 + .../tests/datasrc_clients_builder_unittest.cc | 87 ++++++++++++ .../auth/tests/test_datasrc_clients_mgr.cc | 31 +++++ src/bin/auth/tests/test_datasrc_clients_mgr.h | 95 +++++++++++++ 6 files changed, 347 insertions(+) create mode 100644 src/bin/auth/datasrc_clients_mgr.h create mode 100644 src/bin/auth/tests/datasrc_clients_builder_unittest.cc create mode 100644 src/bin/auth/tests/test_datasrc_clients_mgr.cc create mode 100644 src/bin/auth/tests/test_datasrc_clients_mgr.h diff --git a/src/bin/auth/Makefile.am b/src/bin/auth/Makefile.am index 9eee9d4d54..6ee6677607 100644 --- a/src/bin/auth/Makefile.am +++ b/src/bin/auth/Makefile.am @@ -55,6 +55,7 @@ b10_auth_SOURCES += auth_config.cc auth_config.h b10_auth_SOURCES += command.cc command.h b10_auth_SOURCES += common.h common.cc b10_auth_SOURCES += statistics.cc statistics.h +b10_auth_SOURCES += datasrc_clients_mgr.h b10_auth_SOURCES += datasrc_config.h datasrc_config.cc b10_auth_SOURCES += main.cc diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h new file mode 100644 index 0000000000..2962f0f9d5 --- /dev/null +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -0,0 +1,131 @@ +// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#ifndef DATASRC_CLIENTS_MGR_H +#define DATASRC_CLIENTS_MGR_H 1 + +//#include +#include + +#include + +#include +#include + +namespace isc { +namespace auth { + +namespace internal { +enum CommandID { + NOOP, ///< Do nothing. Only useful for tests + SHUTDOWN ///< Shutdown the builder. +}; +typedef std::pair Command; + +template +class DataSrcClientsBuilderBase { +public: + DataSrcClientsBuilderBase(std::list* command_queue, + CondVarType* cond, MutexType* queue_mutex) : + command_queue_(command_queue), cond_(cond), queue_mutex_(queue_mutex) + {} + + /// Not sure if we need this. It depends on test details. + /// \brief Destructor. + /// + /// This does nothing, but explicitly defined to silence 'unused variable' + /// warnings from some versions of clang++. + ///~DataSrcClientsBuilderBase() {} + + void run(); + + /// separated from run() and made public for the purpose of tests. + /// + /// \return true if it the builder should keep running; false otherwise. + bool handleCommand(const Command& command); + +private: + // NOOP command handler. We use this so tests can override it. + void doNoop() {} + + // end-in, front-out queue + std::list* command_queue_; + CondVarType* cond_; + MutexType* queue_mutex_; + //boost::shared_ptr* map; + //MutexType* data_mutex_; +}; +} + +template +class DataSrcClientsMgrBase { +public: + DataSrcClientsMgrBase() : builder_(&command_queue_, &cond_, &queue_mutex_) + {} + ~DataSrcClientsMgrBase() {} + +private: + std::list command_queue_; + CondVarType cond_; + MutexType queue_mutex_; + internal::DataSrcClientsBuilderBase builder_; +}; + +namespace internal { +template +void +DataSrcClientsBuilderBase::run() { + bool keep_running = true; + while (keep_running) { + std::list current_commands; + { + // Move all new commands to local queue under the protection of + // queue_mutex_. Note that list::splice() should never throw. + typename MutexType::Locker locker(*queue_mutex_); + while (command_queue_->empty()) { + cond_->wait(*queue_mutex_); + } + current_commands.splice(current_commands.end(), *command_queue_); + } // the lock is release here. + + while (keep_running && !current_commands.empty()) { + keep_running = handleCommand(current_commands.front()); + current_commands.pop_front(); + } + } +} + +template +bool +DataSrcClientsBuilderBase::handleCommand( + const Command& command) +{ + switch (command.first) { + case SHUTDOWN: + return (false); + case NOOP: + doNoop(); + } + return (true); +} +} + +} // namespace auth +} // namespace isc + +#endif // DATASRC_CLIENTS_MGR_H + +// Local Variables: +// mode: c++ +// End: diff --git a/src/bin/auth/tests/Makefile.am b/src/bin/auth/tests/Makefile.am index 6b9d385298..7e4257c4e1 100644 --- a/src/bin/auth/tests/Makefile.am +++ b/src/bin/auth/tests/Makefile.am @@ -51,6 +51,8 @@ run_unittests_SOURCES += command_unittest.cc run_unittests_SOURCES += common_unittest.cc run_unittests_SOURCES += query_unittest.cc run_unittests_SOURCES += statistics_unittest.cc +run_unittests_SOURCES += test_datasrc_clients_mgr.h test_datasrc_clients_mgr.cc +run_unittests_SOURCES += datasrc_clients_builder_unittest.cc run_unittests_SOURCES += datasrc_config_unittest.cc run_unittests_SOURCES += run_unittests.cc diff --git a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc new file mode 100644 index 0000000000..548621674f --- /dev/null +++ b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc @@ -0,0 +1,87 @@ +// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#include + +#include +#include "test_datasrc_clients_mgr.h" + +#include + +#include + +using isc::data::ConstElementPtr; +using namespace isc::auth::internal; + +namespace { +class DataSrcClientsBuilderTest : public ::testing::Test { +protected: + DataSrcClientsBuilderTest() : + builder(&command_queue, &cond, &queue_mutex), + cond(command_queue, delayed_command_queue), + shutdown_cmd(SHUTDOWN, ConstElementPtr()), + noop_cmd(NOOP, ConstElementPtr()) + {} + + TestDataSrcClientsBuilder builder; + std::list command_queue; // test command queue + std::list delayed_command_queue; // commands available after wait + TestCondVar cond; + TestMutex queue_mutex; + const Command shutdown_cmd; + const Command noop_cmd; +}; + +TEST_F(DataSrcClientsBuilderTest, runSingleCommand) { + // A simplest case, just to check the basic behavior. + command_queue.push_back(shutdown_cmd); + builder.run(); + EXPECT_TRUE(command_queue.empty()); + EXPECT_EQ(0, cond.wait_count); // no wait because command queue is not empty + EXPECT_EQ(1, queue_mutex.lock_count); + EXPECT_EQ(1, queue_mutex.unlock_count); +} + +TEST_F(DataSrcClientsBuilderTest, runMultiCommands) { + // Two NOOP commands followed by SHUTDOWN. We should see two doNoop() + // calls. + command_queue.push_back(noop_cmd); + command_queue.push_back(noop_cmd); + command_queue.push_back(shutdown_cmd); + builder.run(); + EXPECT_TRUE(command_queue.empty()); + EXPECT_EQ(1, queue_mutex.lock_count); + EXPECT_EQ(1, queue_mutex.unlock_count); + EXPECT_EQ(2, queue_mutex.noop_count); +} + +TEST_F(DataSrcClientsBuilderTest, condWait) { + // command_queue is originally empty, so it will require waiting on + // condvar. specialized wait() will make the delayed command available. + delayed_command_queue.push_back(shutdown_cmd); + builder.run(); + + // There should be one call to wait() + EXPECT_EQ(1, cond.wait_count); + // wait() effectively involves one more set of lock/unlock, so we have + // two in total + EXPECT_EQ(2, queue_mutex.lock_count); + EXPECT_EQ(2, queue_mutex.unlock_count); +} + +TEST_F(DataSrcClientsBuilderTest, shutdown) { + EXPECT_FALSE(builder.handleCommand(shutdown_cmd)); +} + +} // unnamed namespace diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.cc b/src/bin/auth/tests/test_datasrc_clients_mgr.cc new file mode 100644 index 0000000000..0634487c2a --- /dev/null +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.cc @@ -0,0 +1,31 @@ +// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#include "test_datasrc_clients_mgr.h" + +namespace isc { +namespace auth { +namespace internal { +template<> +void +TestDataSrcClientsBuilder::doNoop() { + ++queue_mutex_->noop_count; +} +} +} +} + +// Local Variables: +// mode: c++ +// End: diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h new file mode 100644 index 0000000000..dece856a30 --- /dev/null +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -0,0 +1,95 @@ +// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#ifndef TEST_DATASRC_CLIENTS_MGR_H +#define TEST_DATASRC_CLIENTS_MGR_H 1 + +#include + +#include + +#include + +// Below we extend the isc::auth::internal namespace to specialize +// the doNoop() method. +namespace isc { +namespace auth { +namespace internal { +class TestMutex { +public: + TestMutex() : lock_count(0), unlock_count(0), noop_count(0) {} + class Locker { + public: + Locker(TestMutex& mutex) : mutex_(mutex) { + ++mutex.lock_count; + } + ~Locker() { + ++mutex_.unlock_count; + } + private: + TestMutex& mutex_; + }; + size_t lock_count; + size_t unlock_count; + size_t noop_count; // allow doNoop() to modify this +}; + +class TestCondVar { +public: + TestCondVar(std::list& command_queue, + std::list& delayed_command_queue) : + wait_count(0), + command_queue_(command_queue), + delayed_command_queue_(delayed_command_queue) + { + } + void wait(TestMutex& mutex) { + // bookkeeping + ++mutex.unlock_count; + ++wait_count; + ++mutex.lock_count; + + // make the delayed commands available + command_queue_.splice(command_queue_.end(), delayed_command_queue_); + } + size_t wait_count; +private: + std::list& command_queue_; + std::list& delayed_command_queue_; +}; + +class TestThread { +public: + TestThread(const boost::function& /*main*/); +}; + +// Convenient shortcut +typedef DataSrcClientsBuilderBase +TestDataSrcClientsBuilder; + +// We specialize this command handler for the convenience of tests. +// It abuses our specialized Mutex to count the number of calls of this method. +template<> +void +TestDataSrcClientsBuilder::doNoop(); + +} // namespace internal +} +} + +#endif // TEST_DATASRC_CLIENTS_MGR_H + +// Local Variables: +// mode: c++ +// End: From 0c6ad26d377dfa84e4cf4ee202b8f1c912d296a1 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 15 Oct 2012 14:30:11 -0700 Subject: [PATCH 02/22] [2205] introduce DataSrcClientsMgr, tested initial startup. --- src/bin/auth/datasrc_clients_mgr.h | 18 ++++-- src/bin/auth/tests/Makefile.am | 1 + .../tests/datasrc_clients_mgr_unittest.cc | 39 ++++++++++++ .../auth/tests/test_datasrc_clients_mgr.cc | 6 ++ src/bin/auth/tests/test_datasrc_clients_mgr.h | 60 +++++++++++++++---- 5 files changed, 108 insertions(+), 16 deletions(-) create mode 100644 src/bin/auth/tests/datasrc_clients_mgr_unittest.cc diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 2962f0f9d5..2afd3cc57c 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -15,11 +15,12 @@ #ifndef DATASRC_CLIENTS_MGR_H #define DATASRC_CLIENTS_MGR_H 1 -//#include #include #include +#include + #include #include @@ -68,18 +69,27 @@ private: }; } -template +template class DataSrcClientsMgrBase { public: - DataSrcClientsMgrBase() : builder_(&command_queue_, &cond_, &queue_mutex_) + DataSrcClientsMgrBase() : + builder_(&command_queue_, &cond_, &queue_mutex_), + builder_thread_(boost::bind(&BuilderType::run, &builder_)) {} ~DataSrcClientsMgrBase() {} +#ifdef notyet + void shutdown() { + builder_thread_.wait(); + } +#endif private: std::list command_queue_; CondVarType cond_; MutexType queue_mutex_; - internal::DataSrcClientsBuilderBase builder_; + BuilderType builder_; + ThreadType builder_thread_; }; namespace internal { diff --git a/src/bin/auth/tests/Makefile.am b/src/bin/auth/tests/Makefile.am index 7e4257c4e1..3138c27aee 100644 --- a/src/bin/auth/tests/Makefile.am +++ b/src/bin/auth/tests/Makefile.am @@ -53,6 +53,7 @@ run_unittests_SOURCES += query_unittest.cc run_unittests_SOURCES += statistics_unittest.cc run_unittests_SOURCES += test_datasrc_clients_mgr.h test_datasrc_clients_mgr.cc run_unittests_SOURCES += datasrc_clients_builder_unittest.cc +run_unittests_SOURCES += datasrc_clients_mgr_unittest.cc run_unittests_SOURCES += datasrc_config_unittest.cc run_unittests_SOURCES += run_unittests.cc diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc new file mode 100644 index 0000000000..aa5dc93368 --- /dev/null +++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc @@ -0,0 +1,39 @@ +// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#include + +#include +#include "test_datasrc_clients_mgr.h" + +#include + +#include + +using namespace isc::auth; +using namespace isc::auth::internal; + +namespace { +class DataSrcClientsMgrTest : public ::testing::Test { +}; + +TEST_F(DataSrcClientsMgrTest, start) { + // When we create a manager, builder's run() method should be called. + FakeDataSrcClientsBuilder::started = false; + TestDataSrcClientsMgr mgr; + EXPECT_TRUE(FakeDataSrcClientsBuilder::started); + EXPECT_TRUE(FakeDataSrcClientsBuilder::command_queue->empty()); +} + +} // unnamed namespace diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.cc b/src/bin/auth/tests/test_datasrc_clients_mgr.cc index 0634487c2a..9d3565e80e 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.cc +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.cc @@ -16,6 +16,12 @@ namespace isc { namespace auth { +// Define static DataSrcClientsBuilder member variables. +bool FakeDataSrcClientsBuilder::started = false; +std::list* FakeDataSrcClientsBuilder::command_queue = NULL; +internal::TestCondVar* FakeDataSrcClientsBuilder::cond = NULL; +internal::TestMutex* FakeDataSrcClientsBuilder::queue_mutex = NULL; + namespace internal { template<> void diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h index dece856a30..57ef57205f 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.h +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -47,11 +47,14 @@ public: class TestCondVar { public: + TestCondVar() : wait_count(0), command_queue_(NULL), + delayed_command_queue_(NULL) + {} TestCondVar(std::list& command_queue, std::list& delayed_command_queue) : wait_count(0), - command_queue_(command_queue), - delayed_command_queue_(delayed_command_queue) + command_queue_(&command_queue), + delayed_command_queue_(&delayed_command_queue) { } void wait(TestMutex& mutex) { @@ -61,17 +64,12 @@ public: ++mutex.lock_count; // make the delayed commands available - command_queue_.splice(command_queue_.end(), delayed_command_queue_); + command_queue_->splice(command_queue_->end(), *delayed_command_queue_); } size_t wait_count; private: - std::list& command_queue_; - std::list& delayed_command_queue_; -}; - -class TestThread { -public: - TestThread(const boost::function& /*main*/); + std::list* command_queue_; + std::list* delayed_command_queue_; }; // Convenient shortcut @@ -85,8 +83,46 @@ void TestDataSrcClientsBuilder::doNoop(); } // namespace internal -} -} + +// A specialization of DataSrcClientsBuilder that allows tests to inspect +// its internal states via static class variables. Using static is suboptimal, +// but DataSrcClientsMgr is highly encapsulated, this seems to be the best +// possible compromise. +class FakeDataSrcClientsBuilder { +public: + FakeDataSrcClientsBuilder(std::list* command_queue, + internal::TestCondVar* cond, + internal::TestMutex* queue_mutex) + { + FakeDataSrcClientsBuilder::started = false; + FakeDataSrcClientsBuilder::command_queue = command_queue; + FakeDataSrcClientsBuilder::cond = cond; + FakeDataSrcClientsBuilder::queue_mutex = queue_mutex; + } + void run() { + FakeDataSrcClientsBuilder::started = true; + } + + static bool started; + static std::list* command_queue; + static internal::TestCondVar* cond; + static internal::TestMutex* queue_mutex; +}; + +class TestThread { +public: + TestThread(const boost::function& main) { + main(); + } +}; + +// Convenient shortcut +typedef DataSrcClientsMgrBase +TestDataSrcClientsMgr; + +} // namespace auth +} // namespace isc #endif // TEST_DATASRC_CLIENTS_MGR_H From d2009fd950bc48ec3a2294a9064c4fbd9bfa0f79 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 15 Oct 2012 15:06:21 -0700 Subject: [PATCH 03/22] [2205] supported manager's shutdown command --- src/bin/auth/datasrc_clients_mgr.h | 9 ++++++-- .../tests/datasrc_clients_mgr_unittest.cc | 22 +++++++++++++++++++ .../auth/tests/test_datasrc_clients_mgr.cc | 1 + src/bin/auth/tests/test_datasrc_clients_mgr.h | 16 +++++++++++++- 4 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 2afd3cc57c..6b5d0172be 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -78,11 +78,16 @@ public: builder_thread_(boost::bind(&BuilderType::run, &builder_)) {} ~DataSrcClientsMgrBase() {} -#ifdef notyet void shutdown() { + { + using namespace internal; + typename MutexType::Locker locker(queue_mutex_); + command_queue_.push_back(Command(SHUTDOWN, + data::ConstElementPtr())); + } + cond_.signal(); builder_thread_.wait(); } -#endif private: std::list command_queue_; diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc index aa5dc93368..00e6926299 100644 --- a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc @@ -36,4 +36,26 @@ TEST_F(DataSrcClientsMgrTest, start) { EXPECT_TRUE(FakeDataSrcClientsBuilder::command_queue->empty()); } +TEST_F(DataSrcClientsMgrTest, shutdown) { + // Invoke shutdown on the manager. + TestDataSrcClientsMgr mgr; + EXPECT_TRUE(FakeDataSrcClientsBuilder::started); + + // Check pre-command conditions + EXPECT_EQ(0, FakeDataSrcClientsBuilder::cond->signal_count); + EXPECT_FALSE(FakeDataSrcClientsBuilder::thread_waited); + + mgr.shutdown(); + + // The manager should have acquired the lock, put a SHUTDOWN command + // to the queue, and should have signaled the builder. + EXPECT_EQ(1, FakeDataSrcClientsBuilder::queue_mutex->lock_count); + EXPECT_EQ(1, FakeDataSrcClientsBuilder::cond->signal_count); + EXPECT_EQ(1, FakeDataSrcClientsBuilder::command_queue->size()); + const Command& cmd = FakeDataSrcClientsBuilder::command_queue->front(); + EXPECT_EQ(SHUTDOWN, cmd.first); + EXPECT_FALSE(cmd.second); + EXPECT_TRUE(FakeDataSrcClientsBuilder::thread_waited); +} + } // unnamed namespace diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.cc b/src/bin/auth/tests/test_datasrc_clients_mgr.cc index 9d3565e80e..494fb6cad8 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.cc +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.cc @@ -21,6 +21,7 @@ bool FakeDataSrcClientsBuilder::started = false; std::list* FakeDataSrcClientsBuilder::command_queue = NULL; internal::TestCondVar* FakeDataSrcClientsBuilder::cond = NULL; internal::TestMutex* FakeDataSrcClientsBuilder::queue_mutex = NULL; +bool FakeDataSrcClientsBuilder::thread_waited = false; namespace internal { template<> diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h index 57ef57205f..618640edb4 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.h +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -47,7 +47,7 @@ public: class TestCondVar { public: - TestCondVar() : wait_count(0), command_queue_(NULL), + TestCondVar() : wait_count(0), signal_count(0), command_queue_(NULL), delayed_command_queue_(NULL) {} TestCondVar(std::list& command_queue, @@ -66,7 +66,11 @@ public: // make the delayed commands available command_queue_->splice(command_queue_->end(), *delayed_command_queue_); } + void signal() { + ++signal_count; + } size_t wait_count; + size_t signal_count; private: std::list* command_queue_; std::list* delayed_command_queue_; @@ -98,15 +102,22 @@ public: FakeDataSrcClientsBuilder::command_queue = command_queue; FakeDataSrcClientsBuilder::cond = cond; FakeDataSrcClientsBuilder::queue_mutex = queue_mutex; + FakeDataSrcClientsBuilder::thread_waited = false; } void run() { FakeDataSrcClientsBuilder::started = true; } + // true iff a builder has started. static bool started; + + // These three correspond to the resource shared with the manager. static std::list* command_queue; static internal::TestCondVar* cond; static internal::TestMutex* queue_mutex; + + // true iff the manager waited on the thread running the builder. + static bool thread_waited; }; class TestThread { @@ -114,6 +125,9 @@ public: TestThread(const boost::function& main) { main(); } + void wait() { + FakeDataSrcClientsBuilder::thread_waited = true; + } }; // Convenient shortcut From b4ec1b3e236ae2dabfbf6b0fb9c182da27f52596 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 15 Oct 2012 15:52:48 -0700 Subject: [PATCH 04/22] [2205] (unrelated small fix) don't define constants in .h. see also #2358. --- src/bin/auth/auth_log.cc | 6 ++++++ src/bin/auth/auth_log.h | 10 +++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/bin/auth/auth_log.cc b/src/bin/auth/auth_log.cc index d41eaeab77..fae7bd35a3 100644 --- a/src/bin/auth/auth_log.cc +++ b/src/bin/auth/auth_log.cc @@ -21,6 +21,12 @@ namespace auth { isc::log::Logger auth_logger("auth"); +const int DBG_AUTH_START = DBGLVL_START_SHUT; +const int DBG_AUTH_SHUT = DBGLVL_START_SHUT; +const int DBG_AUTH_OPS = DBGLVL_COMMAND; +const int DBG_AUTH_DETAIL = DBGLVL_TRACE_BASIC; +const int DBG_AUTH_MESSAGES = DBGLVL_TRACE_DETAIL_DATA; + } // namespace auth } // namespace isc diff --git a/src/bin/auth/auth_log.h b/src/bin/auth/auth_log.h index 33d4432877..7fb3a2d72a 100644 --- a/src/bin/auth/auth_log.h +++ b/src/bin/auth/auth_log.h @@ -28,21 +28,21 @@ namespace auth { /// output. // Debug messages indicating normal startup are logged at this debug level. -const int DBG_AUTH_START = DBGLVL_START_SHUT; +extern const int DBG_AUTH_START; // Debug messages upon shutdown -const int DBG_AUTH_SHUT = DBGLVL_START_SHUT; +extern const int DBG_AUTH_SHUT; // Debug level used to log setting information (such as configuration changes). -const int DBG_AUTH_OPS = DBGLVL_COMMAND; +extern const int DBG_AUTH_OPS; // Trace detailed operations, including errors raised when processing invalid // packets. (These are not logged at severities of WARN or higher for fear // that a set of deliberately invalid packets set to the authoritative server // could overwhelm the logging.) -const int DBG_AUTH_DETAIL = DBGLVL_TRACE_BASIC; +extern const int DBG_AUTH_DETAIL; // This level is used to log the contents of packets received and sent. -const int DBG_AUTH_MESSAGES = DBGLVL_TRACE_DETAIL_DATA; +extern const int DBG_AUTH_MESSAGES; /// Define the logger for the "auth" module part of b10-auth. We could define /// a logger in each file, but we would want to define a common name to avoid From ae89f6e491d018953ac03d1810c1b467564c281b Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 15 Oct 2012 16:04:46 -0700 Subject: [PATCH 05/22] [2205] added a test case using a real thread --- src/bin/auth/auth_messages.mes | 6 +++++ src/bin/auth/datasrc_clients_mgr.h | 27 ++++++++++++++++++- .../tests/datasrc_clients_mgr_unittest.cc | 14 ++++++---- 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index ae7be1e3ed..c57240ecc4 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -268,3 +268,9 @@ This is a debug message output during the processing of a NOTIFY request. The zone manager component has been informed of the request, but has returned an error response (which is included in the message). The NOTIFY request will not be honored. + +% AUTH_DATASRC_CLIENT_BUILDER_STARTED data source builder thread started + +% AUTH_DATASRC_CLIENT_BUILDER_STOPPED data source builder thread stopped + +% AUTH_DATASRC_CLIENT_BUILDER_COMMAND data source builder received command, ID: %1 diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 6b5d0172be..794b515b3e 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -15,10 +15,16 @@ #ifndef DATASRC_CLIENTS_MGR_H #define DATASRC_CLIENTS_MGR_H 1 +#include #include +#include +#include + #include +#include + #include #include @@ -67,6 +73,10 @@ private: //boost::shared_ptr* map; //MutexType* data_mutex_; }; + +// Shortcut typedef for normal use +typedef DataSrcClientsBuilderBase +DataSrcClientsBuilder; } template void DataSrcClientsBuilderBase::run() { + LOG_INFO(auth_logger, AUTH_DATASRC_CLIENT_BUILDER_STARTED); + bool keep_running = true; while (keep_running) { std::list current_commands; @@ -119,6 +131,8 @@ DataSrcClientsBuilderBase::run() { current_commands.pop_front(); } } + + LOG_INFO(auth_logger, AUTH_DATASRC_CLIENT_BUILDER_STOPPED); } template @@ -126,6 +140,9 @@ bool DataSrcClientsBuilderBase::handleCommand( const Command& command) { + LOG_DEBUG(auth_logger, DBGLVL_TRACE_BASIC, + AUTH_DATASRC_CLIENT_BUILDER_COMMAND).arg(command.first); + switch (command.first) { case SHUTDOWN: return (false); @@ -134,8 +151,16 @@ DataSrcClientsBuilderBase::handleCommand( } return (true); } -} +} // namespace internal +/// \brief Shortcut type for normal data source clients manager. +/// +/// In fact, for non test applications this is the only type of this kind +/// to be considered. +typedef DataSrcClientsMgrBase +DataSrcClientsMgr; } // namespace auth } // namespace isc diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc index 00e6926299..c58494ad70 100644 --- a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc @@ -25,10 +25,7 @@ using namespace isc::auth; using namespace isc::auth::internal; namespace { -class DataSrcClientsMgrTest : public ::testing::Test { -}; - -TEST_F(DataSrcClientsMgrTest, start) { +TEST(DataSrcClientsMgrTest, start) { // When we create a manager, builder's run() method should be called. FakeDataSrcClientsBuilder::started = false; TestDataSrcClientsMgr mgr; @@ -36,7 +33,7 @@ TEST_F(DataSrcClientsMgrTest, start) { EXPECT_TRUE(FakeDataSrcClientsBuilder::command_queue->empty()); } -TEST_F(DataSrcClientsMgrTest, shutdown) { +TEST(DataSrcClientsMgrTest, shutdown) { // Invoke shutdown on the manager. TestDataSrcClientsMgr mgr; EXPECT_TRUE(FakeDataSrcClientsBuilder::started); @@ -58,4 +55,11 @@ TEST_F(DataSrcClientsMgrTest, shutdown) { EXPECT_TRUE(FakeDataSrcClientsBuilder::thread_waited); } +TEST(DataSrcClientsMgrTest, realThread) { + // Using the non-test definition with a real thread. Just checking + // no disruption happens. + DataSrcClientsMgr mgr; + mgr.shutdown(); +} + } // unnamed namespace From f05b0be9d267baab1003963e231a3511dfcd7d5e Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 15 Oct 2012 16:10:12 -0700 Subject: [PATCH 06/22] [2205] small refactoring: extracting send-command part to a thread we'll use this pattern for other commands. --- src/bin/auth/datasrc_clients_mgr.h | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 794b515b3e..6b0384cfb9 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -89,17 +89,19 @@ public: {} ~DataSrcClientsMgrBase() {} void shutdown() { - { - using namespace internal; - typename MutexType::Locker locker(queue_mutex_); - command_queue_.push_back(Command(SHUTDOWN, - data::ConstElementPtr())); - } - cond_.signal(); + sendCommand(internal::SHUTDOWN, data::ConstElementPtr()); builder_thread_.wait(); } private: + void sendCommand(internal::CommandID command, data::ConstElementPtr arg) { + { + typename MutexType::Locker locker(queue_mutex_); + command_queue_.push_back(internal::Command(command, arg)); + } + cond_.signal(); + } + std::list command_queue_; CondVarType cond_; MutexType queue_mutex_; From 16d31a6793cedc28c8acb1bebf4c891c039321cb Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 15 Oct 2012 16:31:18 -0700 Subject: [PATCH 07/22] [2205] handle the case where manager is destroyed without shutdown() --- src/bin/auth/auth_messages.mes | 8 +++-- src/bin/auth/datasrc_clients_mgr.h | 25 +++++++++---- .../tests/datasrc_clients_mgr_unittest.cc | 36 ++++++++++++------- 3 files changed, 47 insertions(+), 22 deletions(-) diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index c57240ecc4..c2389df255 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -269,8 +269,10 @@ request. The zone manager component has been informed of the request, but has returned an error response (which is included in the message). The NOTIFY request will not be honored. -% AUTH_DATASRC_CLIENT_BUILDER_STARTED data source builder thread started +% AUTH_DATASRC_CLIENTS_BUILDER_STARTED data source builder thread started -% AUTH_DATASRC_CLIENT_BUILDER_STOPPED data source builder thread stopped +% AUTH_DATASRC_CLIENTS_BUILDER_STOPPED data source builder thread stopped -% AUTH_DATASRC_CLIENT_BUILDER_COMMAND data source builder received command, ID: %1 +% AUTH_DATASRC_CLIENTS_BUILDER_COMMAND data source builder received command, ID: %1 + +% AUTH_DATASRC_CLIENTS_MANAGER_UNEXPECTED_STOP data source clients manager stopped unexpectedly, leaving the builder thread diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 6b0384cfb9..3374b21db3 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -26,6 +26,7 @@ #include #include +#include #include #include @@ -85,12 +86,22 @@ class DataSrcClientsMgrBase { public: DataSrcClientsMgrBase() : builder_(&command_queue_, &cond_, &queue_mutex_), - builder_thread_(boost::bind(&BuilderType::run, &builder_)) + builder_thread_(new ThreadType(boost::bind(&BuilderType::run, + &builder_))) {} - ~DataSrcClientsMgrBase() {} + ~DataSrcClientsMgrBase() { + if (builder_thread_) { + // An unexpected case. The manager is being destroyed without + // a prior shutdown(). We notify the builder to minimize the risk + // of leaving it as a zombie, but doesn't wait to avoid hangup. + LOG_INFO(auth_logger, AUTH_DATASRC_CLIENTS_MANAGER_UNEXPECTED_STOP); + sendCommand(internal::SHUTDOWN, data::ConstElementPtr()); + } + } void shutdown() { sendCommand(internal::SHUTDOWN, data::ConstElementPtr()); - builder_thread_.wait(); + builder_thread_->wait(); + builder_thread_.reset(); } private: @@ -106,14 +117,14 @@ private: CondVarType cond_; MutexType queue_mutex_; BuilderType builder_; - ThreadType builder_thread_; + boost::scoped_ptr builder_thread_; }; namespace internal { template void DataSrcClientsBuilderBase::run() { - LOG_INFO(auth_logger, AUTH_DATASRC_CLIENT_BUILDER_STARTED); + LOG_INFO(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_STARTED); bool keep_running = true; while (keep_running) { @@ -134,7 +145,7 @@ DataSrcClientsBuilderBase::run() { } } - LOG_INFO(auth_logger, AUTH_DATASRC_CLIENT_BUILDER_STOPPED); + LOG_INFO(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_STOPPED); } template @@ -143,7 +154,7 @@ DataSrcClientsBuilderBase::handleCommand( const Command& command) { LOG_DEBUG(auth_logger, DBGLVL_TRACE_BASIC, - AUTH_DATASRC_CLIENT_BUILDER_COMMAND).arg(command.first); + AUTH_DATASRC_CLIENTS_BUILDER_COMMAND).arg(command.first); switch (command.first) { case SHUTDOWN: diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc index c58494ad70..cfff9201bb 100644 --- a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc @@ -25,12 +25,32 @@ using namespace isc::auth; using namespace isc::auth::internal; namespace { +void +shutdownCheck() { + // Check for common points on shutdown. The manager should have acquired + // the lock, put a SHUTDOWN command to the queue, and should have signaled + // the builder. + EXPECT_EQ(1, FakeDataSrcClientsBuilder::queue_mutex->lock_count); + EXPECT_EQ(1, FakeDataSrcClientsBuilder::cond->signal_count); + EXPECT_EQ(1, FakeDataSrcClientsBuilder::command_queue->size()); + const Command& cmd = FakeDataSrcClientsBuilder::command_queue->front(); + EXPECT_EQ(SHUTDOWN, cmd.first); + EXPECT_FALSE(cmd.second); // no argument +} + TEST(DataSrcClientsMgrTest, start) { // When we create a manager, builder's run() method should be called. FakeDataSrcClientsBuilder::started = false; - TestDataSrcClientsMgr mgr; - EXPECT_TRUE(FakeDataSrcClientsBuilder::started); - EXPECT_TRUE(FakeDataSrcClientsBuilder::command_queue->empty()); + { + TestDataSrcClientsMgr mgr; + EXPECT_TRUE(FakeDataSrcClientsBuilder::started); + EXPECT_TRUE(FakeDataSrcClientsBuilder::command_queue->empty()); + } + + // We stopped the manager implicitly (without shutdown()). In this case + // the builder should be notified but the manager didn't wait. + shutdownCheck(); + EXPECT_FALSE(FakeDataSrcClientsBuilder::thread_waited); } TEST(DataSrcClientsMgrTest, shutdown) { @@ -43,15 +63,7 @@ TEST(DataSrcClientsMgrTest, shutdown) { EXPECT_FALSE(FakeDataSrcClientsBuilder::thread_waited); mgr.shutdown(); - - // The manager should have acquired the lock, put a SHUTDOWN command - // to the queue, and should have signaled the builder. - EXPECT_EQ(1, FakeDataSrcClientsBuilder::queue_mutex->lock_count); - EXPECT_EQ(1, FakeDataSrcClientsBuilder::cond->signal_count); - EXPECT_EQ(1, FakeDataSrcClientsBuilder::command_queue->size()); - const Command& cmd = FakeDataSrcClientsBuilder::command_queue->front(); - EXPECT_EQ(SHUTDOWN, cmd.first); - EXPECT_FALSE(cmd.second); + shutdownCheck(); EXPECT_TRUE(FakeDataSrcClientsBuilder::thread_waited); } From 15724e5de168271303268a09627c3f197833f20d Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 15 Oct 2012 17:35:07 -0700 Subject: [PATCH 08/22] [2205] catch exceptions inside builder and log it. --- src/bin/auth/auth_messages.mes | 4 ++ src/bin/auth/datasrc_clients_mgr.h | 45 ++++++++++++------- .../tests/datasrc_clients_builder_unittest.cc | 12 +++++ .../tests/datasrc_clients_mgr_unittest.cc | 6 +++ .../auth/tests/test_datasrc_clients_mgr.cc | 11 +++++ src/bin/auth/tests/test_datasrc_clients_mgr.h | 30 ++++++++++++- 6 files changed, 90 insertions(+), 18 deletions(-) diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index c2389df255..5eb9572cd2 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -276,3 +276,7 @@ NOTIFY request will not be honored. % AUTH_DATASRC_CLIENTS_BUILDER_COMMAND data source builder received command, ID: %1 % AUTH_DATASRC_CLIENTS_MANAGER_UNEXPECTED_STOP data source clients manager stopped unexpectedly, leaving the builder thread + +% AUTH_DATASRC_CLIENTS_BUILDER_FAILED data source builder thread stopped due to an exception: %1 + +% AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED data source builder thread stopped due to an unexpected exception diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 3374b21db3..db55fc6492 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -126,26 +126,37 @@ void DataSrcClientsBuilderBase::run() { LOG_INFO(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_STARTED); - bool keep_running = true; - while (keep_running) { - std::list current_commands; - { - // Move all new commands to local queue under the protection of - // queue_mutex_. Note that list::splice() should never throw. - typename MutexType::Locker locker(*queue_mutex_); - while (command_queue_->empty()) { - cond_->wait(*queue_mutex_); + try { + bool keep_running = true; + while (keep_running) { + std::list current_commands; + { + // Move all new commands to local queue under the protection of + // queue_mutex_. Note that list::splice() should never throw. + typename MutexType::Locker locker(*queue_mutex_); + while (command_queue_->empty()) { + cond_->wait(*queue_mutex_); + } + current_commands.splice(current_commands.end(), + *command_queue_); + } // the lock is release here. + + while (keep_running && !current_commands.empty()) { + keep_running = handleCommand(current_commands.front()); + current_commands.pop_front(); } - current_commands.splice(current_commands.end(), *command_queue_); - } // the lock is release here. - - while (keep_running && !current_commands.empty()) { - keep_running = handleCommand(current_commands.front()); - current_commands.pop_front(); } - } - LOG_INFO(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_STOPPED); + 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). + arg(ex.what()); + throw; + } catch (...) { + LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED); + throw; + } } template diff --git a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc index 548621674f..53491291d3 100644 --- a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc @@ -66,6 +66,18 @@ TEST_F(DataSrcClientsBuilderTest, runMultiCommands) { EXPECT_EQ(2, queue_mutex.noop_count); } +TEST_F(DataSrcClientsBuilderTest, exception) { + // Let the noop command handler throw exceptions and see if we can see + // them. + command_queue.push_back(noop_cmd); + queue_mutex.throw_from_noop = TestMutex::EXCLASS; + EXPECT_THROW(builder.run(), isc::Exception); + + command_queue.push_back(noop_cmd); + queue_mutex.throw_from_noop = TestMutex::INTEGER; + EXPECT_THROW(builder.run(), int); +} + TEST_F(DataSrcClientsBuilderTest, condWait) { // command_queue is originally empty, so it will require waiting on // condvar. specialized wait() will make the delayed command available. diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc index cfff9201bb..d20d5845b2 100644 --- a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc @@ -67,6 +67,12 @@ TEST(DataSrcClientsMgrTest, shutdown) { EXPECT_TRUE(FakeDataSrcClientsBuilder::thread_waited); } +TEST(DataSrcClientsMgrTest, shutdownWithException) { + TestDataSrcClientsMgr mgr; + FakeDataSrcClientsBuilder::thread_throw_on_wait = true; + EXPECT_THROW(mgr.shutdown(), isc::Unexpected); +} + TEST(DataSrcClientsMgrTest, realThread) { // Using the non-test definition with a real thread. Just checking // no disruption happens. diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.cc b/src/bin/auth/tests/test_datasrc_clients_mgr.cc index 494fb6cad8..1c9fcfa745 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.cc +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.cc @@ -12,6 +12,8 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. +#include + #include "test_datasrc_clients_mgr.h" namespace isc { @@ -22,12 +24,21 @@ std::list* FakeDataSrcClientsBuilder::command_queue = NULL; internal::TestCondVar* FakeDataSrcClientsBuilder::cond = NULL; internal::TestMutex* FakeDataSrcClientsBuilder::queue_mutex = NULL; bool FakeDataSrcClientsBuilder::thread_waited = false; +bool FakeDataSrcClientsBuilder::thread_throw_on_wait = false; namespace internal { template<> void TestDataSrcClientsBuilder::doNoop() { ++queue_mutex_->noop_count; + switch (queue_mutex_->throw_from_noop) { + case TestMutex::NONE: + break; // no throw + case TestMutex::EXCLASS: + isc_throw(Exception, "test exception"); + case TestMutex::INTEGER: + throw 42; + } } } } diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h index 618640edb4..cc221fe455 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.h +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -15,6 +15,8 @@ #ifndef TEST_DATASRC_CLIENTS_MGR_H #define TEST_DATASRC_CLIENTS_MGR_H 1 +#include + #include #include @@ -28,11 +30,23 @@ namespace auth { namespace internal { class TestMutex { public: - TestMutex() : lock_count(0), unlock_count(0), noop_count(0) {} + // for throw_from_noop. + // None: no throw from specialized doNoop() + // EXCLASS: throw some exception class object + // INTEGER: throw an integer + enum ExceptionFromNoop { NONE, EXCLASS, INTEGER }; + + TestMutex() : lock_count(0), unlock_count(0), noop_count(0), + throw_from_noop(NONE) + {} class Locker { public: Locker(TestMutex& mutex) : mutex_(mutex) { ++mutex.lock_count; + if (mutex.lock_count > 100) { // 100 is an arbitrary choice + isc_throw(Unexpected, + "too many test mutex count, likely a bug in test"); + } } ~Locker() { ++mutex_.unlock_count; @@ -43,6 +57,7 @@ public: size_t lock_count; size_t unlock_count; size_t noop_count; // allow doNoop() to modify this + ExceptionFromNoop throw_from_noop; // test can set this to control doNoop }; class TestCondVar { @@ -63,6 +78,11 @@ public: ++wait_count; ++mutex.lock_count; + if (wait_count > 100) { // 100 is an arbitrary choice + isc_throw(Unexpected, + "too many cond wait count, likely a bug in test"); + } + // make the delayed commands available command_queue_->splice(command_queue_->end(), *delayed_command_queue_); } @@ -103,6 +123,7 @@ public: FakeDataSrcClientsBuilder::cond = cond; FakeDataSrcClientsBuilder::queue_mutex = queue_mutex; FakeDataSrcClientsBuilder::thread_waited = false; + FakeDataSrcClientsBuilder::thread_throw_on_wait = false; } void run() { FakeDataSrcClientsBuilder::started = true; @@ -118,6 +139,10 @@ public: // true iff the manager waited on the thread running the builder. static bool thread_waited; + + // If set to true by a test, TestThread::wait() throws an exception + // exception. + static bool thread_throw_on_wait; }; class TestThread { @@ -127,6 +152,9 @@ public: } void wait() { FakeDataSrcClientsBuilder::thread_waited = true; + if (FakeDataSrcClientsBuilder::thread_throw_on_wait) { + isc_throw(Unexpected, "for test"); + } } }; From 5bf4005eb7dd34a08b6070bc17f0e416448062d2 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 15 Oct 2012 20:46:30 -0700 Subject: [PATCH 09/22] [2205] cover the case where Thread.wait() throws. --- src/bin/auth/auth_messages.mes | 2 + src/bin/auth/datasrc_clients_mgr.h | 7 +++- .../tests/datasrc_clients_mgr_unittest.cc | 11 ++++- .../auth/tests/test_datasrc_clients_mgr.cc | 4 +- src/bin/auth/tests/test_datasrc_clients_mgr.h | 40 +++++++++++-------- 5 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index 5eb9572cd2..7a6b6858c6 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -280,3 +280,5 @@ NOTIFY request will not be honored. % AUTH_DATASRC_CLIENTS_BUILDER_FAILED data source builder thread stopped due to an exception: %1 % AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED data source builder thread stopped due to an unexpected exception + +% AUTH_DATASRC_CLIENTS_SHUTDOWN_ERROR error on waiting for data source builder thread: %1 diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index db55fc6492..b396bd5584 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -100,7 +100,12 @@ public: } void shutdown() { sendCommand(internal::SHUTDOWN, data::ConstElementPtr()); - builder_thread_->wait(); + try { + builder_thread_->wait(); + } catch (const util::thread::Thread::UncaughtException& ex) { + LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_SHUTDOWN_ERROR). + arg(ex.what()); + } builder_thread_.reset(); } diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc index d20d5845b2..b27db22586 100644 --- a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc @@ -67,9 +67,18 @@ TEST(DataSrcClientsMgrTest, shutdown) { EXPECT_TRUE(FakeDataSrcClientsBuilder::thread_waited); } +TEST(DataSrcClientsMgrTest, shutdownWithUncaughtException) { + // Emulating the case when the builder exists on exception. shutdown() + // will encounter UncaughtException exception and catch it. + TestDataSrcClientsMgr mgr; + FakeDataSrcClientsBuilder::thread_throw_on_wait = + FakeDataSrcClientsBuilder::THROW_UNCAUGHT_EX; + EXPECT_NO_THROW(mgr.shutdown()); +} TEST(DataSrcClientsMgrTest, shutdownWithException) { TestDataSrcClientsMgr mgr; - FakeDataSrcClientsBuilder::thread_throw_on_wait = true; + FakeDataSrcClientsBuilder::thread_throw_on_wait = + FakeDataSrcClientsBuilder::THROW_OTHER; EXPECT_THROW(mgr.shutdown(), isc::Unexpected); } diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.cc b/src/bin/auth/tests/test_datasrc_clients_mgr.cc index 1c9fcfa745..da318f3453 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.cc +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.cc @@ -24,7 +24,9 @@ std::list* FakeDataSrcClientsBuilder::command_queue = NULL; internal::TestCondVar* FakeDataSrcClientsBuilder::cond = NULL; internal::TestMutex* FakeDataSrcClientsBuilder::queue_mutex = NULL; bool FakeDataSrcClientsBuilder::thread_waited = false; -bool FakeDataSrcClientsBuilder::thread_throw_on_wait = false; +FakeDataSrcClientsBuilder::ExceptionFromWait +FakeDataSrcClientsBuilder::thread_throw_on_wait = + FakeDataSrcClientsBuilder::NOTHROW; namespace internal { template<> diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h index cc221fe455..03a7776c4c 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.h +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -114,21 +114,6 @@ TestDataSrcClientsBuilder::doNoop(); // possible compromise. class FakeDataSrcClientsBuilder { public: - FakeDataSrcClientsBuilder(std::list* command_queue, - internal::TestCondVar* cond, - internal::TestMutex* queue_mutex) - { - FakeDataSrcClientsBuilder::started = false; - FakeDataSrcClientsBuilder::command_queue = command_queue; - FakeDataSrcClientsBuilder::cond = cond; - FakeDataSrcClientsBuilder::queue_mutex = queue_mutex; - FakeDataSrcClientsBuilder::thread_waited = false; - FakeDataSrcClientsBuilder::thread_throw_on_wait = false; - } - void run() { - FakeDataSrcClientsBuilder::started = true; - } - // true iff a builder has started. static bool started; @@ -142,7 +127,23 @@ public: // If set to true by a test, TestThread::wait() throws an exception // exception. - static bool thread_throw_on_wait; + enum ExceptionFromWait { NOTHROW, THROW_UNCAUGHT_EX, THROW_OTHER }; + static ExceptionFromWait thread_throw_on_wait; + + FakeDataSrcClientsBuilder(std::list* command_queue, + internal::TestCondVar* cond, + internal::TestMutex* queue_mutex) + { + FakeDataSrcClientsBuilder::started = false; + FakeDataSrcClientsBuilder::command_queue = command_queue; + FakeDataSrcClientsBuilder::cond = cond; + FakeDataSrcClientsBuilder::queue_mutex = queue_mutex; + FakeDataSrcClientsBuilder::thread_waited = false; + FakeDataSrcClientsBuilder::thread_throw_on_wait = NOTHROW; + } + void run() { + FakeDataSrcClientsBuilder::started = true; + } }; class TestThread { @@ -152,7 +153,12 @@ public: } void wait() { FakeDataSrcClientsBuilder::thread_waited = true; - if (FakeDataSrcClientsBuilder::thread_throw_on_wait) { + switch (FakeDataSrcClientsBuilder::thread_throw_on_wait) { + case FakeDataSrcClientsBuilder::NOTHROW: + break; + case FakeDataSrcClientsBuilder::THROW_UNCAUGHT_EX: + isc_throw(util::thread::Thread::UncaughtException, "for test"); + case FakeDataSrcClientsBuilder::THROW_OTHER: isc_throw(Unexpected, "for test"); } } From 93c198a7b9afd341809b954e76f5e2773e0010ff Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 15 Oct 2012 23:00:28 -0700 Subject: [PATCH 10/22] [2205] make sure wating for builder thread in the mgr destructor. on second thought I realized we need to synchronize them because the builder thread refers to the main thread's class member variables. shutdown() method is now meaningless so deprecated for now. --- src/bin/auth/auth_messages.mes | 2 - src/bin/auth/datasrc_clients_mgr.h | 31 ++++++------ .../tests/datasrc_clients_mgr_unittest.cc | 48 ++++++++----------- 3 files changed, 35 insertions(+), 46 deletions(-) diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index 7a6b6858c6..4a721379c1 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -275,8 +275,6 @@ NOTIFY request will not be honored. % AUTH_DATASRC_CLIENTS_BUILDER_COMMAND data source builder received command, ID: %1 -% AUTH_DATASRC_CLIENTS_MANAGER_UNEXPECTED_STOP data source clients manager stopped unexpectedly, leaving the builder thread - % AUTH_DATASRC_CLIENTS_BUILDER_FAILED data source builder thread stopped due to an exception: %1 % AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED data source builder thread stopped due to an unexpected exception diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index b396bd5584..55e640df38 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -26,7 +26,6 @@ #include #include -#include #include #include @@ -86,27 +85,25 @@ class DataSrcClientsMgrBase { public: DataSrcClientsMgrBase() : builder_(&command_queue_, &cond_, &queue_mutex_), - builder_thread_(new ThreadType(boost::bind(&BuilderType::run, - &builder_))) + builder_thread_(boost::bind(&BuilderType::run, &builder_)) {} ~DataSrcClientsMgrBase() { - if (builder_thread_) { - // An unexpected case. The manager is being destroyed without - // a prior shutdown(). We notify the builder to minimize the risk - // of leaving it as a zombie, but doesn't wait to avoid hangup. - LOG_INFO(auth_logger, AUTH_DATASRC_CLIENTS_MANAGER_UNEXPECTED_STOP); - sendCommand(internal::SHUTDOWN, data::ConstElementPtr()); - } - } - void shutdown() { - sendCommand(internal::SHUTDOWN, data::ConstElementPtr()); + // We share class member variables with the builder, which will be + // invalidated after the call to the destructor, so we need to make + // sure the builder thread is terminated. Depending on the timing + // this could time; if we don't want that to happen in this context, + // we may want to introduce a separate 'shutdown()' method. + // Also, since we don't want to propagate exceptions from a destructor, + // we catch any possible ones. In fact the only really expected one + // is Thread::UncaughtException when the builder thread died due to + // an exception. We specifically log it and just ignore others. try { - builder_thread_->wait(); + sendCommand(internal::SHUTDOWN, data::ConstElementPtr()); + builder_thread_.wait(); } catch (const util::thread::Thread::UncaughtException& ex) { LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_SHUTDOWN_ERROR). arg(ex.what()); - } - builder_thread_.reset(); + } catch (...) {} } private: @@ -122,7 +119,7 @@ private: CondVarType cond_; MutexType queue_mutex_; BuilderType builder_; - boost::scoped_ptr builder_thread_; + ThreadType builder_thread_; }; namespace internal { diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc index b27db22586..074ab53003 100644 --- a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc @@ -36,6 +36,9 @@ shutdownCheck() { const Command& cmd = FakeDataSrcClientsBuilder::command_queue->front(); EXPECT_EQ(SHUTDOWN, cmd.first); EXPECT_FALSE(cmd.second); // no argument + + // Finally, the manager should wait for the thread to terminate. + EXPECT_TRUE(FakeDataSrcClientsBuilder::thread_waited); } TEST(DataSrcClientsMgrTest, start) { @@ -45,48 +48,39 @@ TEST(DataSrcClientsMgrTest, start) { TestDataSrcClientsMgr mgr; EXPECT_TRUE(FakeDataSrcClientsBuilder::started); EXPECT_TRUE(FakeDataSrcClientsBuilder::command_queue->empty()); - } - // We stopped the manager implicitly (without shutdown()). In this case - // the builder should be notified but the manager didn't wait. + // Check pre-destroy conditions + EXPECT_EQ(0, FakeDataSrcClientsBuilder::cond->signal_count); + EXPECT_FALSE(FakeDataSrcClientsBuilder::thread_waited); + } // mgr and builder have been destroyed by this point. + + // We stopped the manager implicitly (without shutdown()). The manager + // will internally notify it shutdownCheck(); - EXPECT_FALSE(FakeDataSrcClientsBuilder::thread_waited); -} - -TEST(DataSrcClientsMgrTest, shutdown) { - // Invoke shutdown on the manager. - TestDataSrcClientsMgr mgr; - EXPECT_TRUE(FakeDataSrcClientsBuilder::started); - - // Check pre-command conditions - EXPECT_EQ(0, FakeDataSrcClientsBuilder::cond->signal_count); - EXPECT_FALSE(FakeDataSrcClientsBuilder::thread_waited); - - mgr.shutdown(); - shutdownCheck(); - EXPECT_TRUE(FakeDataSrcClientsBuilder::thread_waited); } TEST(DataSrcClientsMgrTest, shutdownWithUncaughtException) { // Emulating the case when the builder exists on exception. shutdown() // will encounter UncaughtException exception and catch it. - TestDataSrcClientsMgr mgr; - FakeDataSrcClientsBuilder::thread_throw_on_wait = - FakeDataSrcClientsBuilder::THROW_UNCAUGHT_EX; - EXPECT_NO_THROW(mgr.shutdown()); + EXPECT_NO_THROW({ + TestDataSrcClientsMgr mgr; + FakeDataSrcClientsBuilder::thread_throw_on_wait = + FakeDataSrcClientsBuilder::THROW_UNCAUGHT_EX; + }); } + TEST(DataSrcClientsMgrTest, shutdownWithException) { - TestDataSrcClientsMgr mgr; - FakeDataSrcClientsBuilder::thread_throw_on_wait = - FakeDataSrcClientsBuilder::THROW_OTHER; - EXPECT_THROW(mgr.shutdown(), isc::Unexpected); + EXPECT_NO_THROW({ + TestDataSrcClientsMgr mgr; + FakeDataSrcClientsBuilder::thread_throw_on_wait = + FakeDataSrcClientsBuilder::THROW_OTHER; + }); } TEST(DataSrcClientsMgrTest, realThread) { // Using the non-test definition with a real thread. Just checking // no disruption happens. DataSrcClientsMgr mgr; - mgr.shutdown(); } } // unnamed namespace From e21b1a26b3f78d69975c5735c1965a37c67359c9 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Tue, 16 Oct 2012 15:45:01 -0700 Subject: [PATCH 11/22] [2205] cleanups: use more specific internal namespaces and reorder code mainly for better readability. no functional change. --- src/bin/auth/datasrc_clients_mgr.h | 105 +++++++++--------- .../tests/datasrc_clients_builder_unittest.cc | 2 +- .../tests/datasrc_clients_mgr_unittest.cc | 2 +- .../auth/tests/test_datasrc_clients_mgr.cc | 9 +- src/bin/auth/tests/test_datasrc_clients_mgr.h | 30 ++--- 5 files changed, 77 insertions(+), 71 deletions(-) diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 55e640df38..3d00405902 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -33,13 +33,61 @@ namespace isc { namespace auth { -namespace internal { +namespace datasrc_clientmgr_internal { enum CommandID { NOOP, ///< Do nothing. Only useful for tests SHUTDOWN ///< Shutdown the builder. }; typedef std::pair Command; +} // namespace datasrc_clientmgr_internal +template +class DataSrcClientsMgrBase { +public: + DataSrcClientsMgrBase() : + builder_(&command_queue_, &cond_, &queue_mutex_), + builder_thread_(boost::bind(&BuilderType::run, &builder_)) + {} + ~DataSrcClientsMgrBase() { + // We share class member variables with the builder, which will be + // invalidated after the call to the destructor, so we need to make + // sure the builder thread is terminated. Depending on the timing + // this could time; if we don't want that to happen in this context, + // we may want to introduce a separate 'shutdown()' method. + // Also, since we don't want to propagate exceptions from a destructor, + // we catch any possible ones. In fact the only really expected one + // is Thread::UncaughtException when the builder thread died due to + // an exception. We specifically log it and just ignore others. + try { + sendCommand(datasrc_clientmgr_internal::SHUTDOWN, + data::ConstElementPtr()); + builder_thread_.wait(); + } catch (const util::thread::Thread::UncaughtException& ex) { + LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_SHUTDOWN_ERROR). + arg(ex.what()); + } catch (...) {} + } + +private: + 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)); + } + cond_.signal(); + } + + std::list command_queue_; + CondVarType cond_; + MutexType queue_mutex_; + BuilderType builder_; + ThreadType builder_thread_; +}; + +namespace datasrc_clientmgr_internal { template class DataSrcClientsBuilderBase { public: @@ -77,52 +125,7 @@ private: // Shortcut typedef for normal use typedef DataSrcClientsBuilderBase DataSrcClientsBuilder; -} -template -class DataSrcClientsMgrBase { -public: - DataSrcClientsMgrBase() : - builder_(&command_queue_, &cond_, &queue_mutex_), - builder_thread_(boost::bind(&BuilderType::run, &builder_)) - {} - ~DataSrcClientsMgrBase() { - // We share class member variables with the builder, which will be - // invalidated after the call to the destructor, so we need to make - // sure the builder thread is terminated. Depending on the timing - // this could time; if we don't want that to happen in this context, - // we may want to introduce a separate 'shutdown()' method. - // Also, since we don't want to propagate exceptions from a destructor, - // we catch any possible ones. In fact the only really expected one - // is Thread::UncaughtException when the builder thread died due to - // an exception. We specifically log it and just ignore others. - try { - sendCommand(internal::SHUTDOWN, data::ConstElementPtr()); - builder_thread_.wait(); - } catch (const util::thread::Thread::UncaughtException& ex) { - LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_SHUTDOWN_ERROR). - arg(ex.what()); - } catch (...) {} - } - -private: - void sendCommand(internal::CommandID command, data::ConstElementPtr arg) { - { - typename MutexType::Locker locker(queue_mutex_); - command_queue_.push_back(internal::Command(command, arg)); - } - cond_.signal(); - } - - std::list command_queue_; - CondVarType cond_; - MutexType queue_mutex_; - BuilderType builder_; - ThreadType builder_thread_; -}; - -namespace internal { template void DataSrcClientsBuilderBase::run() { @@ -177,16 +180,16 @@ DataSrcClientsBuilderBase::handleCommand( } return (true); } -} // namespace internal +} // namespace datasrc_clientmgr_internal /// \brief Shortcut type for normal data source clients manager. /// /// In fact, for non test applications this is the only type of this kind /// to be considered. -typedef DataSrcClientsMgrBase -DataSrcClientsMgr; +typedef DataSrcClientsMgrBase< + util::thread::Thread, + datasrc_clientmgr_internal::DataSrcClientsBuilder, + util::thread::Mutex, util::thread::CondVar> DataSrcClientsMgr; } // namespace auth } // namespace isc diff --git a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc index 53491291d3..01a4cb7436 100644 --- a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc @@ -22,7 +22,7 @@ #include using isc::data::ConstElementPtr; -using namespace isc::auth::internal; +using namespace isc::auth::datasrc_clientmgr_internal; namespace { class DataSrcClientsBuilderTest : public ::testing::Test { diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc index 074ab53003..5ad76b83b4 100644 --- a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc @@ -22,7 +22,7 @@ #include using namespace isc::auth; -using namespace isc::auth::internal; +using namespace isc::auth::datasrc_clientmgr_internal; namespace { void diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.cc b/src/bin/auth/tests/test_datasrc_clients_mgr.cc index da318f3453..7d114aedd1 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.cc +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.cc @@ -18,17 +18,18 @@ namespace isc { namespace auth { +namespace datasrc_clientmgr_internal { + // Define static DataSrcClientsBuilder member variables. bool FakeDataSrcClientsBuilder::started = false; -std::list* FakeDataSrcClientsBuilder::command_queue = NULL; -internal::TestCondVar* FakeDataSrcClientsBuilder::cond = NULL; -internal::TestMutex* FakeDataSrcClientsBuilder::queue_mutex = NULL; +std::list* FakeDataSrcClientsBuilder::command_queue = NULL; +TestCondVar* FakeDataSrcClientsBuilder::cond = NULL; +TestMutex* FakeDataSrcClientsBuilder::queue_mutex = NULL; bool FakeDataSrcClientsBuilder::thread_waited = false; FakeDataSrcClientsBuilder::ExceptionFromWait FakeDataSrcClientsBuilder::thread_throw_on_wait = FakeDataSrcClientsBuilder::NOTHROW; -namespace internal { template<> void TestDataSrcClientsBuilder::doNoop() { diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h index 03a7776c4c..3c6cab1e94 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.h +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -23,11 +23,11 @@ #include -// Below we extend the isc::auth::internal namespace to specialize -// the doNoop() method. +// Below we extend the isc::auth::datasrc_clientmgr_internal namespace to +// specialize the doNoop() method. namespace isc { namespace auth { -namespace internal { +namespace datasrc_clientmgr_internal { class TestMutex { public: // for throw_from_noop. @@ -106,8 +106,6 @@ template<> void TestDataSrcClientsBuilder::doNoop(); -} // namespace internal - // A specialization of DataSrcClientsBuilder that allows tests to inspect // its internal states via static class variables. Using static is suboptimal, // but DataSrcClientsMgr is highly encapsulated, this seems to be the best @@ -118,9 +116,9 @@ public: static bool started; // These three correspond to the resource shared with the manager. - static std::list* command_queue; - static internal::TestCondVar* cond; - static internal::TestMutex* queue_mutex; + static std::list* command_queue; + static TestCondVar* cond; + static TestMutex* queue_mutex; // true iff the manager waited on the thread running the builder. static bool thread_waited; @@ -130,9 +128,10 @@ public: enum ExceptionFromWait { NOTHROW, THROW_UNCAUGHT_EX, THROW_OTHER }; static ExceptionFromWait thread_throw_on_wait; - FakeDataSrcClientsBuilder(std::list* command_queue, - internal::TestCondVar* cond, - internal::TestMutex* queue_mutex) + FakeDataSrcClientsBuilder( + std::list* command_queue, + TestCondVar* cond, + TestMutex* queue_mutex) { FakeDataSrcClientsBuilder::started = false; FakeDataSrcClientsBuilder::command_queue = command_queue; @@ -163,11 +162,14 @@ public: } } }; +} // namespace datasrc_clientmgr_internal // Convenient shortcut -typedef DataSrcClientsMgrBase -TestDataSrcClientsMgr; +typedef DataSrcClientsMgrBase< + datasrc_clientmgr_internal::TestThread, + datasrc_clientmgr_internal::FakeDataSrcClientsBuilder, + datasrc_clientmgr_internal::TestMutex, + datasrc_clientmgr_internal::TestCondVar> TestDataSrcClientsMgr; } // namespace auth } // namespace isc From e7bcdb44ef51c4f8fcf81a11d247494384fa0c41 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Tue, 16 Oct 2012 17:12:46 -0700 Subject: [PATCH 12/22] [2205] overall documentation updates --- src/bin/auth/auth_messages.mes | 21 +++ src/bin/auth/datasrc_clients_mgr.h | 131 +++++++++++++++--- src/bin/auth/tests/test_datasrc_clients_mgr.h | 21 ++- 3 files changed, 149 insertions(+), 24 deletions(-) diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index 4a721379c1..872bc3c74d 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -270,13 +270,34 @@ but has returned an error response (which is included in the message). The NOTIFY request will not be honored. % AUTH_DATASRC_CLIENTS_BUILDER_STARTED data source builder thread started +A separate thread for maintaining data source clients has been started. % AUTH_DATASRC_CLIENTS_BUILDER_STOPPED data source builder thread stopped +The separate thread for maintaining data source clients has been stopped. % AUTH_DATASRC_CLIENTS_BUILDER_COMMAND data source builder received command, ID: %1 +A debug message, showing when the separate thread for maintaining data +source clients receives a command from the manager. % AUTH_DATASRC_CLIENTS_BUILDER_FAILED data source builder thread stopped due to an exception: %1 +The separate thread for maintaining data source clients has been +terminated due to some uncaught exception. The manager cannot always +catch this condition in timely fashion, and there is no way to recover +from this situation except for restarting the entire server. So this +message needs to be carefully watched, and should it occur the auth +server needs to be restarted by hand. % AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED data source builder thread stopped due to an unexpected exception +This is similar to AUTH_DATASRC_CLIENTS_BUILDER_FAILED, but the +exception type is even more unexpected. This may rather indicate some +run time failure than program errors, but in any case the server needs +to be restarted by hand. % 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. diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 3d00405902..9f4e4771bc 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -34,21 +34,77 @@ namespace isc { namespace auth { namespace datasrc_clientmgr_internal { +// This namespace is essentially private for DataSrcClientsMgr(Base) and +// DataSrcClientsBuilder(Base). This is exposed in the public header +// only because these classes are templated (for testing purposes) and +// class internal has to be defined here. + +/// \brief ID of commands from the DataSrcClientsMgr to DataSrcClientsBuilder. enum CommandID { - NOOP, ///< Do nothing. Only useful for tests - SHUTDOWN ///< Shutdown the builder. + NOOP, ///< Do nothing. Only useful for tests; no argument + SHUTDOWN ///< Shutdown the builder; no argument }; + +/// \brief The data type passed from DataSrcClientsMgr to +/// DataSrcClientsBuilder. +/// +/// The first element of the pair is the command ID, and the second element +/// is its argument. If the command doesn't take an argument it should be +/// a null pointer. typedef std::pair Command; } // namespace datasrc_clientmgr_internal +/// \brief Frontend to the manager object for data source clients. +/// +/// This class provides interfaces for configuring and updating a set of +/// data source clients "in the background". The user of this class can +/// assume any operation on this class can be done effectively non-blocking, +/// not suspending any delay-sensitive operations such as DNS query +/// processing. The only exception is the time when this class object +/// is destroyed (normally as a result of an implicit call to the destructor); +/// in the current implementation it can take time depending on what is +/// running "in the background" at the time of the call. +/// +/// Internally, an object of this class invokes a separate thread to perform +/// time consuming operations such as loading large zone data into memory, +/// but such details are completely hidden from the user of this class. +/// +/// This class is templated only so that we can test the class without +/// involving actual threads or mutex. Normal applications will only +/// need one specific specialization that has a typedef of +/// \c DataSrcClientsMgr. template class DataSrcClientsMgrBase { public: + /// \brief Constructor. + /// + /// It internally invokes a separate thread and waits for further + /// operations from the user application. + /// + /// This method is basically exception free except in case of really + /// rare system-level errors. When that happens the only reasonable + /// action that the application can take would be to terminate the program + /// in practice. + /// + /// \throw std::bad_alloc internal memory allocation failure. + /// \throw isc::Unexpected general unexpected system errors. DataSrcClientsMgrBase() : builder_(&command_queue_, &cond_, &queue_mutex_), builder_thread_(boost::bind(&BuilderType::run, &builder_)) {} + + /// \brief The destructor. + /// + /// It tells the internal thread to stop and waits for it completion. + /// In the current implementation, it can block for some unpredictably + /// long period depending on what the thread is doing at that time + /// (in future we may want to implement a rapid way of killing the thread + /// and/or provide a separate interface for waiting so that the application + /// can choose the timing). + /// + /// The waiting operation can result in an exception, but this method + /// catches any of them so this method itself is exception free. ~DataSrcClientsMgrBase() { // We share class member variables with the builder, which will be // invalidated after the call to the destructor, so we need to make @@ -64,6 +120,9 @@ public: data::ConstElementPtr()); builder_thread_.wait(); } catch (const util::thread::Thread::UncaughtException& ex) { + // technically, logging this could throw, which will be propagated. + // But such an exception would be a fatal one anyway, so we + // simply let it go through. LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_SHUTDOWN_ERROR). arg(ex.what()); } catch (...) {} @@ -80,46 +139,80 @@ private: cond_.signal(); } + // + // The following are shared with the builder. + // + // The list is used as a one-way queue: back-in, front-out std::list command_queue_; - CondVarType cond_; - MutexType queue_mutex_; + CondVarType cond_; // condition variable for queue operations + MutexType queue_mutex_; // mutex to protect the queue +#ifdef notyet // until #2210 or #2212 + boost::shared_ptr clients_map_; + MutexType map_mutex_; +#endif + BuilderType builder_; - ThreadType builder_thread_; + ThreadType builder_thread_; // for safety this should be placed last }; namespace datasrc_clientmgr_internal { + +/// \brief A class that maintains a set of data source clients. +/// +/// An object of this class is supposed to run on a dedicated thread, whose +/// main function is a call to its \c run() method. It runs in a loop +/// waiting for commands from the manager and handle each command (including +/// reloading a new version of zone data into memory or fully reconfiguration +/// of specific set of data source clients. When it receives a SHUTDOWN +/// command, it exits from the loop, which will terminate the thread. +/// +/// This class is a server of \c DataSrcClientsMgr. Except for tests, +/// applications should not directly access to this class. +/// +/// This class is templated so that we can test it without involving actual +/// threads or locks. template class DataSrcClientsBuilderBase { public: + /// \brief Constructor. + /// + /// It simply sets up a local copy of shared data with the manager. + /// + /// Note: this will take actual set (map) of data source clients and + /// a mutex object for it in #2210 or #2212. + /// + /// \throw None DataSrcClientsBuilderBase(std::list* command_queue, - CondVarType* cond, MutexType* queue_mutex) : + CondVarType* cond, MutexType* queue_mutex +#ifdef notyet + // In #2210 or #2212 we pass other data +#endif + ) : command_queue_(command_queue), cond_(cond), queue_mutex_(queue_mutex) {} - /// Not sure if we need this. It depends on test details. - /// \brief Destructor. - /// - /// This does nothing, but explicitly defined to silence 'unused variable' - /// warnings from some versions of clang++. - ///~DataSrcClientsBuilderBase() {} - + /// \brief The main loop. void run(); - /// separated from run() and made public for the purpose of tests. + /// \brief Handle one command from the manager. /// - /// \return true if it the builder should keep running; false otherwise. + /// This is a dedicated subroutine of run() and is essentially private, + /// but is defined as a separate public method so we can test each + /// command test individually. In any case, this class itself is + /// generally considered private. + /// + /// \return true if the builder should keep running; false otherwise. bool handleCommand(const Command& command); private: - // NOOP command handler. We use this so tests can override it. + // NOOP command handler. We use this so tests can override it; the default + // implementation really does nothing. void doNoop() {} - // end-in, front-out queue + // The following are shared with the manager std::list* command_queue_; CondVarType* cond_; MutexType* queue_mutex_; - //boost::shared_ptr* map; - //MutexType* data_mutex_; }; // Shortcut typedef for normal use diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h index 3c6cab1e94..911e7220e2 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.h +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -23,6 +23,14 @@ #include +// In this file we provide specialization of thread, mutex, condition variable, +// and DataSrcClientsBuilder for convenience of tests. They don't use +// actual threads or mutex, and allow tests to inspect some internal states +// of the corresponding objects. +// +// In many cases, tests can use TestDataSrcClientsMgr (defined below) where +// DataSrcClientsMgr is needed. + // Below we extend the isc::auth::datasrc_clientmgr_internal namespace to // specialize the doNoop() method. namespace isc { @@ -54,10 +62,10 @@ public: private: TestMutex& mutex_; }; - size_t lock_count; - size_t unlock_count; + size_t lock_count; // number of lock acquisitions; tests can check this + size_t unlock_count; // number of lock releases; tests can check this size_t noop_count; // allow doNoop() to modify this - ExceptionFromNoop throw_from_noop; // test can set this to control doNoop + ExceptionFromNoop throw_from_noop; // tests can set this to control doNoop }; class TestCondVar { @@ -89,8 +97,8 @@ public: void signal() { ++signal_count; } - size_t wait_count; - size_t signal_count; + size_t wait_count; // number of calls to wait(); tests can check this + size_t signal_count; // number of calls to signal(); tests can check this private: std::list* command_queue_; std::list* delayed_command_queue_; @@ -145,6 +153,9 @@ public: } }; +// A fake thread class that doesn't really invoke thread but simply calls +// the given main function (synchronously). Tests can tweak the wait() +// behavior via some static variables so it will throw some exceptions. class TestThread { public: TestThread(const boost::function& main) { From c857ced99825b35644c2abdbad8be14644da237c Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Tue, 16 Oct 2012 17:30:18 -0700 Subject: [PATCH 13/22] [2205] make AUTH_DATASRC_CLIENTS_BUILDER_COMMAND more readable using text. also explicitly reject invalid command ID, which should be internal bug. --- src/bin/auth/auth_messages.mes | 2 +- src/bin/auth/datasrc_clients_mgr.h | 19 ++++++++++++++++--- .../tests/datasrc_clients_builder_unittest.cc | 7 +++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index 872bc3c74d..f5253e6959 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -275,7 +275,7 @@ A separate thread for maintaining data source clients has been started. % AUTH_DATASRC_CLIENTS_BUILDER_STOPPED data source builder thread stopped The separate thread for maintaining data source clients has been stopped. -% AUTH_DATASRC_CLIENTS_BUILDER_COMMAND data source builder received command, ID: %1 +% AUTH_DATASRC_CLIENTS_BUILDER_COMMAND data source builder received command: %1 A debug message, showing when the separate thread for maintaining data source clients receives a command from the manager. diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 9f4e4771bc..c3bf1d2440 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -25,6 +25,7 @@ #include +#include #include #include @@ -42,7 +43,8 @@ namespace datasrc_clientmgr_internal { /// \brief ID of commands from the DataSrcClientsMgr to DataSrcClientsBuilder. enum CommandID { NOOP, ///< Do nothing. Only useful for tests; no argument - SHUTDOWN ///< Shutdown the builder; no argument + SHUTDOWN, ///< Shutdown the builder; no argument + NUM_COMMANDS }; /// \brief The data type passed from DataSrcClientsMgr to @@ -262,14 +264,25 @@ bool DataSrcClientsBuilderBase::handleCommand( const Command& command) { - LOG_DEBUG(auth_logger, DBGLVL_TRACE_BASIC, - AUTH_DATASRC_CLIENTS_BUILDER_COMMAND).arg(command.first); + const CommandID cid = command.first; + if (cid >= NUM_COMMANDS) { + // This shouldn't happen except for a bug within this file. + isc_throw(Unexpected, "internal bug: invalid command, ID: " << cid); + } + const boost::array command_desc = { + {"NOOP", "SHUTDOWN"} + }; + LOG_DEBUG(auth_logger, DBGLVL_TRACE_BASIC, + AUTH_DATASRC_CLIENTS_BUILDER_COMMAND).arg(command_desc.at(cid)); switch (command.first) { case SHUTDOWN: return (false); case NOOP: doNoop(); + break; + case NUM_COMMANDS: + assert(false); // we rejected this case above } return (true); } diff --git a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc index 01a4cb7436..4978d6efe2 100644 --- a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc @@ -96,4 +96,11 @@ TEST_F(DataSrcClientsBuilderTest, shutdown) { EXPECT_FALSE(builder.handleCommand(shutdown_cmd)); } +TEST_F(DataSrcClientsBuilderTest, badCommand) { + // out-of-range command ID + EXPECT_THROW(builder.handleCommand(Command(NUM_COMMANDS, + ConstElementPtr())), + isc::Unexpected); +} + } // unnamed namespace From 4b2997b783ed2f9a29842735dad130cfdb41e38a Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Tue, 16 Oct 2012 19:51:25 -0700 Subject: [PATCH 14/22] [2205] create DataSrcClientsMgr in auth's main() just to confirm it starts/ends it currently does nothing user-visible. --- src/bin/auth/main.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/bin/auth/main.cc b/src/bin/auth/main.cc index 99080662ff..acbaa50ce5 100644 --- a/src/bin/auth/main.cc +++ b/src/bin/auth/main.cc @@ -36,6 +36,8 @@ #include #include #include +#include + #include #include #include @@ -230,6 +232,10 @@ 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; From c2608e7b395881e9953962d923dfd1de6eb50f5f Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 17 Oct 2012 15:56:27 -0700 Subject: [PATCH 15/22] [2205] avoid referencing post-destructor member variables. by a bit ugly hack: we specialize some part of the destructor and make a local copy of these variables so the tests can inspect them later. --- src/bin/auth/datasrc_clients_mgr.h | 8 ++++++ .../auth/tests/test_datasrc_clients_mgr.cc | 27 +++++++++++++++++-- src/bin/auth/tests/test_datasrc_clients_mgr.h | 12 +++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index c3bf1d2440..4cfb170e5b 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -128,9 +128,17 @@ public: LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_SHUTDOWN_ERROR). arg(ex.what()); } catch (...) {} + + cleanup(); // see below } private: + // This is expected to be called at the end of the destructor. It + // actually does nothing, but provides a customization point for + // specialized class for tests so that the tests can inspect the last + // state of the class. + void cleanup() {} + void sendCommand(datasrc_clientmgr_internal::CommandID command, data::ConstElementPtr arg) { { diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.cc b/src/bin/auth/tests/test_datasrc_clients_mgr.cc index 7d114aedd1..72979b4844 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.cc +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.cc @@ -23,8 +23,11 @@ namespace datasrc_clientmgr_internal { // Define static DataSrcClientsBuilder member variables. bool FakeDataSrcClientsBuilder::started = false; std::list* FakeDataSrcClientsBuilder::command_queue = NULL; +std::list FakeDataSrcClientsBuilder::command_queue_copy; TestCondVar* FakeDataSrcClientsBuilder::cond = NULL; +TestCondVar FakeDataSrcClientsBuilder::cond_copy; TestMutex* FakeDataSrcClientsBuilder::queue_mutex = NULL; +TestMutex FakeDataSrcClientsBuilder::queue_mutex_copy; bool FakeDataSrcClientsBuilder::thread_waited = false; FakeDataSrcClientsBuilder::ExceptionFromWait FakeDataSrcClientsBuilder::thread_throw_on_wait = @@ -43,9 +46,29 @@ TestDataSrcClientsBuilder::doNoop() { throw 42; } } +} // namespace datasrc_clientmgr_internal + +template<> +void +TestDataSrcClientsMgr::cleanup() { + using namespace datasrc_clientmgr_internal; + // Make copy of some of the manager's member variables and reset the + // corresponding pointers. The currently pointed objects are in the + // manager object, which are going to be invalidated. + + FakeDataSrcClientsBuilder::command_queue_copy = command_queue_; + FakeDataSrcClientsBuilder::command_queue = + &FakeDataSrcClientsBuilder::command_queue_copy; + FakeDataSrcClientsBuilder::queue_mutex_copy = queue_mutex_; + FakeDataSrcClientsBuilder::queue_mutex = + &FakeDataSrcClientsBuilder::queue_mutex_copy; + FakeDataSrcClientsBuilder::cond_copy = cond_; + FakeDataSrcClientsBuilder::cond = + &FakeDataSrcClientsBuilder::cond_copy; } -} -} + +} // namespace auth +} // namespace isc // Local Variables: // mode: c++ diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h index 911e7220e2..0519425859 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.h +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -124,9 +124,14 @@ public: static bool started; // These three correspond to the resource shared with the manager. + // xxx_copy will be set in the manager's destructor to record the + // final state of the manager. static std::list* command_queue; static TestCondVar* cond; static TestMutex* queue_mutex; + static std::list command_queue_copy; + static TestCondVar cond_copy; + static TestMutex queue_mutex_copy; // true iff the manager waited on the thread running the builder. static bool thread_waited; @@ -182,6 +187,13 @@ typedef DataSrcClientsMgrBase< datasrc_clientmgr_internal::TestMutex, datasrc_clientmgr_internal::TestCondVar> TestDataSrcClientsMgr; +// A specialization of manager's "cleanup" called at the end of the +// destructor. We use this to record the final values of some of the class +// member variables. +template<> +void +TestDataSrcClientsMgr::cleanup(); + } // namespace auth } // namespace isc From 949aded6b4682df0f07e78cd4620df303f789236 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 17 Oct 2012 16:05:51 -0700 Subject: [PATCH 16/22] [2205] some wording fixes in-code documentation --- src/bin/auth/datasrc_clients_mgr.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 4cfb170e5b..073e2ccbda 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -111,8 +111,9 @@ public: // We share class member variables with the builder, which will be // invalidated after the call to the destructor, so we need to make // sure the builder thread is terminated. Depending on the timing - // this could time; if we don't want that to happen in this context, - // we may want to introduce a separate 'shutdown()' method. + // this could take a long time; if we don't want that to happen in + // this context, we may want to introduce a separate 'shutdown()' + // method. // Also, since we don't want to propagate exceptions from a destructor, // we catch any possible ones. In fact the only really expected one // is Thread::UncaughtException when the builder thread died due to @@ -171,9 +172,9 @@ namespace datasrc_clientmgr_internal { /// /// An object of this class is supposed to run on a dedicated thread, whose /// main function is a call to its \c run() method. It runs in a loop -/// waiting for commands from the manager and handle each command (including +/// waiting for commands from the manager and handles each command (including /// reloading a new version of zone data into memory or fully reconfiguration -/// of specific set of data source clients. When it receives a SHUTDOWN +/// of specific set of data source clients). When it receives a SHUTDOWN /// command, it exits from the loop, which will terminate the thread. /// /// This class is a server of \c DataSrcClientsMgr. Except for tests, From d5afffd2e3db401cafb5ac2ca80215afffddf7da Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 17 Oct 2012 16:12:50 -0700 Subject: [PATCH 17/22] [2205] logged really unexpected exception in manager's dtor. --- src/bin/auth/auth_messages.mes | 8 ++++++++ src/bin/auth/datasrc_clients_mgr.h | 5 ++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index f5253e6959..e8cb2885f2 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -301,3 +301,11 @@ 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. + +% 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 +separate thread for maintaining data source clients. This shouldn't +happen in normal conditions; it should be either fatal system level +errors such as severe memory shortage or some internal bug. If that +happens, and if it's not in the middle of terminating b10-auth, it's +probably better to stop and restart it. diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 073e2ccbda..7389a6b222 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -128,7 +128,10 @@ public: // simply let it go through. LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_SHUTDOWN_ERROR). arg(ex.what()); - } catch (...) {} + } catch (...) { + LOG_ERROR(auth_logger, + AUTH_DATASRC_CLIENTS_SHUTDOWN_UNEXPECTED_ERROR); + } cleanup(); // see below } From f34ff8e2e66798f4dc2fc63d1aeb4c198431beb1 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 17 Oct 2012 16:15:10 -0700 Subject: [PATCH 18/22] [2205] clarify the accessibility of DataSrcClientsBuilderBase --- src/bin/auth/datasrc_clients_mgr.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 7389a6b222..c8191c3691 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -180,8 +180,9 @@ namespace datasrc_clientmgr_internal { /// of specific set of data source clients). When it receives a SHUTDOWN /// command, it exits from the loop, which will terminate the thread. /// -/// This class is a server of \c DataSrcClientsMgr. Except for tests, -/// applications should not directly access to this class. +/// While this class is defined in a publicly visible namespace, it's +/// essentially private to \c DataSrcClientsMgr. Except for tests, +/// applications should not directly access this class. /// /// This class is templated so that we can test it without involving actual /// threads or locks. From 59ca62327546854c22cf8caae902f023a79f3272 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 17 Oct 2012 16:17:39 -0700 Subject: [PATCH 19/22] [2205] tried to improve what() message of some test exceptions --- src/bin/auth/tests/test_datasrc_clients_mgr.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h index 0519425859..99dc53efb5 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.h +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -172,9 +172,11 @@ public: case FakeDataSrcClientsBuilder::NOTHROW: break; case FakeDataSrcClientsBuilder::THROW_UNCAUGHT_EX: - isc_throw(util::thread::Thread::UncaughtException, "for test"); + isc_throw(util::thread::Thread::UncaughtException, + "TestThread wait() saw an exception"); case FakeDataSrcClientsBuilder::THROW_OTHER: - isc_throw(Unexpected, "for test"); + isc_throw(Unexpected, + "General emulated failure in TestThread wait()"); } } }; From 0582539b207817d0ab63a2f07923d9cd69b520a7 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 17 Oct 2012 16:18:54 -0700 Subject: [PATCH 20/22] [2205] reordered log messages --- src/bin/auth/auth_messages.mes | 82 +++++++++++++++++----------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index e8cb2885f2..aa40c8a95c 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -57,6 +57,47 @@ At attempt to update the configuration the server with information from the configuration database has failed, the reason being given in the message. +% AUTH_DATASRC_CLIENTS_BUILDER_COMMAND data source builder received command: %1 +A debug message, showing when the separate thread for maintaining data +source clients receives a command from the manager. + +% AUTH_DATASRC_CLIENTS_BUILDER_FAILED data source builder thread stopped due to an exception: %1 +The separate thread for maintaining data source clients has been +terminated due to some uncaught exception. The manager cannot always +catch this condition in timely fashion, and there is no way to recover +from this situation except for restarting the entire server. So this +message needs to be carefully watched, and should it occur the auth +server needs to be restarted by hand. + +% AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED data source builder thread stopped due to an unexpected exception +This is similar to AUTH_DATASRC_CLIENTS_BUILDER_FAILED, but the +exception type is even more unexpected. This may rather indicate some +run time failure than program errors, but in any case the server needs +to be restarted by hand. + +% AUTH_DATASRC_CLIENTS_BUILDER_STARTED data source builder thread started +A separate thread for maintaining data source clients has been started. + +% AUTH_DATASRC_CLIENTS_BUILDER_STOPPED data source builder thread stopped +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. + +% 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 +separate thread for maintaining data source clients. This shouldn't +happen in normal conditions; it should be either fatal system level +errors such as severe memory shortage or some internal bug. If that +happens, and if it's not in the middle of terminating b10-auth, it's +probably better to stop and restart it. + % AUTH_DATA_SOURCE data source database file: %1 This is a debug message produced by the authoritative server when it accesses a datebase data source, listing the file that is being accessed. @@ -268,44 +309,3 @@ This is a debug message output during the processing of a NOTIFY request. The zone manager component has been informed of the request, but has returned an error response (which is included in the message). The NOTIFY request will not be honored. - -% AUTH_DATASRC_CLIENTS_BUILDER_STARTED data source builder thread started -A separate thread for maintaining data source clients has been started. - -% AUTH_DATASRC_CLIENTS_BUILDER_STOPPED data source builder thread stopped -The separate thread for maintaining data source clients has been stopped. - -% AUTH_DATASRC_CLIENTS_BUILDER_COMMAND data source builder received command: %1 -A debug message, showing when the separate thread for maintaining data -source clients receives a command from the manager. - -% AUTH_DATASRC_CLIENTS_BUILDER_FAILED data source builder thread stopped due to an exception: %1 -The separate thread for maintaining data source clients has been -terminated due to some uncaught exception. The manager cannot always -catch this condition in timely fashion, and there is no way to recover -from this situation except for restarting the entire server. So this -message needs to be carefully watched, and should it occur the auth -server needs to be restarted by hand. - -% AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED data source builder thread stopped due to an unexpected exception -This is similar to AUTH_DATASRC_CLIENTS_BUILDER_FAILED, but the -exception type is even more unexpected. This may rather indicate some -run time failure than program errors, but in any case the server needs -to be restarted by hand. - -% 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. - -% 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 -separate thread for maintaining data source clients. This shouldn't -happen in normal conditions; it should be either fatal system level -errors such as severe memory shortage or some internal bug. If that -happens, and if it's not in the middle of terminating b10-auth, it's -probably better to stop and restart it. From 9904c81d783e8e4678ba6de3db438ddbb186dcaa Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 17 Oct 2012 17:23:31 -0700 Subject: [PATCH 21/22] [2205] minor style fix: brace position --- src/bin/auth/datasrc_clients_mgr.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index c8191c3691..4505b4e7ec 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -144,7 +144,8 @@ private: void cleanup() {} void sendCommand(datasrc_clientmgr_internal::CommandID command, - data::ConstElementPtr arg) { + data::ConstElementPtr arg) + { { typename MutexType::Locker locker(queue_mutex_); command_queue_.push_back( From 9b439f1bf7dc415312be57570b092459fad4caf7 Mon Sep 17 00:00:00 2001 From: Jelte Jansen Date: Thu, 18 Oct 2012 14:16:22 +0200 Subject: [PATCH 22/22] [2205] initialize member signal_count --- src/bin/auth/tests/test_datasrc_clients_mgr.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h index 99dc53efb5..7512e83715 100644 --- a/src/bin/auth/tests/test_datasrc_clients_mgr.h +++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h @@ -76,6 +76,7 @@ public: TestCondVar(std::list& command_queue, std::list& delayed_command_queue) : wait_count(0), + signal_count(0), command_queue_(&command_queue), delayed_command_queue_(&delayed_command_queue) {