diff --git a/CHANGELOG.md b/CHANGELOG.md index c28d955..d232fcf 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/CMakeLists.txt b/CMakeLists.txt index 28c9f88..648d65b 100644 --- a/CMakeLists.txt +++ b/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" diff --git a/include/system/process.h b/include/system/process.h index 1b58a9c..d66f2af 100644 --- a/include/system/process.h +++ b/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 }; } diff --git a/manual/README.md b/manual/README.md index f0f0cdb..90245f5 100644 --- a/manual/README.md +++ b/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. diff --git a/src/system/process.cpp b/src/system/process.cpp index f2a695e..8717f11 100644 --- a/src/system/process.cpp +++ b/src/system/process.cpp @@ -6,17 +6,15 @@ #include #include #include -#include "filesystem/userdirectories.h" #include "helpers/codehelpers.h" #include "helpers/stringhelpers.h" #ifndef _WIN32 -#include #include -#include #include #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 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 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 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 lock{ m_mutex }; + m_output += buffer; + } + std::this_thread::sleep_for(std::chrono::milliseconds(PROCESS_WAIT_TIMEOUT)); #endif } + std::unique_lock lock{ m_mutex }; #ifdef _WIN32 DWORD exitCode{ 0 }; GetExitCodeProcess(m_pi.hProcess, &exitCode); + m_exitCode = static_cast(exitCode); #else - int exitCode{ WIFEXITED(status) ? WEXITSTATUS(status) : -1 }; + m_exitCode = WIFEXITED(status) ? WEXITSTATUS(status) : -1; #endif - std::unique_lock lock{ m_mutex }; - m_exitCode = static_cast(exitCode); m_running = false; m_completed = true; lock.unlock(); diff --git a/tests/processtests.cpp b/tests/processtests.cpp new file mode 100644 index 0000000..2dba426 --- /dev/null +++ b/tests/processtests.cpp @@ -0,0 +1,50 @@ +#include +#include +#include "system/environment.h" +#include "system/process.h" + +using namespace Nickvision::System; + +class ProcessTest : public ::testing::Test +{ +public: + static std::unique_ptr m_proc; +}; + +std::unique_ptr ProcessTest::m_proc{ nullptr }; + +TEST_F(ProcessTest, Create) +{ +#ifdef _WIN32 + ASSERT_NO_THROW(m_proc = std::make_unique(Environment::findDependency("cmd.exe"), std::vector{ "/c", "timeout", "60" })); +#else + ASSERT_NO_THROW(m_proc = std::make_unique(Environment::findDependency("sleep"), std::vector{ "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()); +} \ No newline at end of file