From aaceb3e08d15befd306b44e3d0a42a33875a08ae Mon Sep 17 00:00:00 2001 From: Florent Peyrusse Date: Fri, 25 Aug 2023 16:45:29 +0200 Subject: [PATCH] refs #60 Fixed an issue where the HNZ::receive thread could be started twice on the same path due to a path swap occuring during the sleep in HNZ::start, leading to infinite loop. Added mutex to prevent race condition on the active and passive path in HNZConnection. Signed-off-by: Florent Peyrusse --- include/hnzconnection.h | 21 +++++++++++++++++++-- src/hnz.cpp | 14 ++++++-------- src/hnzconnection.cpp | 31 +++++++++++++++++++++++-------- 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/include/hnzconnection.h b/include/hnzconnection.h index 8466dc7..d7e49b7 100644 --- a/include/hnzconnection.h +++ b/include/hnzconnection.h @@ -13,6 +13,7 @@ #include #include +#include #include "hnzconf.h" @@ -60,13 +61,28 @@ class HNZConnection { * commands). * @return the active path */ - HNZPath* getActivePath() { return m_active_path; }; + HNZPath* getActivePath() { + std::lock_guard lock(m_path_mutex); + return m_active_path; + }; /** * Get the path in stand-by. * @return the active path */ - HNZPath* getPassivePath() { return m_passive_path; }; + HNZPath* getPassivePath() { + std::lock_guard lock(m_path_mutex); + return m_passive_path; + }; + + /** + * Get both active and passive path (with a single lock) + * @return a path pair (active_path, passive_path) + */ + std::pair getBothPath() { + std::lock_guard lock(m_path_mutex); + return std::make_pair(m_active_path, m_passive_path); + }; /** * Switch between the active path and passive path. Must be called in case of @@ -101,6 +117,7 @@ class HNZConnection { private: HNZPath* m_active_path = nullptr; HNZPath* m_passive_path = nullptr; + std::recursive_mutex m_path_mutex; std::thread* m_messages_thread = nullptr; // Main thread that monitors messages std::atomic m_is_running; // If false, the connection thread will stop uint64_t m_current; // Store the last time requested diff --git a/src/hnz.cpp b/src/hnz.cpp index ddb10dc..c7804a7 100644 --- a/src/hnz.cpp +++ b/src/hnz.cpp @@ -43,15 +43,13 @@ void HNZ::start() { m_sendAllTMQualityReadings(true, false); m_sendAllTSQualityReadings(true, false); - m_receiving_thread_A = - new thread(&HNZ::receive, this, m_hnz_connection->getActivePath()); - - this_thread::sleep_for(milliseconds(1000)); - - HNZPath *passive_path = m_hnz_connection->getPassivePath(); - if (passive_path != nullptr) { + auto pathPair = m_hnz_connection->getBothPath(); + m_receiving_thread_A = new thread(&HNZ::receive, this, pathPair.first); + if (pathPair.second != nullptr) { + // Wait after getting the passive path pointer as connection init of active path may swap path + this_thread::sleep_for(milliseconds(1000)); // Path B is defined in the configuration - m_receiving_thread_B = new thread(&HNZ::receive, this, passive_path); + m_receiving_thread_B = new thread(&HNZ::receive, this, pathPair.second); } m_hnz_connection->start(); diff --git a/src/hnzconnection.cpp b/src/hnzconnection.cpp index d65ef1f..062ab0a 100644 --- a/src/hnzconnection.cpp +++ b/src/hnzconnection.cpp @@ -18,9 +18,12 @@ HNZConnection::HNZConnection(HNZConf* hnz_conf, HNZ* hnz_fledge) { this->m_hnz_fledge = hnz_fledge; // Create the path needed - m_active_path = new HNZPath(hnz_conf, this, false); - if (m_hnz_conf->get_ip_address_B() != "") { - m_passive_path = new HNZPath(hnz_conf, this, true); + { + std::lock_guard lock(m_path_mutex); + m_active_path = new HNZPath(hnz_conf, this, false); + if (m_hnz_conf->get_ip_address_B() != "") { + m_passive_path = new HNZPath(hnz_conf, this, true); + } } // Set settings for GI @@ -38,6 +41,7 @@ HNZConnection::~HNZConnection() { if (m_is_running) { stop(); } + std::lock_guard lock(m_path_mutex); if (m_active_path != nullptr) delete m_active_path; if (m_passive_path != nullptr) delete m_passive_path; } @@ -54,8 +58,11 @@ void HNZConnection::stop() { // Stop the path used (close the TCP connection and stop the threads that // manage HNZ connections) - if (m_active_path != nullptr) m_active_path->disconnect(); - if (m_passive_path != nullptr) m_passive_path->disconnect(); + { + std::lock_guard lock(m_path_mutex); + if (m_active_path != nullptr) m_active_path->disconnect(); + if (m_passive_path != nullptr) m_passive_path->disconnect(); + } // Wait for the end of the thread that manage the messages if (m_messages_thread != nullptr) { @@ -70,6 +77,7 @@ void HNZConnection::stop() { } void HNZConnection::checkGICompleted(bool success) { + std::lock_guard lock(m_path_mutex); // GI is a success if (success) { m_hnz_fledge->GICompleted(true); @@ -89,6 +97,7 @@ void HNZConnection::checkGICompleted(bool success) { } void HNZConnection::onGICompleted() { + std::lock_guard lock(m_path_mutex); m_active_path->gi_repeat = 0; } @@ -117,8 +126,11 @@ void HNZConnection::m_manageMessages() { m_update_current_time(); // Manage repeat/timeout for each path - m_check_timer(m_active_path); - m_check_timer(m_passive_path); + { + std::lock_guard lock(m_path_mutex); + m_check_timer(m_active_path); + m_check_timer(m_passive_path); + } // GI support m_check_GI(); @@ -161,6 +173,7 @@ void HNZConnection::m_check_timer(HNZPath* path) { } void HNZConnection::m_check_GI() { + std::lock_guard lock(m_path_mutex); // Check the status of an ongoing GI if (m_active_path->gi_repeat != 0) { if (m_active_path->gi_start_time + gi_time_max < m_current) { @@ -190,6 +203,7 @@ void HNZConnection::m_check_GI() { } void HNZConnection::m_check_command_timer() { + std::lock_guard lock(m_path_mutex); if (!m_active_path->command_sent.empty()) { list::iterator it = m_active_path->command_sent.begin(); while (it != m_active_path->command_sent.end()) { @@ -221,7 +235,7 @@ void HNZConnection::m_update_quality_update_timer() { void HNZConnection::switchPath() { if (m_passive_path != nullptr) { HnzUtility::log_warn("Switching active and passive path."); - + std::lock_guard lock(m_path_mutex); // Check the status of the passive path before switching // Permute path HNZPath* temp = m_active_path; @@ -246,6 +260,7 @@ void HNZConnection::switchPath() { } void HNZConnection::sendInitialGI() { + std::lock_guard lock(m_path_mutex); m_active_path->gi_repeat = 0; m_active_path->sendGeneralInterrogation(); }