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

[Ashod Nakashian]

 loolwsd/DocumentBroker.cpp |   31 +++++++++++++++++++++++++------
 loolwsd/DocumentBroker.hpp |    2 ++
 2 files changed, 27 insertions(+), 6 deletions(-)

New commits:
commit 342125b8ead18ac79dd644063e58496afd5d7d38
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Thu Sep 22 17:48:37 2016 -0400

    bccu#2018 - Missing tiles when finished typing
    
    aka: don't save invalidated tiles into cache
    
    Tiles that are rendered when invalidation is recieved are outdated.
    
    What we do is remember the last tile version when we get an invalidation.
    This tile version, and any preceding it, is out of date and should not
    get saved in the tile cache.
    
    This fixes a race between rendering and invalidation, which happens
    in the following scenario.
    
    1. Tile requested for rendering.
    2. User inputs key.
    3. While tile is rendering, the tile is invalidated in Core
       (the shared lock between rendering and processing input doesn't
       include parsing tile request or preparing the payload back).
    4. Once the tile rendering is done, but before it's encoded and sent back,
       the invalidation is received on the callback.
    5. Tile cache is cleared of that tile.
    6. The outdated tile is received on its way to the client, saved in cache.
    7. Client requests the tile anew upon getting the invalidation.
    8. The tile is served from the cache, which is outdated, therefore
       missing the last key input form the user.
    
    The fix is in #3 to remember the version number of the tile being
    rendered. Then in #6 we forward the tile to the client, but we
    do not store it in cache. In #8 the tile request results in a new
    rendering, since the cache will not have the tile. Only this last
    rendering of the tile is saved in cache for future requests.
    
    Change-Id: I0b0a3c2b917906c0d0c9046e3e6d3d4d354a7777
    Reviewed-on: https://gerrit.libreoffice.org/29209
    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 c241594..f55f3ed 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -112,7 +112,8 @@ DocumentBroker::DocumentBroker() :
     _cursorHeight(0),
     _isLoaded(false),
     _isModified(false),
-    _tileVersion(0)
+    _tileVersion(0),
+    _invalidatedTileVersion(0)
 {
     Log::info("Empty DocumentBroker (marked to destroy) created.");
 }
@@ -135,7 +136,8 @@ DocumentBroker::DocumentBroker(const Poco::URI& uriPublic,
     _cursorHeight(0),
     _isLoaded(false),
     _isModified(false),
-    _tileVersion(0)
+    _tileVersion(0),
+    _invalidatedTileVersion(0)
 {
     assert(!_docKey.empty());
     assert(!_childRoot.empty());
@@ -470,8 +472,9 @@ void DocumentBroker::invalidateTiles(const std::string& tiles)
     // Remove from cache.
     _tileCache->invalidateTiles(tiles);
 
-    //TODO: Re-issue the tiles again to avoid races.
-
+    // Any older tile than this (inclusive) is not to be trusted.
+    _invalidatedTileVersion = _tileVersion;
+    Log::trace("Last invalidated tile version: " + std::to_string(_invalidatedTileVersion));
 }
 
 void DocumentBroker::handleTileRequest(TileDesc& tile,
@@ -588,6 +591,14 @@ void DocumentBroker::handleTileResponse(const std::vector<char>& payload)
     {
         auto tile = TileDesc::parse(firstLine);
 
+        bool cache = true;
+        if (_invalidatedTileVersion > 0 && tile.getVersion() <= _invalidatedTileVersion)
+        {
+            // Drop from the cache, but forward to clients nontheless.
+            Log::info() << "Dropping potentially invalidated tile (cutoff "
+                        << _invalidatedTileVersion << "): " << firstLine << Log::end;
+            cache = false;
+        }
 
         const auto length = payload.size();
         if (firstLine.size() < static_cast<std::string::size_type>(length) - 1)
@@ -595,7 +606,7 @@ void DocumentBroker::handleTileResponse(const std::vector<char>& payload)
             const auto buffer = payload.data();
             tileCache().notifySubscribers(
                 tile, buffer + firstLine.size() + 1,
-                length - firstLine.size() - 1, true);
+                length - firstLine.size() - 1, cache);
         }
         else
         {
@@ -621,6 +632,14 @@ void DocumentBroker::handleTileCombinedResponse(const std::vector<char>& payload
     {
         auto tileCombined = TileCombined::parse(firstLine);
 
+        bool cache = true;
+        if (_invalidatedTileVersion > 0 && tileCombined.getVersion() <= _invalidatedTileVersion)
+        {
+            // Drop from the cache, but forward to clients nontheless.
+            Log::info() << "Dropping potentially invalidated tile (cutoff "
+                        << _invalidatedTileVersion << "): " << firstLine << Log::end;
+            cache = false;
+        }
 
         const auto length = payload.size();
         if (firstLine.size() < static_cast<std::string::size_type>(length) - 1)
@@ -629,7 +648,7 @@ void DocumentBroker::handleTileCombinedResponse(const std::vector<char>& payload
             auto offset = firstLine.size() + 1;
             for (const auto& tile : tileCombined.getTiles())
             {
-                tileCache().notifySubscribers(tile, buffer + offset, tile.getImgSize(), true);
+                tileCache().notifySubscribers(tile, buffer + offset, tile.getImgSize(), cache);
                 offset += tile.getImgSize();
             }
         }
diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp
index a547a32..29330c8 100644
--- a/loolwsd/DocumentBroker.hpp
+++ b/loolwsd/DocumentBroker.hpp
@@ -267,6 +267,8 @@ private:
     /// Versioning is used to prevent races between
     /// painting and invalidation.
     std::atomic<size_t> _tileVersion;
+    /// The last version number when invalidation happened.
+    std::atomic<int> _invalidatedTileVersion;
 
     static constexpr auto IdleSaveDurationMs = 30 * 1000;
     static constexpr auto AutoSaveDurationMs = 300 * 1000;