strepsirrhini/libreoffice-online
3
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

loolwsd: Tile versioning and fix to race conditions

Browse source 
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@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This commit is contained in:
Ashod Nakashian 2016-05-22 14:31:18 -04:00 committed by Ashod Nakashian
parent 55df9b85e1
commit e5aaac7631
4 changed files with 76 additions and 32 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

49
loolwsd/DocumentBroker.cpp
View file

@ -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;
if (tileCache().isTileBeingRenderedIfSoSubscribe(tile, session) > 0)
{
Log::debug() << "Sending render request for tile (" << tile.getPart() << ',' << tile.getTilePosX() << ',' << tile.getTilePosY() << ")." << Log::end;
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);
}
}
}

6
loolwsd/DocumentBroker.hpp
View file

@ -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;
};

49
loolwsd/TileCache.cpp
View file

@ -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();
}
}

4
loolwsd/TileCache.hpp
View file

@ -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);