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

[Ashod Nakashian]

 loolwsd/DocumentBroker.cpp |   16 ++++++++--------
 loolwsd/LOOLWSD.cpp        |   12 ++++++++----
 2 files changed, 16 insertions(+), 12 deletions(-)

New commits:
commit 702dd48f1f7f671d918fbadcf98fb4ae4743dbbe
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sun May 8 10:07:17 2016 -0400

    loolwsd: safer document saving
    
    Relying on the filesystem to tell us when the document
    was last modified (to decide whether to upload to storage or not,)
    proved unreliable.
    
    Now we always upload to storage if there is only one client.
    This both minimizes the risk and also avoids the file timestamp
    check as a workaround to the problem of re-uploading documents
    as many time as there were clients. Since with one client we
    can only upload no more than once per save, which is reasonable.
    
    Furthermore, when a client disconnects we auto-save automatically
    as a matter of precaution. However, when there are other clients
    still connected, we don't wait for the save to complete, rather
    we let that job to the very last one.
    
    Change-Id: I94a2e4bddaed30a6c9c0e69f8006667d33c5b8ee
    Reviewed-on: https://gerrit.libreoffice.org/24767
    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 4c8591a..adde590 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -177,7 +177,9 @@ bool DocumentBroker::save()
 
     const auto uri = _uriPublic.toString();
 
-    // If the file hasn't been modified within the past 10 seconds, skip saving.
+    // If we aren't potentially destroying just yet, and the file hasn't been
+    // modified within the past 10 seconds, skip saving.
+    //
     // FIXME this is because currently the ChildProcessSession broadcasts the
     // unocommandresult, so we get called several times here, and have no real
     // possibility to distinguish who was the 1st caller.
@@ -187,10 +189,11 @@ bool DocumentBroker::save()
     // is planned post-release.
     const auto newFileModifiedTime = Poco::File(_storage->getLocalRootPath()).getLastModified();
     const auto elapsed = newFileModifiedTime - _lastFileModifiedTime;
-    if (std::abs(elapsed) > 10 * 1000 * 1000)
+    if (!canDestroy() && std::abs(elapsed) > 10 * 1000 * 1000)
     {
         // Nothing to do.
-        Log::debug("Skipping unnecessary saving to URI [" + uri + "].");
+        Log::debug() << "Skipping unnecessary saving to URI [" << uri
+                     << "]. File last modified " << elapsed << " ms ago." << Log::end;
         return true;
     }
 
@@ -502,11 +505,8 @@ bool DocumentBroker::canDestroy()
 {
     std::unique_lock<std::mutex> lock(_mutex);
 
-    if (_sessions.size() == 1)
-    {
-        // Last view going away, can destroy.
-        _markToDestroy = true;
-    }
+    // Last view going away, can destroy.
+    _markToDestroy = (_sessions.size() <= 1);
 
     return _markToDestroy;
 }
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 37c71a9..25a3801 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -648,15 +648,19 @@ private:
                 [&session]() { session->closeFrame(); },
                 [&queueHandlerThread]() { return TerminationFlag || !queueHandlerThread.isRunning(); });
 
-            const bool canDestroy = docBroker->canDestroy();
-            if (canDestroy && !session->_bLoadError)
+            if (!session->_bLoadError)
             {
-                Log::info("Shutdown of the last session, saving the document before tearing down.");
+                // If we are the last, we must wait for the save to complete.
+                const bool canDestroy = docBroker->canDestroy();
+                if (canDestroy)
+                {
+                    Log::info("Shutdown of the last session, saving the document before tearing down.");
+                }
 
                 // 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.
-                if (!docBroker->autoSave(true, COMMAND_TIMEOUT_MS))
+                if (!docBroker->autoSave(canDestroy, COMMAND_TIMEOUT_MS))
                 {
                     Log::error("Auto-save before closing failed.");
                 }