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

[Ashod Nakashian]

 loolwsd/DocumentBroker.cpp |   30 +++++++++++++++++++++++++++---
 loolwsd/DocumentBroker.hpp |   13 ++++++++++++-
 loolwsd/LOOLWSD.cpp        |   16 ++++++++++++----
 3 files changed, 51 insertions(+), 8 deletions(-)

New commits:
commit 280669c253af08c001238b9465d6fa4e048cb545
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sun Apr 10 22:07:09 2016 -0400

    loolwsd: save on disconnection
    
    Last client disconnection now correctly issues a save
    and waits for the confirmation before tearing down
    the sockets, queues and threads.
    
    Change-Id: I28c28d79a17d359e9aa1fe67b983ca9fb592b847
    Reviewed-on: https://gerrit.libreoffice.org/23978
    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 b2d93f1..c7caba4 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -130,6 +130,8 @@ bool DocumentBroker::load(const std::string& jailId)
 
 bool DocumentBroker::save()
 {
+    std::unique_lock<std::mutex> lock(_saveMutex);
+
     const auto uri = _uriPublic.toString();
     Log::debug("Saving to URI [" + uri + "].");
 
@@ -139,6 +141,7 @@ bool DocumentBroker::save()
         _lastSaveTime = std::chrono::steady_clock::now();
         _tileCache->documentSaved();
         Log::debug("Saved to URI [" + uri + "] and updated tile cache.");
+        _saveCV.notify_all();
         return true;
     }
 
@@ -146,13 +149,13 @@ bool DocumentBroker::save()
     return false;
 }
 
-void DocumentBroker::autoSave()
+bool DocumentBroker::autoSave(const bool force)
 {
     std::unique_lock<std::mutex> sessionsLock(_wsSessionsMutex);
     if (_wsSessions.empty())
     {
         // Shouldn't happen.
-        return;
+        return false;
     }
 
     // Find the most recent activity.
@@ -170,8 +173,10 @@ void DocumentBroker::autoSave()
     if (inactivityTimeMs < timeSinceLastSaveMs)
     {
         // Either we've been idle long enough, or it's auto-save time.
+        // Or we are asked to save anyway.
         if (inactivityTimeMs >= IdleSaveDurationMs ||
-            timeSinceLastSaveMs >= AutoSaveDurationMs)
+            timeSinceLastSaveMs >= AutoSaveDurationMs ||
+            force)
         {
             Log::info("Auto-save triggered for doc [" + _docKey + "].");
 
@@ -192,8 +197,27 @@ void DocumentBroker::autoSave()
             {
                 Log::error("Failed to auto-save doc [" + _docKey + "]: No valid sessions.");
             }
+
+            return sent;
         }
     }
+
+    return false;
+}
+
+bool DocumentBroker::waitSave(const size_t timeoutMs)
+{
+    std::unique_lock<std::mutex> lock(_saveMutex);
+
+    // Remeber the last save time, since this is the predicate.
+    const auto lastSaveTime = _lastSaveTime;
+
+    if (_saveCV.wait_for(lock, std::chrono::milliseconds(timeoutMs)) == std::cv_status::no_timeout)
+    {
+        return true;
+    }
+
+    return (lastSaveTime != _lastSaveTime);
 }
 
 std::string DocumentBroker::getJailRoot() const
diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp
index 17a1cfe..27a527a 100644
--- a/loolwsd/DocumentBroker.hpp
+++ b/loolwsd/DocumentBroker.hpp
@@ -13,6 +13,7 @@
 #include <signal.h>
 
 #include <atomic>
+#include <chrono>
 #include <memory>
 #include <mutex>
 #include <string>
@@ -131,7 +132,15 @@ public:
 
     bool save();
 
-    void autoSave();
+    /// Save the document if there was activity since last save.
+    /// force when true, will force saving immediatly, regardless
+    /// of how long ago the activity was.
+    bool autoSave(const bool force);
+
+    /// Wait until the document is saved next.
+    /// This is used to cleanup after the last save.
+    /// Returns false if times out.
+    bool waitSave(const size_t timeoutMs);
 
     Poco::URI getPublicUri() const { return _uriPublic; }
     Poco::URI getJailedUri() const { return _uriJailed; }
@@ -178,6 +187,8 @@ private:
     std::unique_ptr<TileCache> _tileCache;
     std::shared_ptr<ChildProcess> _childProcess;
     std::mutex _mutex;
+    std::condition_variable _saveCV;
+    std::mutex _saveMutex;
 
     static constexpr auto IdleSaveDurationMs = 30 * 1000;
     static constexpr auto AutoSaveDurationMs = 300 * 1000;
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 989c3f7..0f5a391 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -592,10 +592,18 @@ private:
 
         if (docBroker->getSessionsCount() == 1 && !normalShutdown && !session->_bLoadError)
         {
-            //TODO: This isn't this simple. We need to wait for the notification
-            // of save so Storage can persist the save (if necessary).
             Log::info("Non-deliberate shutdown of the last session, saving the document before tearing down.");
-            queue->put("uno .uno:Save");
+
+            // Use auto-save to save only when there are modifications since last save.
+            // We also need to wait until the save notification reaches us
+            // and Storage persists the document.
+            // Note: technically, there is a race between these two (we should
+            // hold the broker lock before issueing the save and waiting,)
+            // but in practice this shouldn't happen.
+            if (docBroker->autoSave(true) && !docBroker->waitSave(5000))
+            {
+                Log::error("Auto-save before closing failed.");
+            }
         }
         else
         {
@@ -1562,7 +1570,7 @@ int LOOLWSD::main(const std::vector<std::string>& /*args*/)
                         std::unique_lock<std::mutex> docBrokersLock(docBrokersMutex);
                         for (auto& brokerIt : docBrokers)
                         {
-                            brokerIt.second->autoSave();
+                            brokerIt.second->autoSave(false);
                         }
                     }
                     catch (const std::exception& exc)