From: Thomas Markwalder Date: Tue, 20 Apr 2021 19:48:33 +0000 (+0000) Subject: [#1736] Address race conditions revolving around early listener start X-Git-Tag: Kea-1.9.7~29 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=df8640f872133d2db4019afe74cb109dfdcf7c35;p=thirdparty%2Fkea.git [#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 b2e3d55b26..eb5b5d219e 100644 --- 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(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 3f04517999..60dd3bb77f 100644 --- 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 a0efbf74b7..b4ada69234 100644 --- 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 41d9724c1b..ff91817d29 100644 --- 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 3c47568fcc..f49088207c 100644 --- 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.