refs #60 Fixed an issue where the HNZ::receive thread could be starte…

…d 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 <[email protected]>

include/hnzconnection.h

@@ -13,6 +13,7 @@
 
 #include <atomic>
 #include <thread>
+#include <mutex>
 
 #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<std::recursive_mutex> 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<std::recursive_mutex> 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<HNZPath*, HNZPath*> getBothPath() {
+    std::lock_guard<std::recursive_mutex> 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<bool> m_is_running;  // If false, the connection thread will stop
   uint64_t m_current;         // Store the last time requested

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();

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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> lock(m_path_mutex);
   if (!m_active_path->command_sent.empty()) {
     list<Command_message>::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<std::recursive_mutex> 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<std::recursive_mutex> lock(m_path_mutex);
   m_active_path->gi_repeat = 0;
   m_active_path->sendGeneralInterrogation();
 }