loolwsd/ChildSession.cpp | 6 ++++ loolwsd/ClientSession.cpp | 8 ++++-- loolwsd/ClientSession.hpp | 9 +++++++ loolwsd/DocumentBroker.cpp | 14 ++--------- loolwsd/DocumentBroker.hpp | 2 - loolwsd/LOOLWSD.cpp | 28 +++++++++++++++++----- loolwsd/TileCache.cpp | 49 +++++----------------------------------- loolwsd/TileCache.hpp | 3 -- loolwsd/test/Makefile.am | 1 loolwsd/test/TileCacheTests.cpp | 14 ----------- loolwsd/test/helpers.hpp | 9 ------- 11 files changed, 53 insertions(+), 90 deletions(-)
New commits: commit 16605b928999d10c8e75c3d9c0a959573f7ef55b Author: Ashod Nakashian <[email protected]> Date: Thu Sep 1 08:27:18 2016 -0400 Revert "loolwsd: canceltiles re-designed using tilecache instead of queue" This reverts commit 571ff06906960c9840cd65a35914ca0607f72a11. Change-Id: Icf9caeafc640b815f64211f240cfdac8e91694a1 Reviewed-on: https://gerrit.libreoffice.org/28595 Reviewed-by: Ashod Nakashian <[email protected]> Tested-by: Ashod Nakashian <[email protected]> diff --git a/loolwsd/ChildSession.cpp b/loolwsd/ChildSession.cpp index 88ece98..93a2312 100644 --- a/loolwsd/ChildSession.cpp +++ b/loolwsd/ChildSession.cpp @@ -119,6 +119,12 @@ bool ChildSession::_handleInput(const char *buffer, int length) // Just to update the activity of a view-only client. return true; } + else if (tokens[0] == "canceltiles") + { + // This command makes sense only on the command queue level. + // Shouldn't get this here. + return true; + } else if (tokens[0] == "commandvalues") { return getCommandValues(buffer, length, tokens); diff --git a/loolwsd/ClientSession.cpp b/loolwsd/ClientSession.cpp index 40deeee..901d138 100644 --- a/loolwsd/ClientSession.cpp +++ b/loolwsd/ClientSession.cpp @@ -147,8 +147,10 @@ bool ClientSession::_handleInput(const char *buffer, int length) } else if (tokens[0] == "canceltiles") { - _docBroker->cancelTileRequests(shared_from_this()); - return true; + if (!_peer.expired()) + { + return forwardToPeer(_peer, buffer, length, false); + } } else if (tokens[0] == "commandvalues") { diff --git a/loolwsd/DocumentBroker.cpp b/loolwsd/DocumentBroker.cpp index e313ae3..a89b9d3 100644 --- a/loolwsd/DocumentBroker.cpp +++ b/loolwsd/DocumentBroker.cpp @@ -616,13 +616,6 @@ void DocumentBroker::handleTileCombinedRequest(TileCombined& tileCombined, } } -void DocumentBroker::cancelTileRequests(const std::shared_ptr<ClientSession>& session) -{ - std::unique_lock<std::mutex> lock(_mutex); - - tileCache().cancelTiles(session); -} - void DocumentBroker::handleTileResponse(const std::vector<char>& payload) { const std::string firstLine = getFirstLine(payload); diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp index 80bf0a3..54ca138 100644 --- a/loolwsd/DocumentBroker.hpp +++ b/loolwsd/DocumentBroker.hpp @@ -214,8 +214,6 @@ public: void handleTileCombinedRequest(TileCombined& tileCombined, const std::shared_ptr<ClientSession>& session); - void cancelTileRequests(const std::shared_ptr<ClientSession>& session); - void handleTileResponse(const std::vector<char>& payload); void handleTileCombinedResponse(const std::vector<char>& payload); diff --git a/loolwsd/TileCache.cpp b/loolwsd/TileCache.cpp index 8f05a14..9b76a15 100644 --- a/loolwsd/TileCache.cpp +++ b/loolwsd/TileCache.cpp @@ -461,26 +461,4 @@ int TileCache::subscribeToTileRendering(const TileDesc& tile, const std::shared_ } } -void TileCache::cancelTiles(const std::shared_ptr<ClientSession> &subscriber) -{ - std::unique_lock<std::mutex> lock(_tilesBeingRenderedMutex); - - const auto sub = subscriber.get(); - - Log::trace("Cancelling tiles for " + subscriber->getName()); - - for (auto it = _tilesBeingRendered.begin(); it != _tilesBeingRendered.end(); ) - { - auto& subscribers = it->second->_subscribers; - Log::trace("Tile " + it->first + " has " + std::to_string(subscribers.size()) + " subscribers."); - subscribers.erase(std::remove_if(subscribers.begin(), subscribers.end(), - [sub](std::weak_ptr<ClientSession>& ptr){ return ptr.lock().get() == sub; }), - subscribers.end()); - Log::trace(" Tile " + it->first + " has " + std::to_string(subscribers.size()) + " subscribers."); - - // Remove if there are no more subscribers on this tile. - it = (subscribers.empty() ? _tilesBeingRendered.erase(it) : ++it); - } -} - /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/loolwsd/TileCache.hpp b/loolwsd/TileCache.hpp index 5703227..bbe5aa0 100644 --- a/loolwsd/TileCache.hpp +++ b/loolwsd/TileCache.hpp @@ -42,9 +42,6 @@ public: /// Otherwise returns 0 to signify a subscription exists. int subscribeToTileRendering(const TileDesc& tile, const std::shared_ptr<ClientSession> &subscriber); - /// Cancels all tile requests by the given subscriber. - void cancelTiles(const std::shared_ptr<ClientSession> &subscriber); - std::unique_ptr<std::fstream> lookupTile(const TileDesc& tile); void saveTileAndNotify(const TileDesc& tile, const char *data, const size_t size, const bool priority); diff --git a/loolwsd/test/TileCacheTests.cpp b/loolwsd/test/TileCacheTests.cpp index d2bfa5e..e44a18d 100644 --- a/loolwsd/test/TileCacheTests.cpp +++ b/loolwsd/test/TileCacheTests.cpp @@ -33,7 +33,6 @@ class TileCacheTests : public CPPUNIT_NS::TestFixture CPPUNIT_TEST(testSimple); CPPUNIT_TEST(testSimpleCombine); CPPUNIT_TEST(testPerformance); - CPPUNIT_TEST(testCancelTiles); CPPUNIT_TEST(testUnresponsiveClient); CPPUNIT_TEST(testClientPartImpress); CPPUNIT_TEST(testClientPartCalc); @@ -49,7 +48,6 @@ class TileCacheTests : public CPPUNIT_NS::TestFixture void testSimple(); void testSimpleCombine(); void testPerformance(); - void testCancelTiles(); void testUnresponsiveClient(); void testClientPartImpress(); void testClientPartCalc(); @@ -210,18 +208,6 @@ void TileCacheTests::testPerformance() socket.shutdown(); } -void TileCacheTests::testCancelTiles() -{ - const auto testName = "cancelTiles "; - auto socket = *loadDocAndGetSocket("load12.ods", _uri, testName); - - // Request a huge tile, and cancel immediately. - sendTextFrame(socket, "tilecombine part=0 width=2560 height=2560 tileposx=0 tileposy=0 tilewidth=38400 tileheight=38400"); - sendTextFrame(socket, "canceltiles"); - - assertNotInResponse(socket, "tile:", testName); -} - void TileCacheTests::testUnresponsiveClient() { const std::string docFilename = "hello.odt"; diff --git a/loolwsd/test/helpers.hpp b/loolwsd/test/helpers.hpp index 7933beb..0a7d003 100644 --- a/loolwsd/test/helpers.hpp +++ b/loolwsd/test/helpers.hpp @@ -330,15 +330,6 @@ std::string assertResponseLine(T& ws, const std::string& prefix, const std::stri return res; } -/// Assert that we don't get a response with the given prefix. -template <typename T> -std::string assertNotInResponse(T& ws, const std::string& prefix, const std::string name = "") -{ - const auto res = getResponseLine(ws, prefix, name); - CPPUNIT_ASSERT_MESSAGE("Did not expect getting message [" + res + "].", res.empty()); - return res; -} - inline void getResponseMessage(const std::shared_ptr<Poco::Net::WebSocket>& ws, const std::string& prefix, std::string& response, const bool isLine) { commit 77a693c353a35607742572e241be9324f5a345d5 Author: Ashod Nakashian <[email protected]> Date: Thu Sep 1 08:25:32 2016 -0400 Revert "loolwsd: remove tile queue and simplify tile response" This reverts commit 59eaacd2f87f46c4c4d2963ef54f4d20d346b2d0. Change-Id: Ieba9bbaaa6406e3e685b46ce12a44a0766127815 Reviewed-on: https://gerrit.libreoffice.org/28594 Reviewed-by: Ashod Nakashian <[email protected]> Tested-by: Ashod Nakashian <[email protected]> diff --git a/loolwsd/ClientSession.cpp b/loolwsd/ClientSession.cpp index c61c574..40deeee 100644 --- a/loolwsd/ClientSession.cpp +++ b/loolwsd/ClientSession.cpp @@ -37,9 +37,11 @@ using Poco::StringTokenizer; ClientSession::ClientSession(const std::string& id, std::shared_ptr<Poco::Net::WebSocket> ws, std::shared_ptr<DocumentBroker> docBroker, + std::shared_ptr<BasicTileQueue> queue, bool readOnly) : LOOLSession(id, Kind::ToClient, ws), _docBroker(std::move(docBroker)), + _queue(std::move(queue)), _haveEditLock(std::getenv("LOK_VIEW_CALLBACK")), _isReadOnly(readOnly), _loadFailed(false), diff --git a/loolwsd/ClientSession.hpp b/loolwsd/ClientSession.hpp index 3a9b1a1..dc531c8 100644 --- a/loolwsd/ClientSession.hpp +++ b/loolwsd/ClientSession.hpp @@ -23,6 +23,7 @@ public: ClientSession(const std::string& id, std::shared_ptr<Poco::Net::WebSocket> ws, std::shared_ptr<DocumentBroker> docBroker, + std::shared_ptr<BasicTileQueue> queue, bool isReadOnly = false); virtual ~ClientSession(); @@ -59,6 +60,11 @@ public: _loadFailed = true; } + void sendToInputQueue(const std::string& message) + { + _queue->put(message); + } + std::shared_ptr<DocumentBroker> getDocumentBroker() const { return _docBroker; } private: @@ -79,6 +85,9 @@ private: std::shared_ptr<DocumentBroker> _docBroker; + /// The incoming message queue. + std::shared_ptr<BasicTileQueue> _queue; + // If this document holds the edit lock. // An edit lock will only allow the current session to make edits, // while other session opening the same document can only see diff --git a/loolwsd/DocumentBroker.cpp b/loolwsd/DocumentBroker.cpp index 2ae259f..e313ae3 100644 --- a/loolwsd/DocumentBroker.cpp +++ b/loolwsd/DocumentBroker.cpp @@ -317,6 +317,7 @@ bool DocumentBroker::sendUnoSave(const bool dontSaveIfUnmodified) _lastFileModifiedTime.fromEpochTime(0); // We do not want save to terminate editing mode if we are in edit mode now + std::ostringstream oss; // arguments init oss << "{"; @@ -342,10 +343,8 @@ bool DocumentBroker::sendUnoSave(const bool dontSaveIfUnmodified) // arguments end oss << "}"; - const auto saveArgs = oss.str(); - Log::trace(".uno:Save arguments: " + saveArgs); - const auto command = "uno .uno:Save " + saveArgs; - sessionIt.second->handleInput(command.data(), command.size()); + Log::debug(".uno:Save arguments: " + oss.str()); + sessionIt.second->sendToInputQueue("uno .uno:Save " + oss.str()); return true; } } diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp index 0637bf3..db7f135 100644 --- a/loolwsd/LOOLWSD.cpp +++ b/loolwsd/LOOLWSD.cpp @@ -399,7 +399,7 @@ private: // Load the document. std::shared_ptr<WebSocket> ws; - auto session = std::make_shared<ClientSession>(id, ws, docBroker); + auto session = std::make_shared<ClientSession>(id, ws, docBroker, nullptr); // Request the child to connect to us and add this session. auto sessionsCount = docBroker->addSession(session); @@ -662,7 +662,10 @@ private: std::shared_ptr<ClientSession> session; try { - session = std::make_shared<ClientSession>(id, ws, docBroker, isReadOnly); + // For ToClient sessions, we store incoming messages in a queue and have a separate + // thread to pump them. This is to empty the queue when we get a "canceltiles" message. + auto queue = std::make_shared<BasicTileQueue>(); + session = std::make_shared<ClientSession>(id, ws, docBroker, queue, isReadOnly); if (!fileinfo._userName.empty()) { Log::debug(uriPublic.toString() + " requested with username [" + fileinfo._userName + "]"); @@ -689,13 +692,18 @@ private: ws->sendFrame(status.data(), (int) status.size()); // Let messages flow + QueueHandler handler(queue, session, "wsd_queue_" + session->getId()); + Thread queueHandlerThread; + queueHandlerThread.start(handler); + IoUtil::SocketProcessor(ws, - [&session](const std::vector<char>& payload) + [&queue](const std::vector<char>& payload) { - return session->handleInput(payload.data(), payload.size()); + queue->put(payload); + return true; }, [&session]() { session->closeFrame(); }, - []() { return !!TerminationFlag; }); + [&queueHandlerThread]() { return TerminationFlag || !queueHandlerThread.isRunning(); }); { std::unique_lock<std::mutex> docBrokersLock(docBrokersMutex); @@ -735,6 +743,12 @@ private: } } + if (session->isLoadFailed()) + { + Log::info("Clearing the queue."); + queue->clear(); + } + if (sessionsCount == 0) { std::unique_lock<std::mutex> docBrokersLock(docBrokersMutex); @@ -748,7 +762,9 @@ private: } LOOLWSD::dumpEventTrace(docBroker->getJailId(), id, "EndSession: " + uri); - Log::info("Finishing GET request handler for session [" + id + "]."); + Log::info("Finishing GET request handler for session [" + id + "]. Joining the queue."); + queue->put("eof"); + queueHandlerThread.join(); } catch (const std::exception& exc) { diff --git a/loolwsd/TileCache.cpp b/loolwsd/TileCache.cpp index ca6af9b..8f05a14 100644 --- a/loolwsd/TileCache.cpp +++ b/loolwsd/TileCache.cpp @@ -166,32 +166,19 @@ void TileCache::saveTileAndNotify(const TileDesc& tile, const char *data, const { if (!tileBeingRendered->_subscribers.empty()) { - std::string response = tile.serialize("tile:"); - Log::debug("Sending tile message to subscribers: " + response); - response += '\n'; - - std::vector<char> output; - output.reserve(static_cast<size_t>(4) * tile.getWidth() * tile.getHeight()); - output.resize(response.size()); - std::memcpy(output.data(), response.data(), response.size()); - - const auto pos = output.size(); - output.resize(pos + size); - std::memcpy(output.data() + pos, data, size); + const std::string message = tile.serialize("tile"); + Log::debug("Sending tile message to subscribers: " + message); for (const auto& i: tileBeingRendered->_subscribers) { auto subscriber = i.lock(); if (subscriber) { - try - { - subscriber->sendBinaryFrame(output.data(), output.size()); - } - catch (const std::exception& ex) - { - Log::warn("Failed to send tile to " + subscriber->getName() + ": " + ex.what()); - } + //FIXME: This is inefficient; should just send directly to each client (although that is risky as well! + // Re-emit the tile command in the other thread(s) to re-check and hit + // the cache. Construct the message from scratch to contain only the + // mandatory parts of the message. + subscriber->sendToInputQueue(message); } } } diff --git a/loolwsd/test/Makefile.am b/loolwsd/test/Makefile.am index 53f7d97..91401dc 100644 --- a/loolwsd/test/Makefile.am +++ b/loolwsd/test/Makefile.am @@ -21,7 +21,6 @@ wsd_sources = \ ../IoUtil.cpp \ ../Log.cpp \ ../LOOLProtocol.cpp \ - ../LOOLSession.cpp \ ../TileCache.cpp \ ../MessageQueue.cpp \ ../Unit.cpp \ _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
