From 0425866486a4652f999d71c359d3645f3aad4ce9 Mon Sep 17 00:00:00 2001 From: Mike Gelfand Date: Sat, 9 Jan 2016 02:16:55 +0300 Subject: [PATCH] Allow for process termination when polling with isRunning On *NIX, one needs to call `waitpid()` in order for process to exit the zombie state. If one uses `Process::isRunning()` to emulate non-blocking wait for child process termination, process will stay zombie and function will always return true. This commit changes `Process::isRunning()` to call `waitpid()` with `WNOHANG` instead of using `kill()` when checking for child process (i.e. the one we have ProcessHandle for), which allows for process termination. Additional trickery with mutex and event is needed to prevent exceptions when `Process::isRunning()` and/or `Process::wait()` is called concurrently on the same handle from different threads. Fixes #1097. --- Foundation/include/Poco/Process_UNIX.h | 9 +++- Foundation/src/Process_UNIX.cpp | 60 ++++++++++++++++++++---- Foundation/testsuite/src/ProcessTest.cpp | 23 +++++++++ Foundation/testsuite/src/ProcessTest.h | 1 + 4 files changed, 82 insertions(+), 11 deletions(-) diff --git a/Foundation/include/Poco/Process_UNIX.h b/Foundation/include/Poco/Process_UNIX.h index 76d17c8a7..9678abd85 100644 --- a/Foundation/include/Poco/Process_UNIX.h +++ b/Foundation/include/Poco/Process_UNIX.h @@ -21,6 +21,9 @@ #include "Poco/Foundation.h" +#include "Poco/Event.h" +#include "Poco/Mutex.h" +#include "Poco/Optional.h" #include "Poco/RefCountedObject.h" #include #include @@ -41,9 +44,13 @@ public: pid_t id() const; int wait() const; + int wait(int options) const; private: - pid_t _pid; + const pid_t _pid; + mutable FastMutex _mutex; + mutable Event _event; + mutable Optional _status; }; diff --git a/Foundation/src/Process_UNIX.cpp b/Foundation/src/Process_UNIX.cpp index 73c1dafc4..1640626ab 100644 --- a/Foundation/src/Process_UNIX.cpp +++ b/Foundation/src/Process_UNIX.cpp @@ -18,6 +18,7 @@ #include "Poco/Exception.h" #include "Poco/NumberFormatter.h" #include "Poco/Pipe.h" +#include "Poco/Thread.h" #include #include #include @@ -42,7 +43,10 @@ namespace Poco { // ProcessHandleImpl // ProcessHandleImpl::ProcessHandleImpl(pid_t pid): - _pid(pid) + _pid(pid), + _mutex(), + _event(Event::EVENT_MANUALRESET), + _status() { } @@ -60,24 +64,60 @@ pid_t ProcessHandleImpl::id() const int ProcessHandleImpl::wait() const { - int status; - int rc; - do - { - rc = waitpid(_pid, &status, 0); - } - while (rc < 0 && errno == EINTR); - if (rc != _pid) + if (wait(0) != _pid) throw SystemException("Cannot wait for process", NumberFormatter::format(_pid)); + + const int status = _status.value(); if (WIFEXITED(status)) return WEXITSTATUS(status); if (WIFSIGNALED(status)) return -WTERMSIG(status); + // This line should never be reached. return std::numeric_limits::max(); } +int ProcessHandleImpl::wait(int options) const +{ + { + FastMutex::ScopedLock lock(_mutex); + if (_status.isSpecified()) + { + return _pid; + } + } + + int status; + int rc; + do + { + rc = waitpid(_pid, &status, options); + } + while (rc < 0 && errno == EINTR); + + if (rc == _pid) + { + FastMutex::ScopedLock lock(_mutex); + _status = status; + _event.set(); + } + else if (rc < 0 && errno == ECHILD) + { + // Looks like another thread was lucky and it should update the status for us shortly + _event.wait(); + + FastMutex::ScopedLock lock(_mutex); + if (_status.isSpecified()) + { + rc = _pid; + } + } + + return rc; +} + + // // ProcessImpl // @@ -251,7 +291,7 @@ void ProcessImpl::killImpl(PIDImpl pid) bool ProcessImpl::isRunningImpl(const ProcessHandleImpl& handle) { - return isRunningImpl(handle.id()); + return handle.wait(WNOHANG) == 0; } diff --git a/Foundation/testsuite/src/ProcessTest.cpp b/Foundation/testsuite/src/ProcessTest.cpp index 71d6eabb3..1226f4a66 100644 --- a/Foundation/testsuite/src/ProcessTest.cpp +++ b/Foundation/testsuite/src/ProcessTest.cpp @@ -16,6 +16,7 @@ #include "Poco/Process.h" #include "Poco/Pipe.h" #include "Poco/PipeStream.h" +#include "Poco/Thread.h" #include @@ -176,6 +177,27 @@ void ProcessTest::testIsRunning() } +void ProcessTest::testIsRunningAllowsForTermination() +{ +#if !defined(_WIN32_WCE) + std::string name("TestApp"); + std::string cmd; + +#if defined(POCO_OS_FAMILY_UNIX) + cmd = "./"; + cmd += name; +#else + cmd = name; +#endif + + std::vector args; + ProcessHandle ph = Process::launch(cmd, args, 0, 0, 0); + while (Process::isRunning(ph)) + Poco::Thread::sleep(100); +#endif // !defined(_WIN32_WCE) +} + + void ProcessTest::testSignalExitCode() { #if defined(POCO_OS_FAMILY_UNIX) @@ -213,6 +235,7 @@ CppUnit::Test* ProcessTest::suite() CppUnit_addTest(pSuite, ProcessTest, testLaunchRedirectOut); CppUnit_addTest(pSuite, ProcessTest, testLaunchEnv); CppUnit_addTest(pSuite, ProcessTest, testIsRunning); + CppUnit_addTest(pSuite, ProcessTest, testIsRunningAllowsForTermination); CppUnit_addTest(pSuite, ProcessTest, testSignalExitCode); return pSuite; diff --git a/Foundation/testsuite/src/ProcessTest.h b/Foundation/testsuite/src/ProcessTest.h index e22c2b7e6..7a0e323f1 100644 --- a/Foundation/testsuite/src/ProcessTest.h +++ b/Foundation/testsuite/src/ProcessTest.h @@ -31,6 +31,7 @@ public: void testLaunchRedirectOut(); void testLaunchEnv(); void testIsRunning(); + void testIsRunningAllowsForTermination(); void testSignalExitCode(); void setUp();