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

[Ashod Nakashian]

 loolwsd/ChildProcessSession.cpp  |   15 ++++++++++++++-
 loolwsd/DocumentBroker.cpp       |   21 +++++++--------------
 loolwsd/MasterProcessSession.cpp |    8 ++++++++
 loolwsd/MasterProcessSession.hpp |    4 ++--
 4 files changed, 31 insertions(+), 17 deletions(-)

New commits:
commit e6a6b296ea354c05f4b43ffab9fce6c8d1b50739
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Tue Apr 26 21:55:18 2016 -0400

    loolwsd: avoid communicating with clients under shared lock
    
    DocumentBroker is a central document management object.
    Using it to communicate with clients leaves it open
    to the whims of slow connections. When its lock
    is help for a long time, all clients stall, giving
    users a very poor experience.
    
    The culprit in this case was takeEditLock, which
    sent to all clients the new edit lock state.
    To avoid this, DocumentBroker now sends this
    state to the children (via a loopback socket,)
    which process messages in a separate process and
    each on its own queue thread. The children then
    in turn echo this edit lock state back to the
    clients. This communication back is done on the
    prisoner socket thread, which doesn't lock or
    stall any shared objects or threads.
    
    Change-Id: I475f6b3ecac9ae2a689bd30f43d416871aa0e384
    Reviewed-on: https://gerrit.libreoffice.org/24420
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/loolwsd/ChildProcessSession.cpp b/loolwsd/ChildProcessSession.cpp
index 0def91d..71fd825 100644
--- a/loolwsd/ChildProcessSession.cpp
+++ b/loolwsd/ChildProcessSession.cpp
@@ -456,7 +456,8 @@ bool ChildProcessSession::_handleInput(const char *buffer, int length)
                tokens[0] == "resetselection" ||
                tokens[0] == "saveas" ||
                tokens[0] == "useractive" ||
-               tokens[0] == "userinactive");
+               tokens[0] == "userinactive" ||
+               tokens[0] == "editlock:");
 
         {
             std::unique_lock<std::recursive_mutex> lock(Mutex);
@@ -534,6 +535,18 @@ bool ChildProcessSession::_handleInput(const char *buffer, int length)
         {
             setIsActive(false);
         }
+        else if (tokens[0] == "editlock:")
+        {
+            // Nothing for us to do but to let the
+            // client know about the edit lock state.
+            // Yes, this is echoed back because it's better
+            // to do this on each child's queue and thread
+            // than for WSD to potentially stall while notifying
+            // each client with the edit lock state.
+            Log::trace("Echoing back [" + firstLine + "].");
+            sendTextFrame(firstLine);
+            return true;
+        }
         else
         {
             assert(false);
diff --git a/loolwsd/DocumentBroker.cpp b/loolwsd/DocumentBroker.cpp
index 62485ae..69728cb 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -269,19 +269,13 @@ std::string DocumentBroker::getJailRoot() const
 
 void DocumentBroker::takeEditLock(const std::string& id)
 {
+    Log::debug("Session " + id + " taking the editing lock.");
     std::lock_guard<std::mutex> lock(_mutex);
+
+    // Forward to all children.
     for (auto& it: _sessions)
     {
-        if (it.first != id)
-        {
-            it.second->setEditLock(false);
-            it.second->sendTextFrame("editlock: 0");
-        }
-        else
-        {
-            it.second->setEditLock(true);
-            it.second->sendTextFrame("editlock: 1");
-        }
+        it.second->setEditLock(it.first == id);
     }
 }
 
@@ -304,8 +298,8 @@ size_t DocumentBroker::addSession(std::shared_ptr<MasterProcessSession>& session
 
     if (_sessions.size() == 1)
     {
-        session->setEditLock(true);
-        session->sendTextFrame("editlock: 1");
+        Log::debug("Giving editing lock to the first session [" + id + "].");
+        _sessions.begin()->second->markEditLock(true);
     }
 
     return _sessions.size();
@@ -336,7 +330,7 @@ size_t DocumentBroker::removeSession(const std::string& id)
     if (it != _sessions.end())
     {
         const auto haveEditLock = it->second->isEditLocked();
-        it->second->setEditLock(false);
+        it->second->markEditLock(false);
         _sessions.erase(it);
 
         if (haveEditLock)
@@ -346,7 +340,6 @@ size_t DocumentBroker::removeSession(const std::string& id)
             if (it != _sessions.end())
             {
                 it->second->setEditLock(true);
-                it->second->sendTextFrame("editlock: 1");
             }
         }
     }
diff --git a/loolwsd/MasterProcessSession.cpp b/loolwsd/MasterProcessSession.cpp
index f85b4f3..12660fe 100644
--- a/loolwsd/MasterProcessSession.cpp
+++ b/loolwsd/MasterProcessSession.cpp
@@ -420,6 +420,14 @@ bool MasterProcessSession::getStatus(const char *buffer, int length)
     return true;
 }
 
+void MasterProcessSession::setEditLock(const bool value)
+{
+    // Update the sate and forward to child.
+    _bEditLock = value;
+    const auto msg = std::string("editlock: ") + (value ? "1" : "0");
+    forwardToPeer(msg.data(), msg.size());
+}
+
 bool MasterProcessSession::getCommandValues(const char *buffer, int length, StringTokenizer& tokens)
 {
     std::string command;
diff --git a/loolwsd/MasterProcessSession.hpp b/loolwsd/MasterProcessSession.hpp
index 16d9b64..c3e1662 100644
--- a/loolwsd/MasterProcessSession.hpp
+++ b/loolwsd/MasterProcessSession.hpp
@@ -47,8 +47,8 @@ class MasterProcessSession final : public LOOLSession, public std::enable_shared
 
     void setPeer(const std::shared_ptr<MasterProcessSession>& peer) { _peer = peer; }
 
-    void setEditLock(const bool value) { _bEditLock = value; }
-
+    void setEditLock(const bool value);
+    void markEditLock(const bool value) { _bEditLock = value; }
     bool isEditLocked() const { return _bEditLock; }
 
     bool shutdownPeer(Poco::UInt16 statusCode, const std::string& message);