Fixed possible infinite loop in Groot2Publisher when destructor is ca… (#1058)

* Fixed possible infinite loop in Groot2Publisher when destructor is called

* Made heartbeat read/writes an atomic operation in Groot2Publisher

* Cleanup gtest_groot2_publisher.cpp

* Added tsan_suppressions file to suppress TSAN warnings from zeromq

* Replaced atomics with mutex for type safety with chrono

* Fixed suppressions path

Author: alvintps

.github/workflows/cmake_ubuntu_sanitizers.yml

@@ -63,7 +63,7 @@ jobs:
         GTEST_COLOR: "On"
         ASAN_OPTIONS: "color=always"
         UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1:color=always"
-        TSAN_OPTIONS: "color=always"
+        TSAN_OPTIONS: "suppressions=../../../tests/tsan_suppressions.txt:color=always"
       # There is a known issue with TSAN on recent kernel versions. Without the vm.mmap_rnd_bits=28
       # workaround all binaries with TSan enabled crash with "FATAL: ThreadSanitizer: unexpected memory mapping"
       run: sudo sysctl vm.mmap_rnd_bits=28 && ctest --test-dir build/${{env.BUILD_TYPE}} --output-on-failure

src/loggers/groot2_publisher.cpp

@@ -81,7 +81,7 @@ struct Groot2Publisher::PImpl
 
   std::string tree_xml;
 
-  std::atomic_bool active_server = false;
+  std::atomic_bool active_server = true;
   std::thread server_thread;
 
   std::mutex status_mutex;
@@ -98,7 +98,8 @@ struct Groot2Publisher::PImpl
   std::unordered_map<uint16_t, Monitor::Hook::Ptr> pre_hooks;
   std::unordered_map<uint16_t, Monitor::Hook::Ptr> post_hooks;
 
-  std::chrono::system_clock::time_point last_heartbeat;
+  std::mutex last_heartbeat_mutex;
+  std::chrono::steady_clock::time_point last_heartbeat;
   std::chrono::milliseconds max_heartbeat_delay = std::chrono::milliseconds(5000);
 
   std::atomic_bool recording = false;
@@ -241,7 +242,6 @@ void Groot2Publisher::serverLoop()
 {
   auto const serialized_uuid = CreateRandomUUID();
 
-  _p->active_server = true;
   auto& socket = _p->server;
 
   auto sendErrorReply = [&socket](const std::string& msg) {
@@ -252,7 +252,10 @@ void Groot2Publisher::serverLoop()
   };
 
   // initialize _p->last_heartbeat
-  _p->last_heartbeat = std::chrono::system_clock::now();
+  {
+    const std::unique_lock lk(_p->last_heartbeat_mutex);
+    _p->last_heartbeat = std::chrono::steady_clock::now();
+  }
 
   while(_p->active_server)
   {
@@ -261,8 +264,12 @@ void Groot2Publisher::serverLoop()
     {
       continue;
     }
+
     // this heartbeat will help establishing if Groot is connected or not
-    _p->last_heartbeat = std::chrono::system_clock::now();
+    {
+      const std::unique_lock lk(_p->last_heartbeat_mutex);
+      _p->last_heartbeat = std::chrono::steady_clock::now();
+    }
 
     std::string const request_str = requestMsg[0].to_string();
     if(request_str.size() != Monitor::RequestHeader::size())
@@ -512,10 +519,13 @@ void Groot2Publisher::heartbeatLoop()
   {
     std::this_thread::sleep_for(std::chrono::milliseconds(10));
 
-    auto now = std::chrono::system_clock::now();
+    auto now = std::chrono::steady_clock::now();
     const bool prev_heartbeat = has_heartbeat;
 
-    has_heartbeat = (now - _p->last_heartbeat < _p->max_heartbeat_delay);
+    {
+      const std::unique_lock lk(_p->last_heartbeat_mutex);
+      has_heartbeat = (now - _p->last_heartbeat < _p->max_heartbeat_delay);
+    }
 
     // if we loose or gain heartbeat, disable/enable all breakpoints
     if(has_heartbeat != prev_heartbeat)

tests/CMakeLists.txt

@@ -12,6 +12,7 @@ set(BT_TESTS
   gtest_enums.cpp
   gtest_factory.cpp
   gtest_fallback.cpp
+  gtest_groot2_publisher.cpp
   gtest_parallel.cpp
   gtest_preconditions.cpp
   gtest_ports.cpp

tests/gtest_groot2_publisher.cpp

@@ -0,0 +1,48 @@
+#include <chrono>
+#include <cstdlib>
+#include <future>
+
+#include <behaviortree_cpp/loggers/groot2_protocol.h>
+#include <behaviortree_cpp/loggers/groot2_publisher.h>
+#include <gtest/gtest.h>
+
+using namespace std::chrono_literals;
+
+namespace
+{
+static const char* xml_text = R"(
+<root BTCPP_format="4">
+    <BehaviorTree ID="MainTree">
+        <ThrowRuntimeError/>
+    </BehaviorTree>
+</root>
+)";
+
+void throwRuntimeError()
+{
+  BT::BehaviorTreeFactory factory;
+  factory.registerSimpleAction("ThrowRuntimeError", [](BT::TreeNode&) -> BT::NodeStatus {
+    throw BT::RuntimeError("Oops!");
+  });
+
+  auto tree = factory.createTreeFromText(xml_text);
+  BT::Groot2Publisher publisher(tree);
+  EXPECT_THROW(tree.tickExactlyOnce(), BT::RuntimeError);
+}
+}  // namespace
+
+TEST(Groot2PublisherTest, EnsureNoInfiniteLoopOnThrow)
+{
+  EXPECT_EXIT(
+      {
+        auto fut = std::async(std::launch::async, throwRuntimeError);
+        auto status = fut.wait_for(1s);  // shouldn't run for more than 1 second
+        if(status != std::future_status::ready)
+        {
+          std::cerr << "Test took too long. Possible infinite loop.\n";
+          std::exit(EXIT_FAILURE);
+        }
+        std::exit(EXIT_SUCCESS);
+      },
+      ::testing::ExitedWithCode(EXIT_SUCCESS), ".*");
+}

tests/tsan_suppressions.txt

@@ -0,0 +1,4 @@
+# ThreadSanitizer suppressions file for behaviortree_cpp_test
+
+# ZeroMQ false positives
+race:zmq::epoll_t::add_fd