[#1736] Address race conditions revolving around early listener start

Starting the listener before MT mode is applied is problematic.
Also now explicitly stopping the service rather than relying on
destruction order.

src/hooks/dhcp/high_availability/ha_impl.*
    HAImpl::startService() - added deferred call to
    start HAServic::startClientAndListener()

    HAImpl::~HAImpl() - new destructor which explicitly
    calls HAService::stopClientAndListener()

src/hooks/dhcp/high_availability/ha_service.*
    HAService::HAService() - moved client and listener
    instantation here but without listener start

    HAService::startClientAndListener() - now only invokes
    listener start

src/hooks/dhcp/high_availability/tests/ha_mt_unittest.cc
    TEST_F(HAMtServiceTest, multiThreadingStartup) - added call
    to startClientAndListener()

diff --git a/src/hooks/dhcp/high_availability/ha_impl.cc b/src/hooks/dhcp/high_availability/ha_impl.cc
index b2e3d55b26f835accc3d2b99f0b7bea1d4b07dca..eb5b5d219ee9d49d7f236bda5215bc55340eb8c8 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_impl.cc
+++ b/src/hooks/dhcp/high_availability/ha_impl.cc
@@ -44,6 +44,17 @@ HAImpl::startService(const IOServicePtr& io_service,
     // Create the HA service and crank up the state machine.
     service_ = boost::make_shared<HAService>(io_service, network_state,
                                              config_, server_type);
+    // Schedule a start of the services. This ensures we begin after
+    // the dust has settled and Kea MT mode has been firmly established.
+    io_service->post([&]() { service_->startClientAndListener(); } );
+}
+
+HAImpl::~HAImpl() {
+    if (service_) {
+        // Shut down the services explicitly, we need finer control
+        // than relying on destruction order.
+        service_->stopClientAndListener();
+    }
 }
 
 void

diff --git a/src/hooks/dhcp/high_availability/ha_impl.h b/src/hooks/dhcp/high_availability/ha_impl.h
index 3f04517999198c92aba08cc83688cb1d8daa1ac7..60dd3bb77f9e7a3bbabf196b11ecfd43ef80353f 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_impl.h
+++ b/src/hooks/dhcp/high_availability/ha_impl.h
@@ -54,6 +54,11 @@ public:
                       const dhcp::NetworkStatePtr& network_state,
                       const HAServerType& server_type);
 
+    /// @brief Destructor.
+    ~HAImpl();
+
+public:
+
     /// @brief Returns parsed configuration.
     HAConfigPtr getConfig() const {
         return (config_);

diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc
index a0efbf74b7d2e6def5e60da656420a6e758fa25e..b4ada692342633c0d2a03f0193b4cca02ef40d96 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -68,8 +68,37 @@ HAService::HAService(const IOServicePtr& io_service, const NetworkStatePtr& netw
 
     startModel(HA_WAITING_ST);
 
-    // Start client and/or listener.
-    startClientAndListener();
+    // Create the client and(or) listener as appropriate.
+    if (!config_->getEnableMultiThreading()) {
+        // Not configured for multi-threading, start a client in ST mode.
+        client_.reset(new HttpClient(*io_service_, 0));
+    } else {
+        // Start a client in MT mode.
+        client_.reset(new HttpClient(*io_service_,
+                      config_->getHttpClientThreads()));
+
+        // If we're configured to use our own listener create and start it.
+        if (config_->getHttpDedicatedListener()) {
+            // Get the server address and port from this server's URL.
+            auto my_url = config_->getThisServerConfig()->getUrl();
+            IOAddress server_address(IOAddress::IPV4_ZERO_ADDRESS());
+            try {
+                // Since we do not currently support hostname resolution,
+                // we need to make sure we have an IP address here.
+                server_address = IOAddress(my_url.getStrippedHostname());
+            } catch (const std::exception& ex) {
+                isc_throw(Unexpected, "server Url:" << my_url.getStrippedHostname()
+                          << " is not a valid IP address");
+            }
+
+            // Fetch how many threads the listener will use.
+            uint32_t listener_threads = config_->getHttpListenerThreads();
+
+            // Instantiate the listener.
+            listener_.reset(new CmdHttpListener(server_address, my_url.getPort(),
+                                            listener_threads));
+        }
+    }
 
     LOG_INFO(ha_logger, HA_SERVICE_STARTED)
         .arg(HAConfig::HAModeToString(config->getHAMode()))
@@ -2788,38 +2817,7 @@ HAService::getPendingRequestInternal(const QueryPtrType& query) {
 
 void
 HAService::startClientAndListener() {
-    // If we're not configured for multi-threading, then we start
-    // a client in ST mode and return.
-    if (!config_->getEnableMultiThreading()) {
-        client_.reset(new HttpClient(*io_service_, 0));
-        return;
-    }
-
-    // Start a client in MT mode.
-    client_.reset(new HttpClient(*io_service_, 
-                  config_->getHttpClientThreads()));
-
-    // If we're configured to use our own listener create and start it.
-    if (config_->getHttpDedicatedListener()) {
-        // Get the server address and port from this server's URL.
-        auto my_url = config_->getThisServerConfig()->getUrl();
-        IOAddress server_address(IOAddress::IPV4_ZERO_ADDRESS());
-        try {
-            // Since we do not currently support hostname resolution,
-            // we need to make sure we have an IP address here.
-            server_address = IOAddress(my_url.getStrippedHostname());
-        } catch (const std::exception& ex) {
-            isc_throw(Unexpected, "server Url:" << my_url.getStrippedHostname()
-                      << " is not a valid IP address");
-        }
-
-        // Fetch how many threads the listener will use.
-        uint32_t listener_threads = config_->getHttpListenerThreads();
-
-        // Instantiate the listener.
-        listener_.reset(new CmdHttpListener(server_address, my_url.getPort(),
-                                            listener_threads));
-        // Start the listener listening.
+    if (listener_) {
         listener_->start();
     }
 }

diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h
index 41d9724c1b3a74fee34e5ac5af119d657799d379..ff91817d29a4c42ad039871f9619a33b3cb29807 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.h
+++ b/src/hooks/dhcp/high_availability/ha_service.h
@@ -1002,11 +1002,25 @@ public:
     /// @return Pointer to the response to the ha-maintenance-cancel.
     data::ConstElementPtr processMaintenanceCancel();
 
-protected:
+    /// @brief Start the client and(or) listener instances.
+    ///
+    /// Starts the dedicated listener thread pool, if the listener exists.
+    /// Nothing is required for the client as it does not currently support
+    /// a discrete "start" method, rather it is "started" during its
+    /// construction.
     void startClientAndListener();
 
+    /// @brief Stop the client and(or) listener instances.
+    ///
+    /// Closing connections and stops the thread pools for the client
+    /// and listener, if they exist.
+    ///
+    /// @note Once stopped the service cannot be restarted, it must
+    /// be recreated.
     void stopClientAndListener();
 
+protected:
+
     /// @brief Checks if the response is valid or contains an error.
     ///
     /// The response must be non-null, must contain a JSON body and must

diff --git a/src/hooks/dhcp/high_availability/tests/ha_mt_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_mt_unittest.cc
index 3c47568fcc1e30162ec95fa9fdd5c7e04ba0442d..f49088207cd652f77853ab806ea2e43a1b5e6b09 100644 (file)
--- a/src/hooks/dhcp/high_availability/tests/ha_mt_unittest.cc
+++ b/src/hooks/dhcp/high_availability/tests/ha_mt_unittest.cc
@@ -371,6 +371,8 @@ TEST_F(HAMtServiceTest, multiThreadingStartup) {
         TestHAServicePtr service;
         ASSERT_NO_THROW_LOG(service.reset(new TestHAService(io_service_, network_state_,
                                                             ha_config)));
+        ASSERT_NO_THROW(service->startClientAndListener());
+
         // Verify the configuration is as expected.
         if (!scenario.exp_ha_mt_enabled_) {
             // When HA+MT is disabled, client should be single-threaded.