[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 7a6b6858c6392a30712bfaa0abc6e38587075269..4a721379c13e8950e40764e5a7f1d0b192d37454 100644 (file)
--- 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 b396bd5584ecb990fc79eb35f46b11d47322988e..55e640df38adc74f4a4bc48113050a8178e47c81 100644 (file)
--- a/src/bin/auth/datasrc_clients_mgr.h
+++ b/src/bin/auth/datasrc_clients_mgr.h
@@ -26,7 +26,6 @@
 #include <auth/auth_log.h>
 
 #include <boost/bind.hpp>
-#include <boost/scoped_ptr.hpp>
 
 #include <list>
 #include <utility>
@@ -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<ThreadType> 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 b27db225866af33872366517856935dbb03d2c00..074ab53003ef82ded047f237e5e693c324132a3a 100644 (file)
--- 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