[Libreoffice-commits] online.git: Branch 'distro/collabora/collabora-online-3-0' - wsd/DocumentBroker.cpp wsd/DocumentBroker.hpp wsd/Storage.hpp

[Ashod Nakashian]

 wsd/DocumentBroker.cpp |   13 +++++++++----
 wsd/DocumentBroker.hpp |   10 +++++++++-
 wsd/Storage.hpp        |    2 +-
 3 files changed, 19 insertions(+), 6 deletions(-)

New commits:
commit f4dc93f55c74f28ab45cb1bf0edf7c2ee2a4ef6e
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sun Jan 14 18:59:11 2018 -0500

    wsd: separate lastSaveTime from lastSaveResponseTime
    
    Previously we assumed we are saving based on
    lastSaveTime, which is incorrect because it
    is set only upon successful saving and storing,
    which might fail.
    
    Now lastSaveResponseTime is used to track whether
    there are saving requests in flight. And lastSaveTime
    is only used when we do store the document in storage.
    
    Change-Id: I73e5c04432981d0cca11b8cf854414738bd894de
    Reviewed-on: https://gerrit.libreoffice.org/47884
    Reviewed-by: Michael Meeks <michael.meeks at collabora.com>
    Tested-by: Michael Meeks <michael.meeks at collabora.com>
    (cherry picked from commit 8ad29ee05006899a256016ed2ba00720dfc9b79f)
    Reviewed-on: https://gerrit.libreoffice.org/48342
    Reviewed-by: Jan Holesovsky <kendy at collabora.com>
    Tested-by: Jan Holesovsky <kendy at collabora.com>

diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 67c406b4..a9b62924 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -258,7 +258,7 @@ void DocumentBroker::pollThread()
             adminRecv = recv;
         }
 
-        if (_lastSaveTime < _lastSaveRequestTime &&
+        if (isSaving() &&
             std::chrono::duration_cast<std::chrono::milliseconds>
                     (now - _lastSaveRequestTime).count() <= COMMAND_TIMEOUT_MS)
         {
@@ -693,6 +693,9 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
 {
     assertCorrectThread();
 
+    // Record that we got a response to avoid timeing out on saving.
+    _lastSaveResponseTime = std::chrono::steady_clock::now();
+
     const bool isSaveAs = !saveAsPath.empty();
 
     // If save requested, but core didn't save because document was unmodified
@@ -732,7 +735,6 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
         // Nothing to do.
         LOG_DBG("Skipping unnecessary saving to URI [" << uri << "] with docKey [" << _docKey <<
                 "]. File last modified " << _lastFileModifiedTime.elapsed() / 1000000 << " seconds ago.");
-        _lastSaveTime = std::chrono::steady_clock::now();
         _poll->wakeup();
         return true;
     }
@@ -745,13 +747,13 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
     {
         if (!isSaveAs)
         {
+            // Saved and stored; update flags.
             setModified(false);
             _lastFileModifiedTime = newFileModifiedTime;
             _tileCache->saveLastModified(_lastFileModifiedTime);
             _lastSaveTime = std::chrono::steady_clock::now();
-            _poll->wakeup();
 
-            // So set _documentLastModifiedTime then
+            // Save the storage timestamp.
             _documentLastModifiedTime = _storage->getFileInfo()._modifiedTime;
 
             // After a successful save, we are sure that document in the storage is same as ours
@@ -760,6 +762,9 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
             Log::debug() << "Saved docKey [" << _docKey << "] to URI [" << uri
                          << "] and updated tile cache. Document modified timestamp: "
                          << _documentLastModifiedTime << Log::end;
+
+            // Resume polling.
+            _poll->wakeup();
         }
         else
         {
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index 18c6e624..0b8153ad 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -364,6 +364,9 @@ private:
     /// Saves the doc to the storage.
     bool saveToStorageInternal(const std::string& sesionId, bool success, const std::string& result = "", const std::string& saveAsPath = std::string(), const std::string& saveAsFilename = std::string());
 
+    /// True iff a save is in progress (requested but not completed).
+    bool isSaving() const { return _lastSaveResponseTime < _lastSaveRequestTime; }
+
     /// True iff there is at least one non-readonly session other than the given.
     /// Since only editable sessions can save, we need to use the last to
     /// save modified documents, otherwise we'll potentially have to save on
@@ -408,14 +411,19 @@ private:
     /// document was modified and saved or not.
     std::chrono::steady_clock::time_point _lastSaveTime;
 
-    /// The last time we sent a save request.
+    /// The last time we sent a save request to lokit.
     std::chrono::steady_clock::time_point _lastSaveRequestTime;
 
+    /// The last time we received a response for a save request from lokit.
+    std::chrono::steady_clock::time_point _lastSaveResponseTime;
+
     /// The document's last-modified time on storage.
     Poco::Timestamp _documentLastModifiedTime;
 
     /// The jailed file last-modified time.
     Poco::Timestamp _lastFileModifiedTime;
+
+    /// All session of this DocBroker by ID.
     std::map<std::string, std::shared_ptr<ClientSession> > _sessions;
 
     std::unique_ptr<StorageBase> _storage;
diff --git a/wsd/Storage.hpp b/wsd/Storage.hpp
index c4aff64f..e55e3047 100644
--- a/wsd/Storage.hpp
+++ b/wsd/Storage.hpp
@@ -137,7 +137,7 @@ public:
     {
         // Could assert here that it is in the same directory?
         _jailedFilePath = newPath;
-    };
+    }
 
     bool isLoaded() const { return _isLoaded; }