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

[Ashod Nakashian]

 loolwsd/ChildSession.cpp |    7 ------
 loolwsd/ChildSession.hpp |    4 ++-
 loolwsd/LOOLKit.cpp      |   52 ++++++++++++++++++-----------------------------
 3 files changed, 25 insertions(+), 38 deletions(-)

New commits:
commit a28b832309e30ea609df85582956643c1db29385
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sun Aug 21 11:12:12 2016 -0400

    loolwsd: proper ChildSession cleanup
    
    ChildSession cleanup is tricky because it needs
    to be cleaned up when the connection is dropped.
    The ChildSession itself needs to initiate this
    cleanup from Document.
    
    A new approach simplifies the design and correctly
    broadcasts remview to all other connections so
    they would be able to cleanup visual elements.
    
    Change-Id: I78fd01fb42b801913534c858324c16dd7ad6451d
    Reviewed-on: https://gerrit.libreoffice.org/28302
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/loolwsd/ChildSession.cpp b/loolwsd/ChildSession.cpp
index 51b445f..d94b68b 100644
--- a/loolwsd/ChildSession.cpp
+++ b/loolwsd/ChildSession.cpp
@@ -58,14 +58,9 @@ void ChildSession::disconnect()
     {
         std::unique_lock<std::recursive_mutex> lock(Mutex);
 
-        sendTextFrame("remview: " + std::to_string(_viewId));
-
         if (_viewId >= 0)
         {
-            if (_multiView && _loKitDocument)
-                _loKitDocument->setView(_viewId);
-
-            _docManager.onUnload(getId());
+            _docManager.onUnload(*this);
         }
         else
         {
diff --git a/loolwsd/ChildSession.hpp b/loolwsd/ChildSession.hpp
index e69c415..1f6d2eb 100644
--- a/loolwsd/ChildSession.hpp
+++ b/loolwsd/ChildSession.hpp
@@ -19,6 +19,8 @@
 #include "LOOLSession.hpp"
 #include "LibreOfficeKit.hpp"
 
+class ChildSession;
+
 /// An abstract interface that defines the
 /// DocumentManager interface and functionality.
 class IDocumentManager
@@ -35,7 +37,7 @@ public:
     /// Unload a client session, which unloads the document
     /// if it is the last and only.
     virtual
-    void onUnload(const std::string& sessionId) = 0;
+    void onUnload(const ChildSession& session) = 0;
 
     /// Get a list of all current view IDs.
     virtual
diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp
index a1df19f..897f033 100644
--- a/loolwsd/LOOLKit.cpp
+++ b/loolwsd/LOOLKit.cpp
@@ -248,6 +248,7 @@ public:
         join();
     }
 
+    const std::string& getSessionId() const { return _sessionId; };
     std::shared_ptr<WebSocket> getWebSocket() const { return _ws; }
     std::shared_ptr<ChildSession> getSession() { return _session; }
 
@@ -325,8 +326,6 @@ public:
             Log::error("Connection::run:: Unexpected exception");
         }
 
-        // Release the session and unload view.
-        _session.reset();
         Log::debug("Thread finished.");
     }
 
@@ -882,55 +881,46 @@ private:
         return _loKitDocument;
     }
 
-    void onUnload(const std::string& sessionId) override
+    void onUnload(const ChildSession& session) override
     {
+        const auto sessionId = session.getId();
         Log::info("Unloading [" + sessionId + "].");
-        const unsigned intSessionId = Util::decodeId(sessionId);
 
-        if (_loKitDocument == nullptr)
-        {
-            Log::error("Unloading session [" + sessionId + "] without loKitDocument.");
-            return;
-        }
-
-        // Find this session connection.
-        int sessionViewId = -1;
+        // Broadcast the demise and removal of session.
         {
             std::unique_lock<std::mutex> lock(_mutex);
 
-            const auto it = _connections.find(intSessionId);
-            if (it == _connections.end() || !it->second || !it->second->getSession())
+            // We should be removed by this point, otherwise
+            // our closed connection will throw, if not segfault.
+            for (const auto& pair : _connections)
             {
-                Log::error("Session [" + sessionId + "] not found to unload.");
-                return;
+                assert(sessionId != pair.second->getSessionId() && "Unloading connection still lingering.");
+                pair.second->getSession()->sendTextFrame("remview: " + sessionId);
             }
 
-            sessionViewId = it->second->getSession()->getViewId();
+            if (_loKitDocument == nullptr)
+            {
+                Log::error("Unloading session [" + sessionId + "] without loKitDocument.");
+                return;
+            }
         }
 
         --_clientViews;
-        Log::info("Session " + sessionId + " is unloading. " + std::to_string(_clientViews) +
-                  " view" + (_clientViews != 1 ? "s" : "") + " remain.");
+        Log::info() << "Document [" << _url << "] session ["
+                    << sessionId << "] unloaded, " << _clientViews
+                    << " view" << (_clientViews != 1 ? "s" : "")
+                    << Log::end;
 
         if (_multiView)
         {
-            Log::info() << "Document [" << _url << "] session ["
-                        << sessionId << "] unloaded, leaving "
-                        << _clientViews << " views." << Log::end;
-
             std::unique_lock<std::mutex> lock(_loKitDocument->getLock());
 
-            const auto viewId = _loKitDocument->getView();
-            if (viewId != sessionViewId)
-            {
-                Log::error() << "Unloading view [" << sessionViewId
-                             << "] from view [" << viewId << "]." << Log::end;
-                return;
-            }
-
+            const auto viewId = session.getViewId();
             _viewIdToCallbackDescr.erase(viewId);
+            _loKitDocument->setView(viewId);
             _loKitDocument->registerCallback(nullptr, nullptr);
             _loKitDocument->destroyView(viewId);
+            Log::debug("Destroyed view " + std::to_string(viewId));
         }
     }