From: JINMEI Tatuya <jinmei@isc.org>
Date: Mon, 15 Oct 2012 23:31:18 +0000 (-0700)
Subject: [2205] handle the case where manager is destroyed without shutdown()
X-Git-Tag: trac2402_base~15^2~15
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=16d31a6793cedc28c8acb1bebf4c891c039321cb;p=thirdparty%2Fkea.git

[2205] handle the case where manager is destroyed without shutdown()
---

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 <auth/auth_log.h>
 
 #include <boost/bind.hpp>
+#include <boost/scoped_ptr.hpp>
 
 #include <list>
 #include <utility>
@@ -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<ThreadType> builder_thread_;
 };
 
 namespace internal {
 template <typename MutexType, typename CondVarType>
 void
 DataSrcClientsBuilderBase<MutexType, CondVarType>::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<MutexType, CondVarType>::run() {
         }
     }
 
-    LOG_INFO(auth_logger, AUTH_DATASRC_CLIENT_BUILDER_STOPPED);
+    LOG_INFO(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_STOPPED);
 }
 
 template <typename MutexType, typename CondVarType>
@@ -143,7 +154,7 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::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);
 }