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

[Ashod Nakashian]

 loolwsd/DocumentBroker.cpp |   13 ++++++++-----
 loolwsd/DocumentBroker.hpp |   15 ++++++++-------
 loolwsd/LOOLWSD.cpp        |   24 ++++++++++++------------
 3 files changed, 28 insertions(+), 24 deletions(-)

New commits:
commit 60531e7def7ed8be8caddf823449cc3ff53da20a
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sun Apr 10 13:03:57 2016 -0400

    loolwsd: removed redundant session count in DocumentBroker
    
    The sessions container already has the number of sessions.
    No need for separate counters to track them.
    
    Change-Id: I838865e2b8a843e87e81a6cc1226bcacd774b032
    Reviewed-on: https://gerrit.libreoffice.org/23964
    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 3df2254..4551270 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -71,8 +71,7 @@ DocumentBroker::DocumentBroker(const Poco::URI& uriPublic,
     _childRoot(childRoot),
     _cacheRoot(getCachePath(uriPublic.toString())),
     _lastSaveTime(std::chrono::steady_clock::now()),
-    _childProcess(childProcess),
-    _sessionsCount(0)
+    _childProcess(childProcess)
 {
     assert(!_docKey.empty());
     assert(!_childRoot.empty());
@@ -125,22 +124,25 @@ bool DocumentBroker::load(const std::string& jailId)
 
         return true;
     }
-    else
-        return false;
+
+    return false;
 }
 
 bool DocumentBroker::save()
 {
-    Log::debug("Saving to URI: " + _uriPublic.toString());
+    const auto uri = _uriPublic.toString();
+    Log::debug("Saving to URI [" + uri + "].");
 
     assert(_storage && _tileCache);
     if (_storage->saveLocalFileToStorage())
     {
         _lastSaveTime = std::chrono::steady_clock::now();
         _tileCache->documentSaved();
+        Log::debug("Saved to URI [" + uri + "] and updated tile cache.");
         return true;
     }
 
+    Log::error("Failed to save to URI [" + uri + "].");
     return false;
 }
 
@@ -221,6 +223,7 @@ void DocumentBroker::takeEditLock(const std::string id)
 void DocumentBroker::addWSSession(const std::string id, std::shared_ptr<MasterProcessSession>& ws)
 {
     std::lock_guard<std::mutex> sessionsLock(_wsSessionsMutex);
+
     auto ret = _wsSessions.emplace(id, ws);
     if (!ret.second)
     {
diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp
index 259572f..17a1cfe 100644
--- a/loolwsd/DocumentBroker.hpp
+++ b/loolwsd/DocumentBroker.hpp
@@ -120,8 +120,8 @@ public:
     ~DocumentBroker()
     {
         Log::info() << "~DocumentBroker [" << _uriPublic.toString()
-                    << "] destroyed with " << _sessionsCount
-                    << " sessions." << Log::end;
+                    << "] destroyed with " << getSessionsCount()
+                    << " sessions left." << Log::end;
     }
 
     void validate(const Poco::URI& uri);
@@ -137,10 +137,12 @@ public:
     Poco::URI getJailedUri() const { return _uriJailed; }
     const std::string& getJailId() const { return _jailId; }
     const std::string& getDocKey() const { return _docKey; }
-    unsigned decSessions() { return --_sessionsCount; }
-    unsigned incSessions() { return ++_sessionsCount; }
-    unsigned getSessionsCount() { return _sessionsCount; }
     TileCache& tileCache() { return *_tileCache; }
+    unsigned getSessionsCount() const
+    {
+        std::lock_guard<std::mutex> sessionsLock(_wsSessionsMutex);
+        return _wsSessions.size();
+    }
 
     /// Returns the time in milliseconds since last save.
     double getTimeSinceLastSaveMs() const
@@ -171,12 +173,11 @@ private:
     std::string _filename;
     std::chrono::steady_clock::time_point _lastSaveTime;
     std::map<std::string, std::shared_ptr<MasterProcessSession>> _wsSessions;
-    std::mutex _wsSessionsMutex;
+    mutable std::mutex _wsSessionsMutex;
     std::unique_ptr<StorageBase> _storage;
     std::unique_ptr<TileCache> _tileCache;
     std::shared_ptr<ChildProcess> _childProcess;
     std::mutex _mutex;
-    std::atomic<unsigned> _sessionsCount;
 
     static constexpr auto IdleSaveDurationMs = 30 * 1000;
     static constexpr auto AutoSaveDurationMs = 300 * 1000;
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 0ff7e5d..823775d 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -341,10 +341,9 @@ private:
                     std::shared_ptr<WebSocket> ws;
                     auto session = std::make_shared<MasterProcessSession>(id, LOOLSession::Kind::ToClient, ws, docBroker, nullptr);
                     docBroker->addWSSession(id, session);
-                    unsigned wsSessionsCount = docBroker->getWSSessionsCount();
+                    auto wsSessionsCount = docBroker->getWSSessionsCount();
                     Log::trace(docKey + ", ws_sessions++: " + std::to_string(wsSessionsCount));
                     session->setEditLock(true);
-                    docBroker->incSessions();
                     lock.unlock();
 
                     if (!waitBridgeCompleted(session, docBroker))
@@ -384,7 +383,9 @@ private:
                     }
 
                     lock.lock();
-                    if (docBroker->decSessions() == 0)
+                    docBroker->removeWSSession(id);
+                    wsSessionsCount = docBroker->getWSSessionsCount();
+                    if (wsSessionsCount == 0)
                     {
                         Log::debug("Removing DocumentBroker for docKey [" + docKey + "].");
                         docBrokers.erase(docKey);
@@ -550,14 +551,14 @@ private:
 
         auto ws = std::make_shared<WebSocket>(request, response);
         auto session = std::make_shared<MasterProcessSession>(id, LOOLSession::Kind::ToClient, ws, docBroker, queue);
-        docBroker->incSessions();
-        docBrokersLock.unlock();
-
         docBroker->addWSSession(id, session);
-        unsigned wsSessionsCount = docBroker->getWSSessionsCount();
+        auto wsSessionsCount = docBroker->getWSSessionsCount();
         Log::trace(docKey + ", ws_sessions++: " + std::to_string(wsSessionsCount));
         if (wsSessionsCount == 1)
+        {
             session->setEditLock(true);
+        }
+        docBrokersLock.unlock();
 
         if (!waitBridgeCompleted(session, docBroker))
         {
@@ -607,16 +608,15 @@ private:
             queue->clear();
         }
 
-        docBroker->removeWSSession(id);
-        wsSessionsCount = docBroker->getWSSessionsCount();
-        Log::trace(docKey + ", ws_sessions--: " + std::to_string(wsSessionsCount));
-
         Log::info("Finishing GET request handler for session [" + id + "]. Joining the queue.");
         queue->put("eof");
         queueHandlerThread.join();
 
         docBrokersLock.lock();
-        if (docBroker->decSessions() == 0)
+        docBroker->removeWSSession(id);
+        wsSessionsCount = docBroker->getWSSessionsCount();
+        Log::trace(docKey + ", ws_sessions--: " + std::to_string(wsSessionsCount));
+        if (wsSessionsCount == 0)
         {
             Log::debug("Removing DocumentBroker for docKey [" + docKey + "].");
             docBrokers.erase(docKey);