[Libreoffice-commits] online.git: Branch 'distro/collabora/collabora-online-2-0' - wsd/LOOLWSD.cpp

[Ashod Nakashian]

 wsd/LOOLWSD.cpp |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

New commits:
commit 8d6e9828c1fb24a84152c8f256ad77a0d0521bc5
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Thu Dec 1 22:04:43 2016 -0500

    loolwsd: guarantee DocBrokersMutex locked when alerting
    
    We now guarantee that forkChildren will be invoked
    under DocBrokersMutex lock.
    
    This eliminates the case when alertAllUsersInternal
    is invoked when this mutex isn't locked.
    
    Change-Id: Ibb259bbb4f380300a90ad2fc7affe6013dd71fef
    Reviewed-on: https://gerrit.libreoffice.org/31519
    Reviewed-by: Jan Holesovsky <kendy at collabora.com>
    Tested-by: Jan Holesovsky <kendy at collabora.com>

diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 6b143a4..c0a5246 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -283,6 +283,7 @@ bool cleanupDocBrokers()
 
 static void forkChildren(const int number)
 {
+    Util::assertIsLocked(DocBrokersMutex);
     Util::assertIsLocked(NewChildrenMutex);
 
     if (number > 0)
@@ -345,12 +346,8 @@ static void preForkChildren()
 static void prespawnChildren()
 {
     // First remove dead DocBrokers, if possible.
-    std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex, std::defer_lock);
-    if (docBrokersLock.try_lock())
-    {
-        cleanupDocBrokers();
-        docBrokersLock.unlock();
-    }
+    std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
+    cleanupDocBrokers();
 
     std::unique_lock<std::mutex> lock(NewChildrenMutex, std::defer_lock);
     if (!lock.try_lock())
@@ -397,6 +394,7 @@ static size_t addNewChild(const std::shared_ptr<ChildProcess>& child)
 
 static std::shared_ptr<ChildProcess> getNewChild()
 {
+    Util::assertIsLocked(DocBrokersMutex);
     std::unique_lock<std::mutex> lock(NewChildrenMutex);
 
     namespace chrono = std::chrono;
@@ -531,6 +529,13 @@ private:
                 {
                     LOG_INF("Conversion request for URI [" << fromPath << "].");
 
+                    auto uriPublic = DocumentBroker::sanitizeURI(fromPath);
+                    const auto docKey = DocumentBroker::getDocKey(uriPublic);
+
+                    // This lock could become a bottleneck.
+                    // In that case, we can use a pool and index by publicPath.
+                    std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
+
                     // Request a kit process for this doc.
                     auto child = getNewChild();
                     if (!child)
@@ -539,16 +544,10 @@ private:
                         throw std::runtime_error("Failed to spawn lokit child.");
                     }
 
-                    auto uriPublic = DocumentBroker::sanitizeURI(fromPath);
-                    const auto docKey = DocumentBroker::getDocKey(uriPublic);
                     LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
                     auto docBroker = std::make_shared<DocumentBroker>(uriPublic, docKey, LOOLWSD::ChildRoot, child);
                     child->setDocumentBroker(docBroker);
 
-                    // This lock could become a bottleneck.
-                    // In that case, we can use a pool and index by publicPath.
-                    std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
-
                     cleanupDocBrokers();
 
                     // FIXME: What if the same document is already open? Need a fake dockey here?