[Libreoffice-commits] online.git: loolwsd/DocumentBroker.cpp loolwsd/DocumentBroker.hpp loolwsd/LOOLWSD.cpp

[Ashod Nakashian]

 loolwsd/DocumentBroker.cpp |   23 +++++++++++++++++++++++
 loolwsd/DocumentBroker.hpp |    7 +++++++
 loolwsd/LOOLWSD.cpp        |   25 ++++++++++++++++++-------
 3 files changed, 48 insertions(+), 7 deletions(-)

New commits:
commit 800e711321e19b02f5e15f496525d42edcba4376
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sat Nov 5 23:00:16 2016 -0400

    loolwsd: proper ChildProcess cleanup
    
    Change-Id: If9be827aa50471b7d1d922402414d028ccdcff8b
    Reviewed-on: https://gerrit.libreoffice.org/30629
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/loolwsd/DocumentBroker.cpp b/loolwsd/DocumentBroker.cpp
index 19dbb97..507362c 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -180,6 +180,10 @@ DocumentBroker::~DocumentBroker()
     {
         LOG_WRN("DocumentBroker still has unremoved sessions.");
     }
+
+    // Need to first make sure the child exited, socket closed,
+    // and thread finished before we are destroyed.
+    _childProcess.reset();
 }
 
 bool DocumentBroker::load(const std::string& sessionId, const std::string& jailId)
@@ -922,4 +926,23 @@ void DocumentBroker::childSocketTerminated()
     }
 }
 
+void DocumentBroker::terminateChild(std::unique_lock<std::mutex>& lock)
+{
+    Util::assertIsLocked(_mutex);
+    Util::assertIsLocked(lock);
+
+    Log::info() << "Terminating child [" << getPid() << "] of doc [" << _docKey << "]." << Log::end;
+
+    assert(_sessions.empty() && "DocumentBroker still has unremoved sessions!");
+
+    // First flag to stop as it might be waiting on our lock
+    // to process some incoming message.
+    _childProcess->stop();
+
+    // Release the lock and wait for the thread to finish.
+    lock.unlock();
+
+    _childProcess->close(false);
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp
index e58b642..0075fb6 100644
--- a/loolwsd/DocumentBroker.hpp
+++ b/loolwsd/DocumentBroker.hpp
@@ -270,6 +270,13 @@ public:
     /// or upon failing to process an incoming message.
     void childSocketTerminated();
 
+    /// This gracefully terminates the connection
+    /// with the child and cleans up ChildProcess etc.
+    /// We must be called under lock and it must be
+    /// passed to us so we unlock before waiting on
+    /// the ChildProcess thread, which can take our lock.
+    void terminateChild(std::unique_lock<std::mutex>& lock);
+
     /// Get the PID of the associated child process
     Poco::Process::PID getPid() const { return _childProcess->getPid(); }
 
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 7ec35a1..ed5810a 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -565,11 +565,9 @@ private:
                     if (sessionsCount == 0)
                     {
                         // At this point we're done.
-                        // We can't save if we hadn't, just kill.
-                        Log::debug("Closing child for docKey [" + docKey + "].");
-                        child->close(true);
-                        Log::debug("Removing DocumentBroker for docKey [" + docKey + "].");
+                        LOG_DBG("Removing DocumentBroker for docKey [" << docKey << "].");
                         DocBrokers.erase(docKey);
+                        docBroker->terminateChild(docLock);
                     }
                     else
                     {
@@ -928,9 +926,22 @@ private:
 
             if (sessionsCount == 0)
             {
-                std::unique_lock<std::mutex> DocBrokersLock(DocBrokersMutex);
-                Log::debug("Removing DocumentBroker for docKey [" + docKey + "].");
-                DocBrokers.erase(docKey);
+                // We've supposedly destroyed the last session and can do away with
+                // DocBroker. But first we need to take both locks in the correct
+                // order and check again. We can't take the DocBrokersMutex while
+                // holding the docBroker lock as that can deadlock with autoSave below.
+                std::unique_lock<std::mutex> docBrokersLock2(DocBrokersMutex);
+                it = DocBrokers.find(docKey);
+                if (it != DocBrokers.end() && it->second)
+                {
+                    auto lock = it->second->getLock();
+                    if (it->second->getSessionsCount() == 0)
+                    {
+                        Log::info("Removing DocumentBroker for docKey [" + docKey + "].");
+                        DocBrokers.erase(docKey);
+                        docBroker->terminateChild(lock);
+                    }
+                }
             }
 
             LOOLWSD::dumpEventTrace(docBroker->getJailId(), id, "EndSession: " + uri);