Core/Common: Output stdout/stderr from child process without waiting for it to finish

Shauren · Shauren · commit 25be3cdc3b2b · 2025-04-13T19:05:27.000+02:00
(cherry picked from commit f270686201c84a07f67a9615fd610d917fc8f802)
diff --git a/src/common/Utilities/StartProcess.cpp b/src/common/Utilities/StartProcess.cpp
@@ -28,117 +28,10 @@
 #include <boost/process/search_path.hpp>
 #include <fmt/ranges.h>
 
-using namespace boost::process;
+namespace bp = boost::process;
 
 namespace Trinity
 {
-template<typename T>
-static int CreateChildProcess(T waiter, std::string const& executable,
-                              std::vector<std::string> const& argsVector,
-                              std::string const& logger, std::string const& input,
-                              bool secure)
-{
-#if TRINITY_COMPILER == TRINITY_COMPILER_MICROSOFT
-#pragma warning(push)
-#pragma warning(disable:4297)
-/*
-  Silence warning with boost 1.83
-
-    boost/process/pipe.hpp(132,5): warning C4297: 'boost::process::basic_pipebuf<char,std::char_traits<char>>::~basic_pipebuf': function assumed not to throw an exception but does
-    boost/process/pipe.hpp(132,5): message : destructor or deallocator has a (possibly implicit) non-throwing exception specification
-    boost/process/pipe.hpp(124,6): message : while compiling class template member function 'boost::process::basic_pipebuf<char,std::char_traits<char>>::~basic_pipebuf(void)'
-    boost/process/pipe.hpp(304,42): message : see reference to class template instantiation 'boost::process::basic_pipebuf<char,std::char_traits<char>>' being compiled
-*/
-#endif
-    ipstream outStream;
-    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, " "));
-    }
-
-    // prepare file with only read permission (boost process opens with read_write)
-    auto inputFile = std::shared_ptr<FILE>(!input.empty() ? fopen(input.c_str(), "rb") : nullptr, [](FILE* f) { if (f) fclose(f); });
-
-    // Start the child process
-    child c = [&]()
-    {
-        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
-            };
-        }
-        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
-            };
-        }
-    }();
-
-    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);
-    }
-
-    // Call the waiter in the current scope to prevent
-    // the streams from closing too early on leaving the scope.
-    int const result = waiter(c);
-
-    if (!secure)
-    {
-        TC_LOG_TRACE(logger, ">> Process \"{}\" finished with return value {}.",
-                executable, result);
-    }
-
-    return result;
-}
-
-int StartProcess(std::string const& executable, std::vector<std::string> const& args,
-                 std::string const& logger, std::string input_file, bool secure)
-{
-    return CreateChildProcess([](child& c) -> int
-    {
-        try
-        {
-            c.wait();
-            return c.exit_code();
-        }
-        catch (...)
-        {
-            return EXIT_FAILURE;
-        }
-    }, executable, args, logger, input_file, secure);
-}
-
 class AsyncProcessResultImplementation
     : public AsyncProcessResult
 {
@@ -150,59 +43,129 @@ class AsyncProcessResultImplementation
 
     std::atomic<bool> was_terminated;
 
-    // Workaround for missing move support in boost < 1.57
-    Optional<std::shared_ptr<std::future<int>>> result;
-    Optional<std::reference_wrapper<child>> my_child;
+    Optional<std::future<int>> futureResult;
+    Optional<bp::child> my_child;
 
 public:
     explicit AsyncProcessResultImplementation(std::string executable_, std::vector<std::string> 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_),
+          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!");
 
-        return CreateChildProcess([&](child& c) -> int
+#if TRINITY_COMPILER == TRINITY_COMPILER_MICROSOFT
+#pragma warning(push)
+#pragma warning(disable:4297)
+/*
+  Silence warning with boost 1.83
+
+    boost/process/pipe.hpp(132,5): warning C4297: 'boost::process::basic_pipebuf<char,std::char_traits<char>>::~basic_pipebuf': function assumed not to throw an exception but does
+    boost/process/pipe.hpp(132,5): message : destructor or deallocator has a (possibly implicit) non-throwing exception specification
+    boost/process/pipe.hpp(124,6): message : while compiling class template member function 'boost::process::basic_pipebuf<char,std::char_traits<char>>::~basic_pipebuf(void)'
+    boost/process/pipe.hpp(304,42): message : see reference to class template instantiation 'boost::process::basic_pipebuf<char,std::char_traits<char>>' being compiled
+*/
+#endif
+        bp::ipstream outStream;
+        bp::ipstream errStream;
+#if TRINITY_COMPILER == TRINITY_COMPILER_MICROSOFT
+#pragma warning(pop)
+#endif
+
+        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 = std::shared_ptr<FILE>(!input_file.empty() ? fopen(input_file.c_str(), "rb") : nullptr, [](FILE* f) { if (f) fclose(f); });
+
+        // Start the child process
+        if (inputFile)
+        {
+            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
         {
-            int result;
-            my_child = std::reference_wrapper<child>(c);
+            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
+            );
+        }
 
-            try
+        std::future<void> stdOutReader = std::async(std::launch::async, [&]
+        {
+            std::string line;
+            while (std::getline(outStream, line, '\n'))
             {
-                c.wait();
-                result = c.exit_code();
+                std::erase(line, '\r');
+                if (!line.empty())
+                    TC_LOG_INFO(logger, "{}", line);
             }
-            catch (...)
+        });
+
+        std::future<void> stdErrReader = std::async(std::launch::async, [&]
+        {
+            std::string line;
+            while (std::getline(errStream, line, '\n'))
             {
-                result = EXIT_FAILURE;
+                std::erase(line, '\r');
+                if (!line.empty())
+                    TC_LOG_ERROR(logger, "{}", line);
             }
+        });
 
-            my_child.reset();
-            return was_terminated ? EXIT_FAILURE : result;
+        std::error_code ec;
+        my_child->wait(ec);
+        int32 const result = !ec && !was_terminated ? my_child->exit_code() : EXIT_FAILURE;
+        my_child.reset();
 
-        }, executable, args, logger, input_file, is_secure);
+        stdOutReader.wait();
+        stdErrReader.wait();
+
+        if (!is_secure)
+        {
+            TC_LOG_TRACE(logger, ">> Process \"{}\" finished with return value {}.",
+                executable, result);
+        }
+
+        return result;
     }
 
-    void SetFuture(std::future<int> result_)
+    void SetFuture(std::future<int32> result_)
     {
-        result = std::make_shared<std::future<int>>(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<int>& GetFutureResult() override
+    std::future<int32>& 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
@@ -211,23 +174,25 @@ class AsyncProcessResultImplementation
         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<AsyncProcessResult>
-    StartAsyncProcess(std::string executable, std::vector<std::string> args,
-                      std::string logger, std::string input_file, bool secure)
+int StartProcess(std::string executable, std::vector<std::string> args,
+    std::string logger, std::string input_file, bool secure)
+{
+    AsyncProcessResultImplementation handle(
+        std::move(executable), std::move(args), std::move(logger), std::move(input_file), secure);
+
+    return handle.StartProcess();
+}
+
+std::shared_ptr<AsyncProcessResult> StartAsyncProcess(std::string executable, std::vector<std::string> args,
+    std::string logger, std::string input_file, bool secure)
 {
-    auto handle = std::make_shared<AsyncProcessResultImplementation>(
+    std::shared_ptr<AsyncProcessResultImplementation> handle = std::make_shared<AsyncProcessResultImplementation>(
         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(); }));
@@ -238,7 +203,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
@@ -15,8 +15,8 @@
  * with this program. If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef Process_h__
-#define Process_h__
+#ifndef TRINITYCORE_START_PROCESS_H
+#define TRINITYCORE_START_PROCESS_H
 
 #include "Define.h"
 #include <future>
@@ -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<std::string> const& args,
-                               std::string const& logger, std::string input_file = "",
+TC_COMMON_API int32 StartProcess(std::string executable, std::vector<std::string> args,
+                               std::string logger, std::string input_file = "",
                                bool secure = false);
 
 /// Platform and library independent representation
@@ -45,7 +45,7 @@ class AsyncProcessResult
 
     /// Returns the future which contains the result of the process
     /// as soon it is finished.
-    virtual std::future<int>& GetFutureResult() = 0;
+    virtual std::future<int32>& 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
@@ -391,7 +391,7 @@ void DBUpdater<T>::ApplyFile(DatabaseWorkerPool<T>& 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)
