Skip to content

Commit

Permalink
MON-147699 check_exec doesn't lock on recursion (#1688)
Browse files Browse the repository at this point in the history 
* check_exec doesn't lock on recursion

* implement process class with a template parameter
  • Loading branch information
@jean-christophe81
jean-christophe81 committed Sep 13, 2024
1 parent 022f000 commit 564c934
Show file tree
Hide file tree
Showing 7 changed files with 144 additions and 66 deletions.
8 changes: 5 additions & 3 deletions agent/inc/com/centreon/agent/check_exec.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ namespace detail {
* ensure that completion is called for the right process and not for the
* previous one
*/
class process : public common::process {
class process : public common::process<false> {
bool _process_ended;
bool _stdout_eof;
std::string _stdout;
Expand All @@ -54,9 +54,11 @@ class process : public common::process {

void start(unsigned running_index);

void kill() { common::process::kill(); }
void kill() { common::process<false>::kill(); }

int get_exit_status() const { return common::process::get_exit_status(); }
int get_exit_status() const {
return common::process<false>::get_exit_status();
}

const std::string& get_stdout() const { return _stdout; }

Expand Down
10 changes: 5 additions & 5 deletions agent/src/check_exec.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ detail::process::process(const std::shared_ptr<asio::io_context>& io_context,
const std::shared_ptr<spdlog::logger>& logger,
const std::string& cmd_line,
const std::shared_ptr<check_exec>& parent)
: common::process(io_context, logger, cmd_line), _parent(parent) {}
: common::process<false>(io_context, logger, cmd_line), _parent(parent) {}

/**
* @brief start a new process, if a previous one is already running, it's killed
Expand All @@ -44,7 +44,7 @@ void detail::process::start(unsigned running_index) {
_stdout_eof = false;
_running_index = running_index;
_stdout.clear();
common::process::start_process(false);
common::process<false>::start_process(false);
}

/**
Expand All @@ -61,7 +61,7 @@ void detail::process::on_stdout_read(const boost::system::error_code& err,
_stdout_eof = true;
_on_completion();
}
common::process::on_stdout_read(err, nb_read);
common::process<false>::on_stdout_read(err, nb_read);
}

/**
Expand All @@ -76,7 +76,7 @@ void detail::process::on_stderr_read(const boost::system::error_code& err,
SPDLOG_LOGGER_ERROR(_logger, "process error: {}",
std::string_view(_stderr_read_buffer, nb_read));
}
common::process::on_stderr_read(err, nb_read);
common::process<false>::on_stderr_read(err, nb_read);
}

/**
Expand All @@ -91,7 +91,7 @@ void detail::process::on_process_end(const boost::system::error_code& err,
_stdout += fmt::format("fail to execute process {} : {}", get_exe_path(),
err.message());
}
common::process::on_process_end(err, raw_exit_status);
common::process<false>::on_process_end(err, raw_exit_status);
_process_ended = true;
_on_completion();
}
Expand Down
2 changes: 2 additions & 0 deletions agent/src/main.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ static std::string read_file(const std::string& file_path) {
ss << file.rdbuf();
file.close();
return ss.str();
} else {
SPDLOG_LOGGER_ERROR(g_logger, "fail to open {}", file_path);
}
} catch (const std::exception& e) {
SPDLOG_LOGGER_ERROR(g_logger, "fail to read {}: {}", file_path, e.what());
Expand Down
23 changes: 23 additions & 0 deletions agent/test/check_exec_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,26 @@ TEST(check_exec_test, bad_command) {
"or directory");
#endif
}

TEST(check_exec_test, recurse_not_lock) {
command_line = ECHO_PATH " hello toto";
std::condition_variable cond;
unsigned cpt = 0;
std::shared_ptr<check_exec> check = check_exec::load(
g_io_context, spdlog::default_logger(), time_point(), serv, cmd_name,
command_line, engine_to_agent_request_ptr(),
[&](const std::shared_ptr<com::centreon::agent::check>& caller, int,
const std::list<com::centreon::common::perfdata>& perfdata,
const std::list<std::string>& output) {
if (!cpt) {
++cpt;
caller->start_check(std::chrono::seconds(1));
} else
cond.notify_one();
});
check->start_check(std::chrono::seconds(1));

std::mutex mut;
std::unique_lock l(mut);
cond.wait(l);
}
94 changes: 62 additions & 32 deletions common/process/inc/com/centreon/common/process/process.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,33 @@ namespace detail {
struct boost_process;
} // namespace detail

namespace detail {
template <bool use_mutex>
class mutex;

template <bool use_mutex>
class lock;

template <>
class mutex<true> : public absl::Mutex {};

template <>
class lock<true> : public absl::MutexLock {
public:
lock(absl::Mutex* mut) : absl::MutexLock(mut) {}
};

template <>
class mutex<false> {};

template <>
class lock<false> {
public:
lock(mutex<false>* dummy_mut) {}
};

} // namespace detail

/**
* @brief This class allow to exec a process asynchronously.
* It's a base class. If you want to get stdin and stdout returned data, you
Expand All @@ -37,22 +64,23 @@ struct boost_process;
* When completion methods like on_stdout_read are called, _protect is already
* locked
*/
class process : public std::enable_shared_from_this<process> {

template <bool use_mutex = true>
class process : public std::enable_shared_from_this<process<use_mutex>> {
using std::enable_shared_from_this<process<use_mutex>>::shared_from_this;
std::string _exe_path;
std::vector<std::string> _args;

std::deque<std::shared_ptr<std::string>> _stdin_write_queue
ABSL_GUARDED_BY(_protect);
bool _write_pending ABSL_GUARDED_BY(_protect) = false;
std::deque<std::shared_ptr<std::string>> _stdin_write_queue;
bool _write_pending;

std::shared_ptr<detail::boost_process> _proc ABSL_GUARDED_BY(_protect);
std::shared_ptr<detail::boost_process> _proc;

int _exit_status = 0;

absl::Mutex _protect;
detail::mutex<use_mutex> _protect;

void stdin_write_no_lock(const std::shared_ptr<std::string>& data)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
void stdin_write_no_lock(const std::shared_ptr<std::string>& data);
void stdin_write(const std::shared_ptr<std::string>& data);

void stdout_read();
Expand All @@ -62,22 +90,18 @@ class process : public std::enable_shared_from_this<process> {
std::shared_ptr<asio::io_context> _io_context;
std::shared_ptr<spdlog::logger> _logger;

char _stdout_read_buffer[0x1000] ABSL_GUARDED_BY(_protect);
char _stderr_read_buffer[0x1000] ABSL_GUARDED_BY(_protect);
char _stdout_read_buffer[0x1000];
char _stderr_read_buffer[0x1000];

virtual void on_stdout_read(const boost::system::error_code& err,
size_t nb_read)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
size_t nb_read);
virtual void on_stderr_read(const boost::system::error_code& err,
size_t nb_read)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
size_t nb_read);

virtual void on_process_end(const boost::system::error_code& err,
int raw_exit_status)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
int raw_exit_status);

virtual void on_stdin_write(const boost::system::error_code& err)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(_protect);
virtual void on_stdin_write(const boost::system::error_code& err);

public:
template <typename string_iterator>
Expand Down Expand Up @@ -126,12 +150,13 @@ class process : public std::enable_shared_from_this<process> {
* @param arg_begin iterator to first argument
* @param arg_end iterator after the last argument
*/
template <bool use_mutex>
template <typename string_iterator>
process::process(const std::shared_ptr<asio::io_context>& io_context,
const std::shared_ptr<spdlog::logger>& logger,
const std::string_view& exe_path,
string_iterator arg_begin,
string_iterator arg_end)
process<use_mutex>::process(const std::shared_ptr<asio::io_context>& io_context,
const std::shared_ptr<spdlog::logger>& logger,
const std::string_view& exe_path,
string_iterator arg_begin,
string_iterator arg_end)
: _exe_path(exe_path),
_args(arg_begin, arg_end),
_io_context(io_context),
Expand All @@ -146,11 +171,13 @@ process::process(const std::shared_ptr<asio::io_context>& io_context,
* @param exe_path path of executable without argument
* @param args container of arguments
*/
template <bool use_mutex>
template <typename args_container>
process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
const std::shared_ptr<spdlog::logger>& logger,
const std::string_view& exe_path,
const args_container& args)
process<use_mutex>::process(
const std::shared_ptr<boost::asio::io_context>& io_context,
const std::shared_ptr<spdlog::logger>& logger,
const std::string_view& exe_path,
const args_container& args)
: _exe_path(exe_path),
_args(args),
_io_context(io_context),
Expand All @@ -166,11 +193,13 @@ process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
* @param exe_path path of executable without argument
* @param args brace of arguments {"--flag1", "arg1", "-c", "arg2"}
*/
template <bool use_mutex>
template <typename string_type>
process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
const std::shared_ptr<spdlog::logger>& logger,
const std::string_view& exe_path,
const std::initializer_list<string_type>& args)
process<use_mutex>::process(
const std::shared_ptr<boost::asio::io_context>& io_context,
const std::shared_ptr<spdlog::logger>& logger,
const std::string_view& exe_path,
const std::initializer_list<string_type>& args)
: _exe_path(exe_path), _io_context(io_context), _logger(logger) {
_args.reserve(args.size());
for (const auto& str : args) {
Expand All @@ -185,8 +214,9 @@ process::process(const std::shared_ptr<boost::asio::io_context>& io_context,
* can be used to construct a std::string
* @param content
*/
template <bool use_mutex>
template <typename string_class>
void process::write_to_stdin(const string_class& content) {
void process<use_mutex>::write_to_stdin(const string_class& content) {
stdin_write(std::make_shared<std::string>(content));
}

Expand Down
Loading

0 comments on commit 564c934

Please sign in to comment.