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

[Ashod Nakashian]

 loolwsd/DocumentBroker.cpp |   47 +++++++++++++++++++++------------------------
 loolwsd/DocumentBroker.hpp |    2 -
 2 files changed, 23 insertions(+), 26 deletions(-)

New commits:
commit 2507f7f89cbdc6787c6fa806b7b7fe7970dc68cd
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sun Nov 6 23:14:23 2016 -0500

    loolwsd: refactor DocumentBroker load and addSession
    
    Change-Id: I01db44561f79280f152bdf802efcbc064b22ab89
    Reviewed-on: https://gerrit.libreoffice.org/30646
    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 50208ba..723b73b 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -165,8 +165,12 @@ DocumentBroker::~DocumentBroker()
     _childProcess.reset();
 }
 
-bool DocumentBroker::load(const std::string& sessionId, const std::string& jailId)
+bool DocumentBroker::load(std::shared_ptr<ClientSession>& session, const std::string& jailId)
 {
+    Util::assertIsLocked(_mutex);
+
+    const std::string sessionId = session->getId();
+
     LOG_INF("Loading [" << _docKey << "] for session [" << sessionId << "] and jail [" << jailId << "].");
 
     {
@@ -182,14 +186,6 @@ bool DocumentBroker::load(const std::string& sessionId, const std::string& jailI
         return false;
     }
 
-    auto it = _sessions.find(sessionId);
-    if (it == _sessions.end() || !it->second)
-    {
-        LOG_ERR("Session with sessionId [" << sessionId << "] not found while loading");
-        return false;
-    }
-
-    auto session = it->second;
     const Poco::URI& uriPublic = session->getPublicUri();
     LOG_DBG("Loading from URI: " << uriPublic.toString());
 
@@ -466,21 +462,13 @@ size_t DocumentBroker::addSession(std::shared_ptr<ClientSession>& session)
     const auto id = session->getId();
     const std::string aMessage = "session " + id + " " + _docKey;
 
-    std::lock_guard<std::mutex> lock(_mutex);
-
-    auto ret = _sessions.emplace(id, session);
-    if (!ret.second)
-    {
-        LOG_WRN("DocumentBroker: Trying to add already existing session.");
-    }
+    std::unique_lock<std::mutex> lock(_mutex);
 
     try
     {
         // First load the document, since this can fail.
-        if (!load(id, std::to_string(_childProcess->getPid())))
+        if (!load(session, std::to_string(_childProcess->getPid())))
         {
-            _sessions.erase(id);
-
             const auto msg = "Failed to load document with URI [" + session->getPublicUri().toString() + "].";
             LOG_ERR(msg);
             throw std::runtime_error(msg);
@@ -489,7 +477,6 @@ size_t DocumentBroker::addSession(std::shared_ptr<ClientSession>& session)
     catch (const StorageSpaceLowException&)
     {
         LOG_ERR("Out of storage while loading document with URI [" << session->getPublicUri().toString() << "].");
-        _sessions.erase(id);
 
         // We use the same message as is sent when some of lool's own locations are full,
         // even if in this case it might be a totally different location (file system, or
@@ -504,14 +491,23 @@ size_t DocumentBroker::addSession(std::shared_ptr<ClientSession>& session)
     _lastEditableSession = false;
     _markToDestroy = false;
 
-    // Request a new session from the child kit.
-    _childProcess->sendTextFrame(aMessage);
-
     if (session->isReadOnly())
     {
         LOG_DBG("Adding a readonly session [" << id << "]");
     }
 
+    if (!_sessions.emplace(id, session).second)
+    {
+        LOG_WRN("DocumentBroker: Trying to add already existing session.");
+    }
+
+    const auto count = _sessions.size();
+
+    lock.unlock();
+
+    // Request a new session from the child kit.
+    _childProcess->sendTextFrame(aMessage);
+
     // Tell the admin console about this new doc
     Admin::instance().addDoc(_docKey, getPid(), getFilename(), id);
 
@@ -521,7 +517,7 @@ size_t DocumentBroker::addSession(std::shared_ptr<ClientSession>& session)
     session->setPeer(prisonerSession);
     prisonerSession->setPeer(session);
 
-    return _sessions.size();
+    return count;
 }
 
 size_t DocumentBroker::removeSession(const std::string& id)
@@ -550,9 +546,10 @@ void DocumentBroker::alertAllUsersOfDocument(const std::string& cmd, const std::
 
     std::stringstream ss;
     ss << "error: cmd=" << cmd << " kind=" << kind;
+    const auto msg = ss.str();
     for (auto& it : _sessions)
     {
-        it.second->sendTextFrame(ss.str());
+        it.second->sendTextFrame(msg);
     }
 }
 
diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp
index 60a7099..34195c9 100644
--- a/loolwsd/DocumentBroker.hpp
+++ b/loolwsd/DocumentBroker.hpp
@@ -198,7 +198,7 @@ public:
     ~DocumentBroker();
 
     /// Loads a document from the public URI into the jail.
-    bool load(const std::string& sessionId, const std::string& jailId);
+    bool load(std::shared_ptr<ClientSession>& session, const std::string& jailId);
     bool isLoaded() const { return _isLoaded; }
     void setLoaded() { _isLoaded = true; }