System - Better UNIX Process Implementation

CHANGELOG.md

@@ -7,7 +7,7 @@ None
 None
 ### Fixes
 #### System
-- Fixed reading console output from `Nickvision::System::Process` on linux
+- Improved `Nickvision::System::Process` implementation on UNIX systems
 
 ## 2024.8.1
 ### Breaking Changes

CMakeLists.txt

@@ -152,6 +152,7 @@ if (BUILD_TESTING)
         "tests/networktests.cpp"
         "tests/notificationtests.cpp"
         "tests/passwordtests.cpp"
+        "tests/processtests.cpp"
         "tests/storetests.cpp"
         "tests/stringtests.cpp"
         "tests/systemcredentialstests.cpp"

include/system/process.h

@@ -125,8 +125,8 @@ namespace Nickvision::System
         HANDLE m_write;
         PROCESS_INFORMATION m_pi;
 #else
+        int m_pipe[2];
         pid_t m_pid;
-        std::filesystem::path m_consoleFilePath;
 #endif
     };
 }

manual/README.md

@@ -13,7 +13,7 @@ None
 None
 ### Fixes
 #### System
-- Fixed reading console output from `Nickvision::System::Process` on linux
+- Improved `Nickvision::System::Process` implementation on UNIX systems
 
 ## Dependencies
 The following are a list of dependencies used by libnick. 

src/system/process.cpp

@@ -6,17 +6,15 @@
 #include <iostream>
 #include <sstream>
 #include <stdexcept>
-#include "filesystem/userdirectories.h"
 #include "helpers/codehelpers.h"
 #include "helpers/stringhelpers.h"
 #ifndef _WIN32
-#include <fcntl.h>
 #include <unistd.h>
-#include <signal.h>
 #include <sys/wait.h>
 #endif
 
-using namespace Nickvision::Filesystem;
+#define PROCESS_WAIT_TIMEOUT 50
+
 using namespace Nickvision::Events;
 using namespace Nickvision::Helpers;
 
@@ -33,8 +31,7 @@ namespace Nickvision::System
         m_write{ nullptr },
         m_pi{ 0 }
 #else
-        m_pid{ -1 },
-        m_consoleFilePath{ UserDirectories::get(UserDirectory::Cache) / (StringHelpers::newUuid() + ".lnproc") }
+        m_pid{ -1 }
 #endif
     {
 #ifdef _WIN32
@@ -75,43 +72,11 @@ namespace Nickvision::System
             throw std::runtime_error("Failed to create process.");
         }
 #else
-        //Fork
-        int fd{ open(m_consoleFilePath.string().c_str(), O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH) };
-        if(fd < 0)
-        {
-            std::cerr << CodeHelpers::getLastSystemError() << std::endl;
-            throw std::runtime_error("Failed to create file.");
-        }
-        if((m_pid = fork()) < 0)
+        if(pipe(m_pipe) < 0)
         {
             std::cerr << CodeHelpers::getLastSystemError() << std::endl;
-            throw std::runtime_error("Failed to create fork.");
-        }
-        else if(m_pid == 0) //child
-        {
-            //Create process arguments
-            std::string filename{ m_path.filename().string() };
-            std::vector<char*> appArgs;
-            appArgs.push_back(filename.data());
-            for(std::string& arg : m_args)
-            {
-                appArgs.push_back(arg.data());
-            }
-            appArgs.push_back(nullptr);
-            //Redirect console output
-            dup2(fd, STDERR_FILENO);
-            dup2(fd, STDOUT_FILENO);
-            //Create process
-            raise(SIGSTOP);
-            if(execvp(m_path.string().c_str(), appArgs.data()) < 0)
-            {
-                std::cerr << CodeHelpers::getLastSystemError() << std::endl;
-                m_completed = true;
-                m_exitCode = 1;
-                exit(1);
-            }
+            throw std::runtime_error("Failed to create pipe.");
         }
-        close(fd);
 #endif
     }
 
@@ -125,9 +90,10 @@ namespace Nickvision::System
         CloseHandle(m_read);
         CloseHandle(m_write);
         CloseHandle(m_pi.hProcess);
-        CloseHandle(m_pi.hThread);
+        CloseHandle(m_pi.hThread); 
 #else
-        std::filesystem::remove(m_consoleFilePath);
+        close(m_pipe[0]);
+        close(m_pipe[1]);
 #endif
     }
 
@@ -180,14 +146,45 @@ namespace Nickvision::System
         }
 #ifdef _WIN32
         if(!ResumeThread(m_pi.hThread))
+        {
+            std::cerr << CodeHelpers::getLastSystemError() << std::endl;
+            return false;
+        }
 #else
-        waitpid(m_pid, nullptr, WUNTRACED);
-        if(::kill(m_pid, SIGCONT) < 0)
-#endif
+        if((m_pid = fork()) < 0)
         {
             std::cerr << CodeHelpers::getLastSystemError() << std::endl;
             return false;
         }
+        //child
+        else if(m_pid == 0)
+        {
+            //Create process arguments
+            std::string filename{ m_path.filename().string() };
+            std::vector<char*> appArgs;
+            appArgs.push_back(filename.data());
+            for(std::string& arg : m_args)
+            {
+                appArgs.push_back(arg.data());
+            }
+            appArgs.push_back(nullptr);
+            //Redirect console output
+            close(m_pipe[0]);
+            dup2(m_pipe[1], STDERR_FILENO);
+            dup2(m_pipe[1], STDOUT_FILENO);
+            close(m_pipe[1]);
+            //Create process
+            if(execvp(m_path.string().c_str(), appArgs.data()) < 0)
+            {
+                std::cerr << CodeHelpers::getLastSystemError() << std::endl;
+                m_completed = true;
+                m_exitCode = 1;
+                exit(1);
+            }
+        }
+        //parent
+        close(m_pipe[1]);
+#endif
         m_watchThread = std::thread(&Process::watch, this);
         m_running = true;
         return true;
@@ -203,13 +200,13 @@ namespace Nickvision::System
         //Kill child processes spawned by the process
 #ifdef _WIN32
         if(!GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, m_pi.dwProcessId))
-#else
-        if(::kill(-m_pid, SIGTERM) < 0)
-#endif
         {
             std::cerr << CodeHelpers::getLastSystemError() << std::endl;
             return false;
         }
+#else
+        ::kill(-m_pid, SIGTERM);
+#endif
         //Kill process
 #ifdef _WIN32
         if(!TerminateProcess(m_pi.hProcess, 0))
@@ -234,7 +231,7 @@ namespace Nickvision::System
         }
         while(!hasCompleted())
         {
-            std::this_thread::sleep_for(std::chrono::milliseconds(100));
+            std::this_thread::sleep_for(std::chrono::milliseconds(PROCESS_WAIT_TIMEOUT));
         }
         return getExitCode();
     }
@@ -249,7 +246,7 @@ namespace Nickvision::System
         {
 #ifdef _WIN32
             //Determine if ended
-            ended = WaitForSingleObject(m_pi.hProcess, 50) == WAIT_OBJECT_0;
+            ended = WaitForSingleObject(m_pi.hProcess, PROCESS_WAIT_TIMEOUT) == WAIT_OBJECT_0;
             //Read console output
             while(true)
             {
@@ -274,31 +271,34 @@ namespace Nickvision::System
             }
 #else
             //Determine if ended
-            while(waitpid(m_pid, &status, WNOHANG | WUNTRACED | WCONTINUED) > 0)
+            while(waitpid(m_pid, &status, WNOHANG | WUNTRACED) > 0)
             {
                 if(WIFEXITED(status) || WIFSIGNALED(status))
                 {
                     ended = true;
+                    close(m_pipe[0]);
                 }
             }
             //Read console output
-            std::unique_lock<std::mutex> lock{ m_mutex };
-            std::ifstream file{ m_consoleFilePath };
-            std::stringstream buffer;
-            buffer << file.rdbuf();
-            m_output = buffer.str();
-            lock.unlock();
-            std::this_thread::sleep_for(std::chrono::milliseconds(2));
+            char buffer[1024];
+            ssize_t bytes{ 0 };
+            while((bytes = read(m_pipe[0], buffer, sizeof(buffer) - 1)) > 0)
+            {
+                buffer[bytes] = '\0';
+                std::lock_guard<std::mutex> lock{ m_mutex };
+                m_output += buffer;
+            }
+            std::this_thread::sleep_for(std::chrono::milliseconds(PROCESS_WAIT_TIMEOUT));
 #endif
         }
+        std::unique_lock<std::mutex> lock{ m_mutex };
 #ifdef _WIN32
         DWORD exitCode{ 0 };
         GetExitCodeProcess(m_pi.hProcess, &exitCode);
+        m_exitCode = static_cast<int>(exitCode);
 #else
-        int exitCode{ WIFEXITED(status) ? WEXITSTATUS(status) : -1 };
+        m_exitCode = WIFEXITED(status) ? WEXITSTATUS(status) : -1;
 #endif
-        std::unique_lock<std::mutex> lock{ m_mutex };
-        m_exitCode = static_cast<int>(exitCode);
         m_running = false;
         m_completed = true;
         lock.unlock();

tests/processtests.cpp

@@ -0,0 +1,50 @@
+#include <gtest/gtest.h>
+#include <memory>
+#include "system/environment.h"
+#include "system/process.h"
+
+using namespace Nickvision::System;
+
+class ProcessTest : public ::testing::Test
+{
+public:
+    static std::unique_ptr<Process> m_proc;
+};
+
+std::unique_ptr<Process> ProcessTest::m_proc{ nullptr };
+
+TEST_F(ProcessTest, Create)
+{
+#ifdef _WIN32
+    ASSERT_NO_THROW(m_proc = std::make_unique<Process>(Environment::findDependency("cmd.exe"), std::vector<std::string>{ "/c", "timeout", "60" }));
+#else
+    ASSERT_NO_THROW(m_proc = std::make_unique<Process>(Environment::findDependency("sleep"), std::vector<std::string>{ "60" }));
+#endif
+    ASSERT_FALSE(m_proc->isRunning());
+    ASSERT_FALSE(m_proc->hasCompleted());
+}
+
+TEST_F(ProcessTest, Start)
+{
+    ASSERT_TRUE(m_proc->start());
+    ASSERT_TRUE(m_proc->isRunning());
+    ASSERT_FALSE(m_proc->hasCompleted());
+}
+
+TEST_F(ProcessTest, Kill)
+{
+    ASSERT_TRUE(m_proc->kill());
+    ASSERT_FALSE(m_proc->isRunning());
+    ASSERT_TRUE(m_proc->hasCompleted());
+    ASSERT_EQ(m_proc->getExitCode(), -1);
+}
+
+TEST_F(ProcessTest, Wait)
+{
+    ASSERT_EQ(m_proc->waitForExit(), -1);
+}
+
+TEST_F(ProcessTest, Destroy)
+{
+    ASSERT_NO_THROW(m_proc.reset());
+}