[Libreoffice-commits] online.git: loolwsd/Admin.cpp loolwsd/DocumentBroker.hpp loolwsd/LOOLForKit.cpp loolwsd/Util.cpp loolwsd/Util.hpp

[Ashod Nakashian]

 loolwsd/Admin.cpp          |   11 ++++-------
 loolwsd/DocumentBroker.hpp |   17 ++++++++---------
 loolwsd/LOOLForKit.cpp     |    2 +-
 loolwsd/Util.cpp           |   29 +++++++++++++++++++++++++++++
 loolwsd/Util.hpp           |    5 +++++
 5 files changed, 47 insertions(+), 17 deletions(-)

New commits:
commit 6ad3b64d30a1398a9d355a7b9b9ab7ece1658c3f
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sun Nov 13 16:12:01 2016 -0500

    loolwsd: kill children using SIGTERM from via a helper function
    
    Change-Id: I901183fc59725681208a5c0f23f0916e158e5654
    Reviewed-on: https://gerrit.libreoffice.org/30825
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/loolwsd/Admin.cpp b/loolwsd/Admin.cpp
index e8d9d3a..9fc3535 100644
--- a/loolwsd/Admin.cpp
+++ b/loolwsd/Admin.cpp
@@ -123,15 +123,12 @@ bool AdminRequestHandler::adminCommandHandler(const std::vector<char>& payload)
         try
         {
             const auto pid = std::stoi(tokens[1]);
-            if (kill(pid, SIGINT) != 0 && kill(pid, 0) !=0)
-            {
-                LOG_SYS("Cannot terminate PID: " << tokens[0]);
-            }
+            LOG_INF("Admin request to kill PID: " << pid);
+            Util::killChild(pid);
         }
-        catch(std::invalid_argument& exc)
+        catch (std::invalid_argument& exc)
         {
-            Log::warn() << "Invalid PID to kill: " << tokens[0] << Log::end;
-            return false;
+            LOG_WRN("Invalid PID to kill: " << tokens[1]);
         }
     }
     else if (tokens[0] == "settings")
diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp
index 6906391..2184925 100644
--- a/loolwsd/DocumentBroker.hpp
+++ b/loolwsd/DocumentBroker.hpp
@@ -96,24 +96,21 @@ public:
             }
 
             _ws.reset();
-            if (_pid != -1 && kill(_pid, 0) != 0)
+            if (_pid != -1 && rude && kill(_pid, 0) != 0 && errno != ESRCH)
             {
-                if (errno != ESRCH && rude)
+                LOG_INF("Killing child [" << _pid << "].");
+                if (Util::killChild(_pid))
                 {
-                    LOG_INF("Killing child [" << _pid << "].");
-                    if (kill(_pid, SIGINT) != 0 && kill(_pid, 0) != 0 && errno != ESRCH)
-                    {
-                        LOG_SYS("Cannot terminate lokit [" << _pid << "]. Abandoning.");
-                    }
+                    LOG_ERR("Cannot terminate lokit [" << _pid << "]. Abandoning.");
                 }
             }
-
-            _pid = -1;
         }
         catch (const std::exception& ex)
         {
             LOG_ERR("Error while closing child process: " << ex.what());
         }
+
+        _pid = -1;
     }
 
     Poco::Process::PID getPid() const { return _pid; }
@@ -142,6 +139,8 @@ public:
     }
 
     /// Check whether this child is alive and able to respond.
+    /// Note: zombies will show as alive, and sockets have waiting
+    /// time after the other end-point closes. So this isn't accurate.
     bool isAlive() const
     {
         try
diff --git a/loolwsd/LOOLForKit.cpp b/loolwsd/LOOLForKit.cpp
index f234405..75156a0 100644
--- a/loolwsd/LOOLForKit.cpp
+++ b/loolwsd/LOOLForKit.cpp
@@ -77,7 +77,7 @@ public:
         }
         else if (ready < 0)
         {
-            // Termination is done via SIGINT, which breaks the wait.
+            // Termination is done via SIGTERM, which breaks the wait.
             if (!TerminationFlag)
             {
                 Log::error("Error reading from pipe [" + getName() + "].");
diff --git a/loolwsd/Util.cpp b/loolwsd/Util.cpp
index e3a2b02..b5c26a7 100644
--- a/loolwsd/Util.cpp
+++ b/loolwsd/Util.cpp
@@ -32,6 +32,7 @@
 #include <random>
 #include <sstream>
 #include <string>
+#include <thread>
 
 #include <Poco/Base64Encoder.h>
 #include <Poco/ConsoleChannel.h>
@@ -397,6 +398,34 @@ namespace Util
         static std::atomic_int counter(0);
         return std::to_string(Poco::Process::id()) + "/" + std::to_string(counter++);
     }
+
+    bool killChild(const int pid)
+    {
+        LOG_DBG("Killing PID: " << pid);
+        if (kill(pid, SIGTERM) == 0 || errno == ESRCH)
+        {
+            // Killed or doesn't exist.
+            return true;
+        }
+
+        LOG_SYS("Error when trying to kill PID: " << pid << ". Will wait for termination.");
+
+        const auto sleepMs = 50;
+        const auto count = std::max(CHILD_REBALANCE_INTERVAL_MS / sleepMs, 2);
+        for (int i = 0; i < count; ++i)
+        {
+            if (kill(pid, 0) == 0 || errno == ESRCH)
+            {
+                // Doesn't exist.
+                return true;
+            }
+
+            std::this_thread::sleep_for(std::chrono::milliseconds(sleepMs));
+        }
+
+        LOG_WRN("Cannot terminate PID: " << pid);
+        return false;
+    }
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/loolwsd/Util.hpp b/loolwsd/Util.hpp
index 7e4170f..93c95f7 100644
--- a/loolwsd/Util.hpp
+++ b/loolwsd/Util.hpp
@@ -88,6 +88,11 @@ namespace Util
 
     void requestTermination(const Poco::Process::PID& pid);
 
+    /// Kills a child process and returns true when
+    /// child pid is removed from the process table
+    /// after a certain (short) timeout.
+    bool killChild(const int pid);
+
     int getMemoryUsage(const Poco::Process::PID nPid);
 
     std::string replace(const std::string& s, const std::string& a, const std::string& b);