loolwsd/DocumentBroker.cpp | 31 +++++++++++++++++++++++++------ loolwsd/DocumentBroker.hpp | 2 ++ 2 files changed, 27 insertions(+), 6 deletions(-)
New commits: commit 342125b8ead18ac79dd644063e58496afd5d7d38 Author: Ashod Nakashian <ashod.nakash...@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 <ashnak...@gmail.com> Tested-by: Ashod Nakashian <ashnak...@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; _______________________________________________ Libreoffice-commits mailing list libreoffice-comm...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits