Merge pull request #87 from fledge-power/86-plugin-wont-restart-after…

…-failed-reconfigure

refs #86 Ensured HNZ connection will restart whenever a valid configu…

include/hnz.h

@@ -74,13 +74,15 @@ class HNZ {
 
   /**
    * Start the HZN south plugin
+   * @param requestedStart tells if start was requested by the Fledge API
    */
-  void start();
+  void start(bool requestedStart = false);
 
   /**
    * Stop the HZN south plugin
+   * @param requestedStop tells if stop was requested by the Fledge API
    */
-  void stop();
+  void stop(bool requestedStop = false);
 
   /**
    * Save the callback function and its data
@@ -170,7 +172,11 @@ class HNZ {
   void sendInitialGI();
 
 private:
+  // Tells if the plugin is currently running
   std::atomic<bool> m_is_running{false};
+  // Tells if the plugin should be running (eg: if you should be restarted after a configuration change)
+  std::atomic<bool> m_should_run{false};
+
   // Receiving threads
   std::unique_ptr<std::thread> m_receiving_thread_A;
   std::unique_ptr<std::thread> m_receiving_thread_B;

src/hnz.cpp

@@ -21,14 +21,23 @@ HNZ::HNZ() {}
 
 HNZ::~HNZ() {
   if (m_is_running) {
-    stop();
+    stop(true);
   }
 }
 
-void HNZ::start() {
+void HNZ::start(bool requestedStart /*= false*/) {
   std::lock_guard<std::recursive_mutex> guard(m_configMutex);
   std::string beforeLog = HnzUtility::NamePlugin + " - HNZ::start -";
 
+  if (requestedStart) {
+    m_should_run = true;
+  }
+
+  if (m_is_running) {
+    HnzUtility::log_info("%s HNZ south plugin already started", beforeLog.c_str());
+    return;
+  }
+
   if (!m_hnz_conf->is_complete()) {
     HnzUtility::log_info("%s HNZ south plugin can't start because configuration is incorrect.", beforeLog.c_str());
     return;
@@ -53,11 +62,15 @@ void HNZ::start() {
   m_hnz_connection->start();
 }
 
-void HNZ::stop() {
+void HNZ::stop(bool requestedStop /*= false*/) {
   std::string beforeLog = HnzUtility::NamePlugin + " - HNZ::stop -";
   HnzUtility::log_info("%s Starting shutdown of HNZ plugin", beforeLog.c_str());
   m_is_running = false;
 
+  if (requestedStop) {
+    m_should_run = false;
+  }
+
   // Connection must be stopped before management threads of both path
   // or join on both receive threads will hang forever
   if (m_hnz_connection != nullptr) {
@@ -104,7 +117,7 @@ void HNZ::setJsonConfig(const string& protocol_conf_json, const string& msg_conf
     HnzUtility::log_info("%s No new configuration provided to reconfigure, skipping", beforeLog.c_str());
     return;
   }
-  bool was_running = m_is_running;
+
   if (m_is_running) {
     HnzUtility::log_info("%s Configuration change requested, stopping the plugin", beforeLog.c_str());
     stop();
@@ -129,7 +142,7 @@ void HNZ::setJsonConfig(const string& protocol_conf_json, const string& msg_conf
   m_test_msg_receive = m_hnz_conf->get_test_msg_receive();
   m_hnz_connection = make_unique<HNZConnection>(m_hnz_conf, this);
 
-  if (was_running) {
+  if (m_should_run) {
     HnzUtility::log_warn("%s Restarting the plugin...", beforeLog.c_str());
     start();
   }

src/plugin.cpp

@@ -190,7 +190,7 @@ void plugin_start(PLUGIN_HANDLE *handle) {
   std::string beforeLog = HnzUtility::NamePlugin + " - plugin_start -";
   HnzUtility::log_info("%s Starting the plugin...", beforeLog.c_str());
   auto hnz = reinterpret_cast<HNZ *>(handle);
-  hnz->start();
+  hnz->start(true);
   HnzUtility::log_info("%s Plugin started", beforeLog.c_str());
 }
 

tests/test_hnz.cpp

@@ -204,13 +204,13 @@ class HNZTest : public testing::Test {
     const std::string& protocol_stack = protocol_stack_generator(port, port2);
     std::string configure = std::regex_replace(configureTemplate, std::regex("<protocol_stack>"), protocol_stack);
     configure = std::regex_replace(configure, std::regex("<exchanged_data>"), exchanged_data_def);
-    auto config = new ConfigCategory("newConfig", configure);
-    hnz->reconfigure(*config);
+    ConfigCategory config("newConfig", configure);
+    hnz->reconfigure(config);
   }
 
   static void startHNZ(int port, int port2) {
     initConfig(port, port2);
-    hnz->start();
+    hnz->start(true);
   }
 
   template<class... Args>
@@ -661,13 +661,13 @@ class ServersWrapper {
     }
     std::shared_ptr<BasicHNZServer> server1() {
       if (m_server1 && !m_server1->HNZServerIsReady()) {
-        m_server1 = nullptr;
+        return nullptr;
       }
       return m_server1;
     }
     std::shared_ptr<BasicHNZServer> server2() {
       if (m_server2 && !m_server2->HNZServerIsReady()) {
-        m_server2 = nullptr;
+        return nullptr;
       }
       return m_server2;
     }
@@ -2155,6 +2155,107 @@ TEST_F(HNZTest, ReconfigureWhileConnectionActive) {
   if(HasFatalFailure()) return;
 }
 
+TEST_F(HNZTest, ReconfigureBadConfig) {
+  ServersWrapper wrapper(0x05, getNextPort());
+  BasicHNZServer* server = wrapper.server1().get();
+  ASSERT_NE(server, nullptr) << "Something went wrong. Connection is not established in 10s...";
+  validateAllTIQualityUpdate(true, false);
+  if(HasFatalFailure()) return;
+
+  debug_print("[HNZ south plugin] Send bad plugin configuration");
+  clearReadings();
+  static const std::string badConfig = QUOTE({
+    "exchanged_data" : {
+      "value": 42
+    }
+  });
+  ConfigCategory config("newConfig", badConfig);
+  hnz->reconfigure(config);
+
+  // Check that connection was lost
+  waitUntil(southEventsReceived, 1, 1000);
+  // Check that ingestCallback had been called only one time
+  ASSERT_EQ(southEventsReceived, 1);
+  // Validate new connection state
+  std::shared_ptr<Reading> currentReading = popFrontReadingsUntil("TEST_STATUS");
+  validateSouthEvent(currentReading, "TEST_STATUS", {
+    {"connx_status", "not connected"},
+  });
+  if(HasFatalFailure()) return;
+
+  // No quality update message as new config contains no TI
+  ASSERT_EQ(dataObjectsReceived, 0);
+
+  // Also stop the server as it is unable to reconnect on the fly
+  debug_print("[HNZ server] Request server stop...");
+  server->stopHNZServer();
+  debug_print("[HNZ server] Request server start...");
+  server->startHNZServer();
+
+  // Check that connection cannot be established as client is no longer running due to invalid configuration
+  BasicHNZServer* deadServer = wrapper.server1().get();
+  ASSERT_EQ(deadServer, nullptr) << "Something went wrong. Server should not be able to reconnect, but it did!";
+
+  // This calls HNZ::reconfigure() again, causing a reconnect of the client
+  debug_print("[HNZ south plugin] Send good plugin configuration");
+  clearReadings();
+  wrapper.initHNZPlugin();
+
+  // Check that connection attempt to reopen on client side
+  validateAllTIQualityUpdate(true, false, true);
+  if(HasFatalFailure()) return;
+
+  // Restart the server
+  debug_print("[HNZ server] Request server stop 2...");
+  server->stopHNZServer();
+  debug_print("[HNZ server] Request server start 2...");
+  server->startHNZServer();
+
+  // Check that the server is reconnected after reconfigure
+  server = wrapper.server1().get();
+  ASSERT_NE(server, nullptr) << "Something went wrong. Connection 2 is not established in 10s...";
+  // Wait for initial CG request
+  waitUntil(southEventsReceived, 2, 1000);
+  // Check that ingestCallback had been called only for two GI status updates
+  ASSERT_EQ(southEventsReceived, 2);
+  resetCounters();
+  // Validate reconnection
+  currentReading = popFrontReadingsUntil("TEST_STATUS");
+  validateSouthEvent(currentReading, "TEST_STATUS", {
+    {"connx_status", "started"},
+  });
+  if(HasFatalFailure()) return;
+  // Validate new GI state
+  currentReading = popFrontReadingsUntil("TEST_STATUS");
+  validateSouthEvent(currentReading, "TEST_STATUS", {
+    {"gi_status", "started"},
+  });
+  if(HasFatalFailure()) return;
+
+  // Complete CG request by sending all expected TS
+  server->sendFrame({0x16, 0x33, 0x00, 0x00, 0x00, 0x00}, false);
+  server->sendFrame({0x16, 0x39, 0x00, 0x02, 0x00, 0x00}, false);
+  debug_print("[HNZ Server] TSCG sent");
+  waitUntil(southEventsReceived, 2, 1000);
+  // Check that ingestCallback had been called only for two GI status updates
+  ASSERT_EQ(southEventsReceived, 2);
+  resetCounters();
+  currentReading = popFrontReadingsUntil("TEST_STATUS");
+  validateSouthEvent(currentReading, "TEST_STATUS", {
+    {"gi_status", "in progress"},
+  });
+  if(HasFatalFailure()) return;
+  currentReading = popFrontReadingsUntil("TEST_STATUS");
+  validateSouthEvent(currentReading, "TEST_STATUS", {
+    {"gi_status", "finished"},
+  });
+  if(HasFatalFailure()) return;
+
+  // Manually stop the server here or we may end up in a deadlock in the HNZServer
+  debug_print("[HNZ server] Request server stop 3...");
+  server->stopHNZServer();
+}
+
 TEST_F(HNZTest, UnknownMessage) {
   ServersWrapper wrapper(0x05, getNextPort());
   BasicHNZServer* server = wrapper.server1().get();