From f270686201c84a07f67a9615fd610d917fc8f802 Mon Sep 17 00:00:00 2001 From: Shauren Date: Sat, 14 Sep 2024 14:45:40 +0200 Subject: [PATCH] Core/Common: Output stdout/stderr from child process without waiting for it to finish --- src/common/Utilities/StartProcess.cpp | 259 ++++++++++------------ src/common/Utilities/StartProcess.h | 12 +- src/server/database/Updater/DBUpdater.cpp | 2 +- 3 files changed, 119 insertions(+), 154 deletions(-) diff --git a/src/common/Utilities/StartProcess.cpp b/src/common/Utilities/StartProcess.cpp index bc188a3f29e..378f73c6ba1 100644 --- a/src/common/Utilities/StartProcess.cpp +++ b/src/common/Utilities/StartProcess.cpp @@ -29,16 +29,43 @@ #include #include -using namespace boost::process; +namespace bp = boost::process; namespace Trinity { -template -static int CreateChildProcess(T waiter, std::string const& executable, - std::vector const& argsVector, - std::string const& logger, std::string const& input, - bool secure) +class AsyncProcessResultImplementation + : public AsyncProcessResult { + std::string const executable; + std::vector const args; + std::string const logger; + std::string const input_file; + bool const is_secure; + + std::atomic was_terminated; + + Optional> futureResult; + Optional my_child; + +public: + explicit AsyncProcessResultImplementation(std::string executable_, std::vector args_, + std::string logger_, std::string input_file_, + bool secure) + : executable(std::move(executable_)), args(std::move(args_)), + logger(std::move(logger_)), input_file(std::move(input_file_)), + is_secure(secure), was_terminated(false) { } + + AsyncProcessResultImplementation(AsyncProcessResultImplementation const&) = delete; + AsyncProcessResultImplementation& operator= (AsyncProcessResultImplementation const&) = delete; + AsyncProcessResultImplementation(AsyncProcessResultImplementation&&) = delete; + AsyncProcessResultImplementation& operator= (AsyncProcessResultImplementation&&) = delete; + + ~AsyncProcessResultImplementation() = default; + + int StartProcess() + { + ASSERT(!my_child, "Process started already!"); + #if TRINITY_COMPILER == TRINITY_COMPILER_MICROSOFT #pragma warning(push) #pragma warning(disable:4297) @@ -51,159 +78,95 @@ static int CreateChildProcess(T waiter, std::string const& executable, boost/process/pipe.hpp(304,42): message : see reference to class template instantiation 'boost::process::basic_pipebuf>' being compiled */ #endif - ipstream outStream; - ipstream errStream; + bp::ipstream outStream; + bp::ipstream errStream; #if TRINITY_COMPILER == TRINITY_COMPILER_MICROSOFT #pragma warning(pop) #endif - if (!secure) - { - TC_LOG_TRACE(logger, "Starting process \"{}\" with arguments: \"{}\".", - executable, fmt::join(argsVector, " ")); - } + if (!is_secure) + { + TC_LOG_TRACE(logger, "Starting process \"{}\" with arguments: \"{}\".", + executable, fmt::join(args, " ")); + } - // prepare file with only read permission (boost process opens with read_write) - auto inputFile = Trinity::make_unique_ptr_with_deleter(!input.empty() ? fopen(input.c_str(), "rb") : nullptr, &::fclose); + // prepare file with only read permission (boost process opens with read_write) + auto inputFile = Trinity::make_unique_ptr_with_deleter(!input_file.empty() ? fopen(input_file.c_str(), "rb") : nullptr, &::fclose); - // Start the child process - child c = [&]() - { + // Start the child process if (inputFile) { - // With binding stdin - return child{ - exe = boost::filesystem::absolute(executable).string(), - args = argsVector, - env = environment(boost::this_process::environment()), - std_in = inputFile.get(), - std_out = outStream, - std_err = errStream - }; + my_child.emplace( + bp::exe = boost::filesystem::absolute(executable).string(), + bp::args = args, + bp::env = bp::environment(boost::this_process::environment()), + bp::std_in = inputFile.get(), + bp::std_out = outStream, + bp::std_err = errStream + ); } else { - // Without binding stdin - return child{ - exe = boost::filesystem::absolute(executable).string(), - args = argsVector, - env = environment(boost::this_process::environment()), - std_in = boost::process::close, - std_out = outStream, - std_err = errStream - }; + my_child.emplace( + bp::exe = boost::filesystem::absolute(executable).string(), + bp::args = args, + bp::env = bp::environment(boost::this_process::environment()), + bp::std_in = boost::process::close, + bp::std_out = outStream, + bp::std_err = errStream + ); } - }(); - std::string line; - while (std::getline(outStream, line, '\n')) - { - std::erase(line, '\r'); - if (!line.empty()) - TC_LOG_INFO(logger, "{}", line); - } + std::future stdOutReader = std::async(std::launch::async, [&] + { + std::string line; + while (std::getline(outStream, line, '\n')) + { + std::erase(line, '\r'); + if (!line.empty()) + TC_LOG_INFO(logger, "{}", line); + } + }); - while (std::getline(errStream, line, '\n')) - { - std::erase(line, '\r'); - if (!line.empty()) - TC_LOG_ERROR(logger, "{}", line); - } + std::future stdErrReader = std::async(std::launch::async, [&] + { + std::string line; + while (std::getline(errStream, line, '\n')) + { + std::erase(line, '\r'); + if (!line.empty()) + TC_LOG_ERROR(logger, "{}", line); + } + }); - // Call the waiter in the current scope to prevent - // the streams from closing too early on leaving the scope. - int const result = waiter(c); + std::error_code ec; + my_child->wait(ec); + int32 const result = !ec && !was_terminated ? my_child->exit_code() : EXIT_FAILURE; + my_child.reset(); - if (!secure) - { - TC_LOG_TRACE(logger, ">> Process \"{}\" finished with return value {}.", + stdOutReader.wait(); + stdErrReader.wait(); + + if (!is_secure) + { + TC_LOG_TRACE(logger, ">> Process \"{}\" finished with return value {}.", executable, result); + } + + return result; } - return result; -} - -int StartProcess(std::string const& executable, std::vector const& args, - std::string const& logger, std::string input_file, bool secure) -{ - return CreateChildProcess([](child& c) -> int + void SetFuture(std::future result_) { - try - { - c.wait(); - return c.exit_code(); - } - catch (...) - { - return EXIT_FAILURE; - } - }, executable, args, logger, input_file, secure); -} - -class AsyncProcessResultImplementation - : public AsyncProcessResult -{ - std::string const executable; - std::vector const args; - std::string const logger; - std::string const input_file; - bool const is_secure; - - std::atomic was_terminated; - - // Workaround for missing move support in boost < 1.57 - Optional>> result; - Optional> my_child; - -public: - explicit AsyncProcessResultImplementation(std::string executable_, std::vector args_, - std::string logger_, std::string input_file_, - bool secure) - : executable(std::move(executable_)), args(std::move(args_)), - logger(std::move(logger_)), input_file(input_file_), - is_secure(secure), was_terminated(false) { } - - AsyncProcessResultImplementation(AsyncProcessResultImplementation const&) = delete; - AsyncProcessResultImplementation& operator= (AsyncProcessResultImplementation const&) = delete; - AsyncProcessResultImplementation(AsyncProcessResultImplementation&&) = delete; - AsyncProcessResultImplementation& operator= (AsyncProcessResultImplementation&&) = delete; - - int StartProcess() - { - ASSERT(!my_child, "Process started already!"); - - return CreateChildProcess([&](child& c) -> int - { - int result; - my_child = std::reference_wrapper(c); - - try - { - c.wait(); - result = c.exit_code(); - } - catch (...) - { - result = EXIT_FAILURE; - } - - my_child.reset(); - return was_terminated ? EXIT_FAILURE : result; - - }, executable, args, logger, input_file, is_secure); - } - - void SetFuture(std::future result_) - { - result = std::make_shared>(std::move(result_)); + futureResult.emplace(std::move(result_)); } /// Returns the future which contains the result of the process /// as soon it is finished. - std::future& GetFutureResult() override + std::future& GetFutureResult() override { - ASSERT(*result, "The process wasn't started!"); - return **result; + ASSERT(futureResult.has_value(), "The process wasn't started!"); + return *futureResult; } /// Tries to terminate the process @@ -212,23 +175,25 @@ public: if (my_child) { was_terminated = true; - try - { - my_child->get().terminate(); - } - catch(...) - { - // Do nothing - } + std::error_code ec; + my_child->terminate(ec); } } }; -std::shared_ptr - StartAsyncProcess(std::string executable, std::vector args, - std::string logger, std::string input_file, bool secure) +int StartProcess(std::string executable, std::vector args, + std::string logger, std::string input_file, bool secure) { - auto handle = std::make_shared( + AsyncProcessResultImplementation handle( + std::move(executable), std::move(args), std::move(logger), std::move(input_file), secure); + + return handle.StartProcess(); +} + +std::shared_ptr StartAsyncProcess(std::string executable, std::vector args, + std::string logger, std::string input_file, bool secure) +{ + std::shared_ptr handle = std::make_shared( std::move(executable), std::move(args), std::move(logger), std::move(input_file), secure); handle->SetFuture(std::async(std::launch::async, [handle] { return handle->StartProcess(); })); @@ -239,7 +204,7 @@ std::string SearchExecutableInPath(std::string const& filename) { try { - return search_path(filename).string(); + return bp::search_path(filename).string(); } catch (...) { diff --git a/src/common/Utilities/StartProcess.h b/src/common/Utilities/StartProcess.h index 71452315e77..b99a24a9f55 100644 --- a/src/common/Utilities/StartProcess.h +++ b/src/common/Utilities/StartProcess.h @@ -15,8 +15,8 @@ * with this program. If not, see . */ -#ifndef Process_h__ -#define Process_h__ +#ifndef TRINITYCORE_START_PROCESS_H +#define TRINITYCORE_START_PROCESS_H #include "Define.h" #include @@ -32,8 +32,8 @@ namespace Trinity /// When an input path is given, the file will be routed to the processes stdin. /// When the process is marked as secure no arguments are leaked to logs. /// Note that most executables expect it's name as the first argument. -TC_COMMON_API int StartProcess(std::string const& executable, std::vector const& args, - std::string const& logger, std::string input_file = "", +TC_COMMON_API int32 StartProcess(std::string executable, std::vector args, + std::string logger, std::string input_file = "", bool secure = false); /// Platform and library independent representation @@ -45,7 +45,7 @@ public: /// Returns the future which contains the result of the process /// as soon it is finished. - virtual std::future& GetFutureResult() = 0; + virtual std::future& GetFutureResult() = 0; /// Tries to terminate the process virtual void Terminate() = 0; @@ -67,4 +67,4 @@ TC_COMMON_API std::string SearchExecutableInPath(std::string const& filename); } // namespace Trinity -#endif // Process_h__ +#endif // TRINITYCORE_START_PROCESS_H diff --git a/src/server/database/Updater/DBUpdater.cpp b/src/server/database/Updater/DBUpdater.cpp index 3a75d90d547..ac7be7cb69b 100644 --- a/src/server/database/Updater/DBUpdater.cpp +++ b/src/server/database/Updater/DBUpdater.cpp @@ -423,7 +423,7 @@ void DBUpdater::ApplyFile(DatabaseWorkerPool& pool, std::string const& hos args.emplace_back(database); // Invokes a mysql process which doesn't leak credentials to logs - int const ret = Trinity::StartProcess(DBUpdaterUtil::GetCorrectedMySQLExecutable(), args, + int const ret = Trinity::StartProcess(DBUpdaterUtil::GetCorrectedMySQLExecutable(), std::move(args), "sql.updates", "", true); if (ret != EXIT_SUCCESS)