From 3745fc899776e48bca8ce2c71b2950ca0c97ae27 Mon Sep 17 00:00:00 2001 From: David Boucher Date: Tue, 10 Aug 2021 14:13:23 +0200 Subject: [PATCH] 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 | 2 +- inc/com/centreon/engine/commands/connector.hh | 1 + inc/com/centreon/engine/logging/broker.hh | 2 +- src/commands/connector.cc | 41 ++++++++++--------- src/logging/broker.cc | 37 ++++++++--------- src/nebmods.cc | 14 +++---- 6 files changed, 45 insertions(+), 52 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c6a0e7728..9c8690d50 100644 --- a/CMakeLists.txt +++ b/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 () diff --git a/inc/com/centreon/engine/commands/connector.hh b/inc/com/centreon/engine/commands/connector.hh index dfc609d4a..79cbfd4ab 100644 --- a/inc/com/centreon/engine/commands/connector.hh +++ b/inc/com/centreon/engine/commands/connector.hh @@ -90,6 +90,7 @@ class connector : public command, public process_listener { bool _is_running; std::unordered_map > _queries; bool _query_quit_ok; + bool _version_set; bool _query_version_ok; mutable std::mutex _lock; process _process; diff --git a/inc/com/centreon/engine/logging/broker.hh b/inc/com/centreon/engine/logging/broker.hh index d4549b62e..ad953329f 100644 --- a/inc/com/centreon/engine/logging/broker.hh +++ b/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(); diff --git a/src/commands/connector.cc b/src/commands/connector.cc index 1e6f2996b..27980fc58 100644 --- a/src/commands/connector.cc +++ b/src/commands/connector.cc @@ -20,6 +20,7 @@ #include #include +#include #include #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 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::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(); } diff --git a/src/logging/broker.cc b/src/logging/broker.cc index e69f0c210..190126124 100644 --- a/src/logging/broker.cc +++ b/src/logging/broker.cc @@ -19,7 +19,6 @@ #include "com/centreon/engine/broker.hh" #include #include -#include #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 lock1(_lock); - std::lock_guard lock2(right._lock); - _thread = right._thread; + std::lock_guard lock1(_lock); + std::lock_guard 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 lock(_lock); + std::lock_guard 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 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 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 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,7 +114,7 @@ void broker::log(uint64_t types, * Open broker log. */ void broker::open() { - std::lock_guard lock(_lock); + std::lock_guard lock(_lock); _enable = true; } @@ -127,6 +122,6 @@ void broker::open() { * Open borker log. */ void broker::reopen() { - std::lock_guard lock(_lock); + std::lock_guard lock(_lock); _enable = true; } diff --git a/src/nebmods.cc b/src/nebmods.cc index e6d00fb29..eefa12094 100644 --- a/src/nebmods.cc +++ b/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) */