fix(logging): A potential deadlock is removed in logs management.

Another bug is fixed: a condition variable waited for the wrong variable while querying the version in connectors.

REFS: MON-10885

CMakeLists.txt

@@ -73,7 +73,7 @@ link_directories(${nlohmann_json_LIB_DIRS})
 # Version.
 set(CENTREON_ENGINE_MAJOR 21)
 set(CENTREON_ENGINE_MINOR 04)
-set(CENTREON_ENGINE_PATCH 3)
+set(CENTREON_ENGINE_PATCH 4)
 if (CENTREON_ENGINE_PRERELEASE)
   set(CENTREON_ENGINE_VERSION "${CENTREON_ENGINE_MAJOR}.${CENTREON_ENGINE_MINOR}.${CENTREON_ENGINE_PATCH}-${CENTREON_ENGINE_PRERELEASE}")
 else ()

inc/com/centreon/engine/commands/connector.hh

@@ -90,6 +90,7 @@ class connector : public command, public process_listener {
   bool _is_running;
   std::unordered_map<uint64_t, std::shared_ptr<query_info> > _queries;
   bool _query_quit_ok;
+  bool _version_set;
   bool _query_version_ok;
   mutable std::mutex _lock;
   process _process;

inc/com/centreon/engine/logging/broker.hh

@@ -34,7 +34,7 @@ namespace logging {
  */
 class broker : public com::centreon::logging::backend {
   bool _enable;
-  std::thread::id _thread;
+  std::thread::id _thread_id;
 
  public:
   broker();

src/commands/connector.cc

@@ -20,6 +20,7 @@
 #include <unistd.h>
 #include <cstdlib>
 
+#include <array>
 #include <list>
 #include "com/centreon/engine/exceptions/error.hh"
 #include "com/centreon/engine/globals.hh"
@@ -62,6 +63,7 @@ connector::connector(const std::string& connector_name,
       process_listener(),
       _is_running(false),
       _query_quit_ok(false),
+      _version_set{false},
       _query_version_ok(false),
       _process(this, true, true, false),  // Disable stderr.
       _try_to_restart(true),
@@ -256,14 +258,15 @@ void connector::set_command_line(const std::string& command_line) {
  */
 void connector::data_is_available(process& p) noexcept {
   typedef void (connector::*recv_query)(char const*);
-  static recv_query tab_recv_query[] = {nullptr,
-                                        &connector::_recv_query_version,
-                                        nullptr,
-                                        &connector::_recv_query_execute,
-                                        nullptr,
-                                        &connector::_recv_query_quit,
-                                        &connector::_recv_query_error,
-                                        nullptr};
+  static const std::array<recv_query, 8> tab_recv_query{
+      nullptr,
+      &connector::_recv_query_version,
+      nullptr,
+      &connector::_recv_query_execute,
+      nullptr,
+      &connector::_recv_query_quit,
+      &connector::_recv_query_error,
+      nullptr};
 
   try {
     logger(dbg_commands, basic)
@@ -286,7 +289,7 @@ void connector::data_is_available(process& p) noexcept {
           size_t pos(_data_available.find(ending));
           if (pos == std::string::npos)
             break;
-          responses.push_back(_data_available.substr(0, pos));
+          responses.emplace_back(_data_available.substr(0, pos));
           _data_available.erase(0, pos + ending.size());
         }
       }
@@ -297,18 +300,14 @@ void connector::data_is_available(process& p) noexcept {
     }
 
     // Parse queries responses.
-    for (std::list<std::string>::const_iterator it(responses.begin()),
-         end(responses.end());
-         it != end; ++it) {
-      char const* data(it->c_str());
+    for (auto& str : responses) {
+      char const* data = str.c_str();
       char* endptr(nullptr);
       uint32_t id(strtol(data, &endptr, 10));
       logger(dbg_commands, basic)
           << "connector::data_is_available: request id=" << id;
       // Invalid query.
-      if (data == endptr ||
-          id >= sizeof(tab_recv_query) / sizeof(*tab_recv_query) ||
-          !tab_recv_query[id])
+      if (data == endptr || id >= tab_recv_query.size() || !tab_recv_query[id])
         logger(log_runtime_warning, basic) << "Warning: Connector '" << _name
                                            << "' "
                                               "received bad request ID: "
@@ -418,6 +417,7 @@ void connector::_connector_start() {
 
     // Reset variables.
     _query_quit_ok = false;
+    _version_set = false;
     _query_version_ok = false;
     _is_running = false;
   }
@@ -432,10 +432,10 @@ void connector::_connector_start() {
     _send_query_version();
 
     // Waiting connector version, or 1 seconds.
-    bool is_timeout{
-        _cv_query.wait_for(
-            lock, std::chrono::seconds(config->service_check_timeout())) ==
-        std::cv_status::timeout};
+    bool is_timeout{!_cv_query.wait_for(
+        lock, std::chrono::seconds(config->service_check_timeout()),
+        [this] { return _version_set; })};
+
     if (is_timeout || !_query_version_ok) {
       _process.kill();
       _try_to_restart = false;
@@ -678,6 +678,7 @@ void connector::_recv_query_version(char const* data) {
 
   LOCK_GUARD(lock, _lock);
   _query_version_ok = version_ok;
+  _version_set = true;
   _cv_query.notify_all();
 }
 

src/logging/broker.cc

@@ -19,7 +19,6 @@
 #include "com/centreon/engine/broker.hh"
 #include <cstring>
 #include <mutex>
-#include <thread>
 #include "com/centreon/engine/logging/broker.hh"
 #include "com/centreon/engine/logging/logger.hh"
 #include "com/centreon/exceptions/basic.hh"
@@ -33,8 +32,8 @@ using namespace com::centreon::engine::logging;
  */
 broker::broker()
     : backend(false, false, com::centreon::logging::none, false),
-      _enable(false) {
-  memset(&_thread, 0, sizeof(_thread));
+      _enable(false),
+      _thread_id{} {
   open();
 }
 
@@ -43,9 +42,8 @@ broker::broker()
  *
  *  @param[in] right Object to copy.
  */
-broker::broker(broker const& right) : backend(right), _enable(false) {
-  operator=(right);
-}
+broker::broker(broker const& right)
+    : backend(right), _enable(false), _thread_id{right._thread_id} {}
 
 /**
  *  Destructor.
@@ -64,10 +62,10 @@ broker::~broker() noexcept {
 broker& broker::operator=(broker const& right) {
   if (this != &right) {
     backend::operator=(right);
-    std::lock_guard<std::mutex> lock1(_lock);
-    std::lock_guard<std::mutex> lock2(right._lock);
-    _thread = right._thread;
+    std::lock_guard<std::recursive_mutex> lock1(_lock);
+    std::lock_guard<std::recursive_mutex> lock2(right._lock);
     _enable = right._enable;
+    _thread_id = right._thread_id;
   }
   return *this;
 }
@@ -76,7 +74,7 @@ broker& broker::operator=(broker const& right) {
  *  Close broker log.
  */
 void broker::close() noexcept {
-  std::lock_guard<std::mutex> lock(_lock);
+  std::lock_guard<std::recursive_mutex> lock(_lock);
   _enable = false;
 }
 
@@ -93,13 +91,12 @@ void broker::log(uint64_t types,
                  char const* message,
                  uint32_t size) noexcept {
   (void)verbose;
-  std::lock_guard<std::mutex> lock(_lock);
-
-  // Broker is only notified of non-debug log messages.
-  if (message && _enable) {
-    if (_thread != std::this_thread::get_id()) {
-      _thread = std::this_thread::get_id();
+  std::lock_guard<std::recursive_mutex> lock(_lock);
 
+  if (_thread_id != std::this_thread::get_id()) {
+    // Broker is only notified of non-debug log messages.
+    if (message && _enable) {
+      _thread_id = std::this_thread::get_id();
       // Copy message because broker module might modify it.
       unique_array_ptr<char> copy(new char[size + 1]);
       strncpy(copy.get(), message, size);
@@ -108,9 +105,7 @@ void broker::log(uint64_t types,
       // Event broker callback.
       broker_log_data(NEBTYPE_LOG_DATA, NEBFLAG_NONE, NEBATTR_NONE, copy.get(),
                       types, time(NULL), NULL);
-
-      // Reset thread.
-      memset(&_thread, 0, sizeof(_thread));
+      _thread_id = std::thread::id();
     }
   }
 }
@@ -119,14 +114,14 @@ void broker::log(uint64_t types,
  *  Open broker log.
  */
 void broker::open() {
-  std::lock_guard<std::mutex> lock(_lock);
+  std::lock_guard<std::recursive_mutex> lock(_lock);
   _enable = true;
 }
 
 /**
  *  Open borker log.
  */
 void broker::reopen() {
-  std::lock_guard<std::mutex> lock(_lock);
+  std::lock_guard<std::recursive_mutex> lock(_lock);
   _enable = true;
 }

src/nebmods.cc

@@ -447,10 +447,8 @@ int neb_make_callbacks(int callback_type, void* data) {
   if (callback_type < 0 || callback_type >= NEBCALLBACK_NUMITEMS)
     return ERROR;
 
-  if (callback_type != NEBCALLBACK_LOG_DATA) {
-    logger(dbg_eventbroker, more)
-        << "Making callbacks (type " << callback_type << ")...";
-  }
+  logger(dbg_eventbroker, more)
+      << "Making callbacks (type " << callback_type << ")...";
 
   /* make the callbacks... */
   for (temp_callback = neb_callback_list[callback_type]; temp_callback != NULL;
@@ -465,11 +463,9 @@ int neb_make_callbacks(int callback_type, void* data) {
     cbresult = (*neb.func)(callback_type, data);
 
     total_callbacks++;
-    if (callback_type != NEBCALLBACK_LOG_DATA) {
-      logger(dbg_eventbroker, most)
-          << "Callback #" << total_callbacks << " (type " << callback_type
-          << ") return (code = " << cbresult << ")";
-    }
+    logger(dbg_eventbroker, most)
+        << "Callback #" << total_callbacks << " (type " << callback_type
+        << ") return (code = " << cbresult << ")";
 
     /* module wants to cancel callbacks to other modules (and potentially cancel
      * the default handling of an event) */