[Libreoffice-commits] online.git: loolwsd/DocumentBroker.cpp loolwsd/DocumentBroker.hpp loolwsd/TileCache.cpp loolwsd/TileCache.hpp
Ashod Nakashian
ashod.nakashian at collabora.co.uk
Mon May 23 01:52:55 UTC 2016
loolwsd/DocumentBroker.cpp | 49 +++++++++++++++++++++++++--------------------
loolwsd/DocumentBroker.hpp | 6 ++++-
loolwsd/TileCache.cpp | 49 ++++++++++++++++++++++++++++++++++++---------
loolwsd/TileCache.hpp | 4 ++-
4 files changed, 76 insertions(+), 32 deletions(-)
New commits:
commit e5aaac7631511df2d303e9627ff896b5e91bc186
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date: Sun May 22 14:31:18 2016 -0400
loolwsd: Tile versioning and fix to race conditions
Tile invalidation and painting can race with one another.
A race when the user types two characters in quick succession:
1. After the first key press, the tile is invalidated.
2. The client request the tile on receiving the invalidate.
3. TileCache doesn't find it, and adds subscription.
A. Sometime before rendering, the second key press is received.
B. This invalidates the very same tile.
C. The client request the same tile.
D. TileCache finds a subscription and ignores the new one.
4. The tile is rendered and sent back.
5. Subscription is found and the tile is forwarded to clients.
6. Subcription is removed as the request is fullfilled.
E. The second tile is rendered and sent back.
F. TileCache finds no subscription and the tile is dropped.
End result: Only the first character appears on the screen.
Versioning fixes the above situation by making sure that in
step 5 the subscription will show to belong to a different
(and newer version) and so the tile will be ignored.
Instead, at F the TileCache will find both subscription
and a matching version and the lastest version will always
be sent back to the client.
Change-Id: I7d7fe1407cda1908d794683c3ce4c2fd18609a2f
Reviewed-on: https://gerrit.libreoffice.org/25341
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 62dfe60..c555298 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -109,7 +109,8 @@ DocumentBroker::DocumentBroker(const Poco::URI& uriPublic,
_lastSaveTime(std::chrono::steady_clock::now()),
_markToDestroy(false),
_isLoaded(false),
- _isModified(false)
+ _isModified(false),
+ _tileVersion(0)
{
assert(!_docKey.empty());
assert(!_childRoot.empty());
@@ -411,22 +412,22 @@ bool DocumentBroker::handleInput(const std::vector<char>& payload)
return true;
}
-void DocumentBroker::handleTileRequest(const TileDesc& tile,
+void DocumentBroker::handleTileRequest(TileDesc& tile,
const std::shared_ptr<ClientSession>& session)
{
- const auto tileMsg = tile.serialize();
- Log::trace() << "Tile request for " << tileMsg << Log::end;
-
std::unique_lock<std::mutex> lock(_mutex);
- std::unique_ptr<std::fstream> cachedTile = _tileCache->lookupTile(tile);
+ tile.setVersion(++_tileVersion);
+ const auto tileMsg = tile.serialize();
+ Log::trace() << "Tile request for " << tile.serialize() << Log::end;
+ std::unique_ptr<std::fstream> cachedTile = _tileCache->lookupTile(tile);
if (cachedTile)
{
#if ENABLE_DEBUG
- const std::string response = "tile:" + tileMsg + " renderid=cached\n";
+ const std::string response = tile.serialize("tile:") + " renderid=cached\n";
#else
- const std::string response = "tile:" + tileMsg + "\n";
+ const std::string response = tile.serialize("tile:") + "\n";
#endif
std::vector<char> output;
@@ -447,29 +448,30 @@ void DocumentBroker::handleTileRequest(const TileDesc& tile,
return;
}
- if (tileCache().isTileBeingRenderedIfSoSubscribe(tile, session))
- return;
-
- Log::debug() << "Sending render request for tile (" << tile.getPart() << ',' << tile.getTilePosX() << ',' << tile.getTilePosY() << ")." << Log::end;
+ if (tileCache().isTileBeingRenderedIfSoSubscribe(tile, session) > 0)
+ {
+ Log::debug() << "Sending render request for tile (" << tile.getPart() << ',' << tile.getTilePosX() << ',' << tile.getTilePosY() << ")." << Log::end;
- // Forward to child to render.
- const std::string request = "tile " + tileMsg;
- _childProcess->getWebSocket()->sendFrame(request.data(), request.size());
+ // Forward to child to render.
+ const std::string request = "tile " + tile.serialize();
+ _childProcess->getWebSocket()->sendFrame(request.data(), request.size());
+ }
}
void DocumentBroker::handleTileCombinedRequest(TileCombined& tileCombined,
const std::shared_ptr<ClientSession>& session)
{
- Log::trace() << "TileCombined request for " << tileCombined.serialize() << Log::end;
-
std::unique_lock<std::mutex> lock(_mutex);
+ tileCombined.setVersion(++_tileVersion);
+ Log::trace() << "TileCombined request for " << tileCombined.serialize() << Log::end;
+
// Satisfy as many tiles from the cache.
auto& tiles = tileCombined.getTiles();
int i = tiles.size();
while (--i >= 0)
{
- const auto& tile = tiles[i];
+ auto& tile = tiles[i];
std::unique_ptr<std::fstream> cachedTile = _tileCache->lookupTile(tile);
if (cachedTile)
{
@@ -499,10 +501,15 @@ void DocumentBroker::handleTileCombinedRequest(TileCombined& tileCombined,
// Remove.
tiles.erase(tiles.begin() + i);
}
- else if (tileCache().isTileBeingRenderedIfSoSubscribe(tile, session))
+ else
{
- // Skip.
- tiles.erase(tiles.begin() + i);
+ tile.setVersion(_tileVersion);
+ const auto ver = tileCache().isTileBeingRenderedIfSoSubscribe(tile, session);
+ if (ver <= 0)
+ {
+ // Skip.
+ tiles.erase(tiles.begin() + i);
+ }
}
}
diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp
index b722d76..3c2a342 100644
--- a/loolwsd/DocumentBroker.hpp
+++ b/loolwsd/DocumentBroker.hpp
@@ -202,7 +202,7 @@ public:
/// Removes a session by ID. Returns the new number of sessions.
size_t removeSession(const std::string& id);
- void handleTileRequest(const TileDesc& tile,
+ void handleTileRequest(TileDesc& tile,
const std::shared_ptr<ClientSession>& session);
void handleTileCombinedRequest(TileCombined& tileCombined,
const std::shared_ptr<ClientSession>& session);
@@ -245,6 +245,10 @@ private:
std::condition_variable _saveCV;
std::mutex _saveMutex;
+ /// Versioning is used to prevent races between
+ /// painting and invalidation.
+ std::atomic<size_t> _tileVersion;
+
static constexpr auto IdleSaveDurationMs = 30 * 1000;
static constexpr auto AutoSaveDurationMs = 300 * 1000;
};
diff --git a/loolwsd/TileCache.cpp b/loolwsd/TileCache.cpp
index d0f4e06..48721d2 100644
--- a/loolwsd/TileCache.cpp
+++ b/loolwsd/TileCache.cpp
@@ -75,11 +75,14 @@ TileCache::~TileCache()
struct TileCache::TileBeingRendered
{
std::vector<std::weak_ptr<ClientSession>> _subscribers;
- TileBeingRendered()
- : _startTime(std::chrono::steady_clock::now())
+ TileBeingRendered(const int version)
+ : _startTime(std::chrono::steady_clock::now()),
+ _ver(version)
{
}
+ int getVersion() const { return _ver; }
+
std::chrono::steady_clock::time_point getStartTime() const { return _startTime; }
void resetStartTime()
{
@@ -88,6 +91,7 @@ struct TileCache::TileBeingRendered
private:
std::chrono::steady_clock::time_point _startTime;
+ int _ver;
};
std::shared_ptr<TileCache::TileBeingRendered> TileCache::findTileBeingRendered(const TileDesc& tileDesc)
@@ -231,9 +235,12 @@ void TileCache::invalidateTiles(int part, int x, int y, int width, int height)
<< ", height: " << height << Log::end;
File dir(_cacheDir);
+
+ std::unique_lock<std::mutex> lock(_cacheMutex);
+ std::unique_lock<std::mutex> lockSubscribers(_tilesBeingRenderedMutex);
+
if (dir.exists() && dir.isDirectory())
{
- std::unique_lock<std::mutex> lock(_cacheMutex);
for (auto tileIterator = DirectoryIterator(dir); tileIterator != DirectoryIterator(); ++tileIterator)
{
const std::string fileName = tileIterator.path().getFileName();
@@ -244,6 +251,21 @@ void TileCache::invalidateTiles(int part, int x, int y, int width, int height)
}
}
}
+
+ // Forget this tile as it will have to be rendered again.
+ for (auto it = _tilesBeingRendered.begin(); it != _tilesBeingRendered.end(); )
+ {
+ const std::string cacheName = it->first;
+ if (intersectsTile(cacheName, part, x, y, width, height))
+ {
+ Log::debug("Removing subscriptions for: " + cacheName);
+ it = _tilesBeingRendered.erase(it);
+ }
+ else
+ {
+ ++it;
+ }
+ }
}
void TileCache::invalidateTiles(const std::string& tiles)
@@ -343,7 +365,16 @@ void TileCache::notifyAndRemoveSubscribers(const TileDesc& tile)
std::shared_ptr<TileBeingRendered> tileBeingRendered = findTileBeingRendered(tile);
if (!tileBeingRendered)
+ {
+ // We don't have anything to send back.
return;
+ }
+
+ if (tileBeingRendered->getVersion() != tile.getVersion())
+ {
+ Log::trace() << "Skipping unexpected tile ver: " << tile.getVersion() << ", waiting for " << tileBeingRendered->getVersion() << Log::end;
+ return;
+ }
const std::string message = tile.serialize("tile");
Log::debug("Sending tile message to subscribers: " + message);
@@ -365,7 +396,7 @@ void TileCache::notifyAndRemoveSubscribers(const TileDesc& tile)
}
// FIXME: to be further simplified when we centralize tile messages.
-bool TileCache::isTileBeingRenderedIfSoSubscribe(const TileDesc& tile, const std::shared_ptr<ClientSession> &subscriber)
+int TileCache::isTileBeingRenderedIfSoSubscribe(const TileDesc& tile, const std::shared_ptr<ClientSession> &subscriber)
{
std::unique_lock<std::mutex> lock(_tilesBeingRenderedMutex);
@@ -382,7 +413,7 @@ bool TileCache::isTileBeingRenderedIfSoSubscribe(const TileDesc& tile, const std
if (s.lock().get() == subscriber.get())
{
Log::debug("Redundant request to re-subscribe on a tile");
- return true;
+ return 0;
}
}
tileBeingRendered->_subscribers.push_back(subscriber);
@@ -391,10 +422,10 @@ bool TileCache::isTileBeingRenderedIfSoSubscribe(const TileDesc& tile, const std
if (std::chrono::duration_cast<std::chrono::milliseconds>(duration).count() > COMMAND_TIMEOUT_MS)
{
// Tile painting has stalled. Reissue.
- return false;
+ return tileBeingRendered->getVersion();
}
- return true;
+ return 0;
}
else
{
@@ -405,11 +436,11 @@ bool TileCache::isTileBeingRenderedIfSoSubscribe(const TileDesc& tile, const std
assert(_tilesBeingRendered.find(cachedName) == _tilesBeingRendered.end());
- tileBeingRendered = std::make_shared<TileBeingRendered>();
+ tileBeingRendered = std::make_shared<TileBeingRendered>(tile.getVersion());
tileBeingRendered->_subscribers.push_back(subscriber);
_tilesBeingRendered[cachedName] = tileBeingRendered;
- return false;
+ return tileBeingRendered->getVersion();
}
}
diff --git a/loolwsd/TileCache.hpp b/loolwsd/TileCache.hpp
index a93fc4f..f776b51 100644
--- a/loolwsd/TileCache.hpp
+++ b/loolwsd/TileCache.hpp
@@ -38,7 +38,9 @@ public:
TileCache(const TileCache&) = delete;
- bool isTileBeingRenderedIfSoSubscribe(const TileDesc& tile, const std::shared_ptr<ClientSession> &subscriber);
+ /// Subscribes if no subscription exists and returns the version number.
+ /// Otherwise returns 0 to signify a subscription exists.
+ int isTileBeingRenderedIfSoSubscribe(const TileDesc& tile, const std::shared_ptr<ClientSession> &subscriber);
std::unique_ptr<std::fstream> lookupTile(const TileDesc& tile);
More information about the Libreoffice-commits
mailing list