Merge lp:~afrantzis/unity-system-compositor/no-external-spinner-zombie-processes into lp:unity-system-compositor

[Alexandros Frantzis]

Don't leave zombie spinner processes

Don't leave zombie spinner processes

This MP uses the double-fork trick to avoid zombie spinner process. It works by forking an intermediate process which then forks again to execute the real spinner process. The intermediate process exits immediately, orphaning the spinner process, which is then adopted by init (pid 1). The init process then takes care of waiting for the process to exit.

[Daniel van Vugt (vanvugt)]

Haven't looked at the branch in detail, but I'm surprised it needs much work at all... Simply calling wait() or waitpid() at the right times will avoid zombies. That's what "zombie process" means -- that the parent has not called wait() yet.

[Alexandros Frantzis (afrantzis)]

> Simply calling wait() or waitpid() at the right times will avoid zombies.
> That's what "zombie process" means -- that the parent has not called wait() yet.

That was my initial approach, but the problem with this is that waitpid() blocks USC for a little bit in the best case, and can block forever if for whatever reason the external spinner happens to not respond to our signals (e.g. it has hung).

The proposed way is indeed more complicated, but it avoid all blocking problems by transferring the responsibility to the init process.

Note that a lot of the code in this MP is just test improvements, and signal-safe wrappers around read/write.

[Chris Halse Rogers (raof)]

On Friday, 10 July 2015 6:21:03 PM AEST, Alexandros Frantzis wrote:
>> Simply calling wait() or waitpid() at the right times will avoid zombies.
>> That's what "zombie process" means -- that the parent has not
>> called wait() yet.
>
> That was my initial approach, but the problem with this is that
> waitpid() blocks USC for a little bit in the best case, and can
> block forever if for whatever reason the external spinner
> happens to not respond to our signals (e.g. it has hung).

The canonical way of handling this is a SIGCHILD handler; that way you only
ever call non-blocking waitpid.

That said, double-forking should work, too.

--
Sent using Dekko from my Ubuntu device

[Alexandros Frantzis (afrantzis)]

> The canonical way of handling this is a SIGCHILD handler; that way you only
> ever call non-blocking waitpid.

I considered this, but was concerned about interaction with libmirserver signal handling. If we think it is safe, though, I would rather use SIGCHILD too. I will propose an alternative MP with sigchild and we can decide.

[Alexandros Frantzis (afrantzis)]

Alternative MP using sigchld:

https://code.launchpad.net/~afrantzis/unity-system-compositor/no-external-spinner-zombie-processes-sigchld/+merge/264410

Also, I split out the test fixes:

https://code.launchpad.net/~afrantzis/unity-system-compositor/fix-1473418-external-spinner-tests/+merge/264411

Unmerged revisions

224. By Alexandros Frantzis on 2015-07-09

Don't leave zombie spinner processes

=== modified file 'src/external_spinner.cpp'
--- src/external_spinner.cpp	2015-04-01 17:06:16 +0000
+++ src/external_spinner.cpp	2015-07-09 13:12:58 +0000
@@ -20,6 +20,64 @@
 
 #include <unistd.h>
 #include <signal.h>
+#include <sys/wait.h>
+
+namespace
+{
+
+void write_pid(int fd, pid_t pid)
+{
+    auto const pid_str = std::to_string(pid);
+    size_t nwritten = 0;
+
+    while (nwritten < pid_str.size())
+    {
+        auto const n = write(fd, pid_str.data() + nwritten, pid_str.size() - nwritten);
+        if (n < 0)
+        {
+            if (errno != EINTR)
+                return;
+        }
+        else if (n == 0)
+        {
+            return;
+        }
+        else
+        {
+            nwritten += n;
+        }
+    }
+}
+
+pid_t read_pid(int fd)
+{
+    std::string pid_str;
+    bool more_data_to_read = true;
+
+    while (more_data_to_read)
+    {
+        size_t const bufsize = 256;
+        char data[bufsize];
+        auto const nread = read(fd, data, bufsize);
+        if (nread < 0)
+        {
+            if (errno != EINTR)
+                return 0;
+        }
+        else if (nread == 0)
+        {
+            more_data_to_read = false;
+        }
+        else
+        {
+            pid_str.append(data, nread);
+        }
+    }
+
+    return std::atoi(pid_str.c_str());
+}
+
+}
 
 usc::ExternalSpinner::ExternalSpinner(
     std::string const& executable,
@@ -42,16 +100,39 @@
     if (executable.empty() || spinner_pid)
         return;
 
-    auto pid = fork();
+    /* Use a double fork to avoid zombie processes */
+    int pipefd[2] {0,0};
+    if (pipe(pipefd)) {}
+
+    auto const pid = fork();
     if (!pid)
     {
-        setenv("MIR_SOCKET", mir_socket.c_str(), 1);
-        execlp(executable.c_str(), executable.c_str(), nullptr);
+        close(pipefd[0]);
+
+        auto const spid = fork();
+        if (!spid)
+        {
+            close(pipefd[1]);
+            setenv("MIR_SOCKET", mir_socket.c_str(), 1);
+            execlp(executable.c_str(), executable.c_str(), nullptr);
+        }
+        else
+        {
+            /* Send the spinner pid to grandparent (USC) */
+            write_pid(pipefd[1], spid);
+            close(pipefd[1]);
+            exit(0);
+        }
     }
     else
     {
-        spinner_pid = pid;
+        close(pipefd[1]);
+        waitpid(pid, nullptr, 0);
+        /* Get spinner pid from grandchild */
+        spinner_pid = read_pid(pipefd[0]);
     }
+
+    close(pipefd[0]);
 }
 
 void usc::ExternalSpinner::kill()

=== modified file 'tests/integration-tests/CMakeLists.txt'
--- tests/integration-tests/CMakeLists.txt	2015-04-07 13:36:48 +0000
+++ tests/integration-tests/CMakeLists.txt	2015-07-09 13:12:58 +0000
@@ -32,6 +32,7 @@
   run_command.cpp
   dbus_bus.cpp
   dbus_client.cpp
+  spin_wait.cpp
   test_dbus_event_loop.cpp
   test_unity_screen_service.cpp
   test_external_spinner.cpp

=== added file 'tests/integration-tests/spin_wait.cpp'
--- tests/integration-tests/spin_wait.cpp	1970-01-01 00:00:00 +0000
+++ tests/integration-tests/spin_wait.cpp	2015-07-09 13:12:58 +0000
@@ -0,0 +1,39 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "spin_wait.h"
+
+#include <thread>
+
+bool usc::test::spin_wait_for_condition_or_timeout(
+    std::function<bool()> const& condition,
+    std::chrono::milliseconds timeout,
+    std::chrono::milliseconds spin_period)
+{
+    auto const end = std::chrono::steady_clock::now() + timeout;
+    bool condition_fulfilled = false;
+    
+    while (std::chrono::steady_clock::now() < end &&
+           !(condition_fulfilled = condition()))
+    {
+        std::this_thread::sleep_for(spin_period);
+    }   
+
+    return condition_fulfilled;
+}
+

=== added file 'tests/integration-tests/spin_wait.h'
--- tests/integration-tests/spin_wait.h	1970-01-01 00:00:00 +0000
+++ tests/integration-tests/spin_wait.h	2015-07-09 13:12:58 +0000
@@ -0,0 +1,39 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#ifndef USC_SPIN_WAIT_H_
+#define USC_SPIN_WAIT_H_
+
+#include <functional>
+#include <chrono>
+
+namespace usc
+{
+namespace test
+{
+
+
+bool spin_wait_for_condition_or_timeout(
+    std::function<bool()> const& condition,
+    std::chrono::milliseconds timeout,
+    std::chrono::milliseconds spin_period = std::chrono::milliseconds{10});
+
+}
+}
+
+#endif

=== modified file 'tests/integration-tests/test_external_spinner.cpp'
--- tests/integration-tests/test_external_spinner.cpp	2015-04-01 17:11:57 +0000
+++ tests/integration-tests/test_external_spinner.cpp	2015-07-09 13:12:58 +0000
@@ -18,6 +18,7 @@
 
 #include "src/external_spinner.h"
 #include "run_command.h"
+#include "spin_wait.h"
 
 #include <fstream>
 #include <chrono>
@@ -74,12 +75,21 @@
 {
     std::vector<pid_t> spinner_pids()
     {
-        return pidof(spinner_cmd);
+        std::vector<pid_t> pids;
+
+        usc::test::spin_wait_for_condition_or_timeout(
+            [&pids, this] { pids = pidof(spinner_cmd); return !pids.empty(); },
+            timeout);
+
+        if (pids.empty())
+            BOOST_THROW_EXCEPTION(std::runtime_error("spinner_pids timed out"));
+
+        return pids;
     }
 
     std::vector<std::string> environment_of_spinner()
     {
-        auto const pids = pidof(spinner_cmd);
+        auto const pids = spinner_pids();
         if (pids.size() > 1)
             BOOST_THROW_EXCEPTION(std::runtime_error("Detected multiple spinner processes"));
         std::vector<std::string> env;
@@ -96,19 +106,14 @@
 
     void wait_for_spinner_to_terminate()
     {
-        auto const timeout = std::chrono::milliseconds{3000};
-        auto const expire = std::chrono::steady_clock::now() + timeout;
-
-        while (spinner_pids().size() > 0)
-        {
-            if (std::chrono::steady_clock::now() > expire)
-                BOOST_THROW_EXCEPTION(std::runtime_error("wait_for_no_spinner timed out"));
-            std::this_thread::sleep_for(std::chrono::milliseconds{10});
-        }
+        usc::test::spin_wait_for_condition_or_timeout(
+            [this] { return pidof(spinner_cmd).empty(); },
+            timeout);
     }
 
     std::string const spinner_cmd{executable_path() + "/usc_test_helper_wait_for_signal"};
     std::string const mir_socket{"usc_mir_socket"};
+    std::chrono::milliseconds const timeout{3000};
     usc::ExternalSpinner spinner{spinner_cmd, mir_socket};
 };