From: JINMEI Tatuya Date: Tue, 16 Oct 2012 06:00:28 +0000 (-0700) Subject: [2205] make sure wating for builder thread in the mgr destructor. X-Git-Tag: trac2402_base~15^2~12 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=93c198a7b9afd341809b954e76f5e2773e0010ff;p=thirdparty%2Fkea.git [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. --- 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. - 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); + // 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. - mgr.shutdown(); + // We stopped the manager implicitly (without shutdown()). The manager + // will internally notify it 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