[#3025] fix ProcessSpawn on BSD

- Always break after collecting exit status. Previously it broke the
  loop always on failure of waitpid which does happen after calling it
  subsequently, but there is no reason to wait until then.
- When waitpid returns -1 in sync mode, throw exception, except for
  EINTR which happens on signals (usually one time) prior to
  the child process exiting if sigaction is called without SA_RESTART
  which is the default on some systems.
- Only initialize the global IO signal set on the IO service in async
  mode. It makes no sense to do it in sync mode because there is no IO service.
- Swap pid and wpid names to conform to names in `man wait` on BSD.
- Add FAIL() on timer expiration.
- Don't call runOne() the third time in unit tests because it waits for
  the timer to expire.

diff --git a/src/lib/asiolink/process_spawn.cc b/src/lib/asiolink/process_spawn.cc
index 2dc1d3a84e1f2dea18a28420cc7f693d01d95158..36c618c328be9fdccde532109a32a0c84ade83be 100644 (file)
--- a/src/lib/asiolink/process_spawn.cc
+++ b/src/lib/asiolink/process_spawn.cc
@@ -209,7 +209,7 @@ private:
     /// for any child process
     /// @param sync whether this function is called immediately after spawning
     /// (synchronous) or not (asynchronous, default).
-    static void waitForProcess(int signum, pid_t const pid = -1, bool const sync = false);
+    static void waitForProcess(int signum, pid_t const wpid = -1, bool const sync = false);
 
     /// @brief A map holding the status codes of executed processes.
     static ProcessCollection process_collection_;
@@ -332,7 +332,9 @@ ProcessSpawnImpl::getCommandLine() const {
 pid_t
 ProcessSpawnImpl::spawn(bool dismiss) {
     lock_guard<std::mutex> lk(mutex_);
-    ProcessSpawnImpl::IOSignalSetInitializer::initIOSignalSet(io_service_);
+    if (!sync_) {
+        ProcessSpawnImpl::IOSignalSetInitializer::initIOSignalSet(io_service_);
+    }
     // Create the child
     pid_t pid = fork();
     if (pid < 0) {
@@ -413,7 +415,7 @@ ProcessSpawnImpl::allocateInternal(const std::string& src) {
 
 void
 ProcessSpawnImpl::waitForProcess(int /* signum */,
-                                 pid_t const pid /* = -1 */,
+                                 pid_t const wpid /* = -1 */,
                                  bool sync /* = false */) {
     // In synchronous mode, the lock is taken by the caller function
     // spawn(), so do not deadlock.
@@ -427,18 +429,37 @@ ProcessSpawnImpl::waitForProcess(int /* signum */,
         // When called asynchronously, the first IO context event is for
         // receiving the SIGCHLD signal which itself is blocking,
         // hence no need to block here too.
-        pid_t wpid = waitpid(pid, &status, sync ? 0 : WNOHANG);
-        if (wpid <= 0) {
-            break;
-        }
-        for (auto const& instance : process_collection_) {
-            auto const& proc = instance.second.find(wpid);
-            /// Check that the terminating process was started
-            /// by our instance of ProcessSpawn
-            if (proc != instance.second.end()) {
-                // In this order please
-                proc->second->status_ = status;
-                proc->second->running_ = false;
+        pid_t pid = waitpid(wpid, &status, sync ? 0 : WNOHANG);
+        if (pid < 0) {
+            if (!sync) {
+                break;
+            }
+            if (errno == EINTR) {
+                // On some systems that call sigaction wihout SA_RESTART by default, signals make
+                // waitpid exit with EINTR. In this situation, if sync mode is enabled, we're
+                // interested in another round of waitpid.
+                continue;
+            }
+            isc_throw(InvalidOperation, "process with pid " << wpid << " has returned " << pid
+                                                            << " from waitpid in sync mode, errno: "
+                                                            << strerror(errno));
+        } else if (pid == 0) {
+            if (!sync) {
+                break;
+            }
+        } else /* if (pid > 0) */ {
+            for (auto const& instance : process_collection_) {
+                auto const& proc = instance.second.find(pid);
+                /// Check that the terminating process was started
+                /// by our instance of ProcessSpawn
+                if (proc != instance.second.end()) {
+                    proc->second->status_ = status;
+                    proc->second->running_ = false;
+                }
+            }
+            if (sync) {
+                // Collected process status. Can exit loop.
+                break;
             }
         }
     }

diff --git a/src/lib/asiolink/tests/process_spawn_unittest.cc b/src/lib/asiolink/tests/process_spawn_unittest.cc
index 73144475826b67d08de5b574a66d581d3fa4c4db..76706543f063813d28c0e831b3d69bdf215452ee 100644 (file)
--- a/src/lib/asiolink/tests/process_spawn_unittest.cc
+++ b/src/lib/asiolink/tests/process_spawn_unittest.cc
@@ -68,7 +68,11 @@ public:
         io_signal_set_->remove(SIGCHLD);
         io_signal_set_.reset();
         // Make sure the cancel handler for the IOSignalSet is called.
-        io_service_->poll();
+        io_service_->restart();
+        try {
+            io_service_->poll();
+        } catch (...) {
+        }
     }
 
     /// @brief Method used as the IOSignalSet handler.
@@ -98,6 +102,7 @@ public:
     /// @brief Failsafe timer expiration handler.
     void testTimerHandler() {
         io_service_->stop();
+        FAIL() << "Test Time: " << test_time_ms_ << " expired";
     }
 };
 
@@ -119,8 +124,6 @@ TEST_F(ProcessSpawnTest, spawnWithArgs) {
     io_service_->runOne();
 
     // waitForProcess handler.
-    io_service_->runOne();
-
     // ProcessSpawnTest::processSignal handler.
     io_service_->runOne();
 
@@ -155,8 +158,6 @@ TEST_F(ProcessSpawnTest, spawnWithArgsAndEnvVars) {
     io_service_->runOne();
 
     // waitForProcess handler.
-    io_service_->runOne();
-
     // ProcessSpawnTest::processSignal handler.
     io_service_->runOne();
 
@@ -189,14 +190,15 @@ TEST_F(ProcessSpawnTest, spawnTwoProcesses) {
     io_service_->runOne();
 
     // waitForProcess handler.
-    io_service_->runOne();
-
     // ProcessSpawnTest::processSignal handler.
     io_service_->runOne();
 
     // Poll to be sure.
     io_service_->poll();
 
+    ASSERT_EQ(1, processed_signals_.size());
+    ASSERT_EQ(SIGCHLD, processed_signals_[0]);
+
     pid_t pid2 = 0;
     ASSERT_NO_THROW(pid2 = process.spawn());
 
@@ -207,8 +209,6 @@ TEST_F(ProcessSpawnTest, spawnTwoProcesses) {
     io_service_->runOne();
 
     // waitForProcess handler.
-    io_service_->runOne();
-
     // ProcessSpawnTest::processSignal handler.
     io_service_->runOne();
 
@@ -246,8 +246,6 @@ TEST_F(ProcessSpawnTest, spawnNoArgs) {
     io_service_->runOne();
 
     // waitForProcess handler.
-    io_service_->runOne();
-
     // ProcessSpawnTest::processSignal handler.
     io_service_->runOne();
 
@@ -269,8 +267,6 @@ TEST_F(ProcessSpawnTest, spawnNoArgs) {
     io_service_->runOne();
 
     // waitForProcess handler.
-    io_service_->runOne();
-
     // ProcessSpawnTest::processSignal handler.
     io_service_->runOne();
 
@@ -347,8 +343,6 @@ TEST_F(ProcessSpawnTest, isRunning) {
     io_service_->runOne();
 
     // waitForProcess handler.
-    io_service_->runOne();
-
     // ProcessSpawnTest::processSignal handler.
     io_service_->runOne();
 
@@ -379,8 +373,6 @@ TEST_F(ProcessSpawnTest, inheritEnv) {
     io_service_->runOne();
 
     // waitForProcess handler.
-    io_service_->runOne();
-
     // ProcessSpawnTest::processSignal handler.
     io_service_->runOne();
 
@@ -415,8 +407,6 @@ TEST_F(ProcessSpawnTest, inheritEnvWithParentVar) {
     io_service_->runOne();
 
     // waitForProcess handler.
-    io_service_->runOne();
-
     // ProcessSpawnTest::processSignal handler.
     io_service_->runOne();