loolwsd/Admin.cpp | 6 ---- loolwsd/ChildProcessSession.cpp | 9 +------ loolwsd/ChildProcessSession.hpp | 3 -- loolwsd/IoUtil.cpp | 6 ---- loolwsd/LOOLKit.cpp | 12 --------- loolwsd/LOOLSession.cpp | 8 +----- loolwsd/LOOLSession.hpp | 4 +-- loolwsd/MasterProcessSession.cpp | 19 +++++--------- loolwsd/MasterProcessSession.hpp | 4 +-- loolwsd/PROBLEMS | 16 ------------ loolwsd/protocol.txt | 11 -------- loolwsd/test/httpwstest.cpp | 50 +++++++++++++++++++++------------------ 12 files changed, 43 insertions(+), 105 deletions(-)
New commits: commit 4087ac808922dc1e19cb77a0e4b5b575ce8fcda7 Author: Tor Lillqvist <[email protected]> Date: Wed Apr 13 15:56:23 2016 +0300 With "disconnect" messages gone, no need for a 'reason' to disconnect() YAGNI. diff --git a/loolwsd/ChildProcessSession.cpp b/loolwsd/ChildProcessSession.cpp index aeb4219..a7d00ca 100644 --- a/loolwsd/ChildProcessSession.cpp +++ b/loolwsd/ChildProcessSession.cpp @@ -283,7 +283,7 @@ ChildProcessSession::~ChildProcessSession() _callbackThread.join(); } -void ChildProcessSession::disconnect(const std::string& reason) +void ChildProcessSession::disconnect() { if (!isDisconnected()) { @@ -295,15 +295,10 @@ void ChildProcessSession::disconnect(const std::string& reason) //TODO: Handle saving to temp etc. _onUnload(getId()); - LOOLSession::disconnect(reason); + LOOLSession::disconnect(); } } -bool ChildProcessSession::handleDisconnect(StringTokenizer& tokens) -{ - return LOOLSession::handleDisconnect(tokens); -} - bool ChildProcessSession::_handleInput(const char *buffer, int length) { if (isInactive() && _loKitDocument != nullptr) diff --git a/loolwsd/ChildProcessSession.hpp b/loolwsd/ChildProcessSession.hpp index 0f14842..bb55b68 100644 --- a/loolwsd/ChildProcessSession.hpp +++ b/loolwsd/ChildProcessSession.hpp @@ -51,8 +51,7 @@ public: virtual bool getPartPageRectangles(const char *buffer, int length) override; - virtual void disconnect(const std::string& reason = "") override; - virtual bool handleDisconnect(Poco::StringTokenizer& tokens) override; + virtual void disconnect() override; int getViewId() const { return _viewId; } diff --git a/loolwsd/LOOLSession.cpp b/loolwsd/LOOLSession.cpp index 2e32ee1..9dda2e6 100644 --- a/loolwsd/LOOLSession.cpp +++ b/loolwsd/LOOLSession.cpp @@ -179,7 +179,7 @@ void LOOLSession::parseDocOptions(const StringTokenizer& tokens, int& part, std: } } -void LOOLSession::disconnect(const std::string&) +void LOOLSession::disconnect() { try { @@ -195,7 +195,7 @@ void LOOLSession::disconnect(const std::string&) } } -bool LOOLSession::handleDisconnect(StringTokenizer& /*tokens*/) +bool LOOLSession::handleDisconnect() { _disconnected = true; IoUtil::shutdownWebSocket(_ws); diff --git a/loolwsd/LOOLSession.hpp b/loolwsd/LOOLSession.hpp index 2fc5429..8a5b503 100644 --- a/loolwsd/LOOLSession.hpp +++ b/loolwsd/LOOLSession.hpp @@ -49,10 +49,10 @@ public: bool handleInput(const char *buffer, int length); /// Invoked when we want to disconnect a session. - virtual void disconnect(const std::string& reason = ""); + virtual void disconnect(); /// Called to handle disconnection command from socket. - virtual bool handleDisconnect(Poco::StringTokenizer& tokens); + virtual bool handleDisconnect(); bool isInactive() const { return getInactivityMS() >= InactivityThresholdMS; } diff --git a/loolwsd/MasterProcessSession.cpp b/loolwsd/MasterProcessSession.cpp index bd96572..e67b332 100644 --- a/loolwsd/MasterProcessSession.cpp +++ b/loolwsd/MasterProcessSession.cpp @@ -59,11 +59,11 @@ MasterProcessSession::~MasterProcessSession() } } -void MasterProcessSession::disconnect(const std::string& reason) +void MasterProcessSession::disconnect() { if (!isDisconnected()) { - LOOLSession::disconnect(reason); + LOOLSession::disconnect(); // Release the save-as queue. _saveAsQueue.put(""); @@ -71,25 +71,20 @@ void MasterProcessSession::disconnect(const std::string& reason) auto peer = _peer.lock(); if (peer) { - peer->disconnect(reason); + peer->disconnect(); } } } -bool MasterProcessSession::handleDisconnect(Poco::StringTokenizer& tokens) +bool MasterProcessSession::handleDisconnect() { - Log::info("Graceful disconnect on " + getName() + " [" + - (tokens.count() > 1 ? tokens[1] : std::string("no reason")) + - "]."); + Log::info("Graceful disconnect on " + getName() + "."); - LOOLSession::handleDisconnect(tokens); + LOOLSession::handleDisconnect(); auto peer = _peer.lock(); if (peer) - { - const auto reason = (tokens.count() > 1 ? Poco::cat(std::string(" "), tokens.begin() + 1, tokens.end()) : ""); - peer->disconnect(reason); - } + peer->disconnect(); return false; } diff --git a/loolwsd/MasterProcessSession.hpp b/loolwsd/MasterProcessSession.hpp index 04d671d..47b2d97 100644 --- a/loolwsd/MasterProcessSession.hpp +++ b/loolwsd/MasterProcessSession.hpp @@ -36,8 +36,8 @@ class MasterProcessSession final : public LOOLSession, public std::enable_shared virtual bool getPartPageRectangles(const char *buffer, int length) override; - virtual void disconnect(const std::string& reason = "") override; - virtual bool handleDisconnect(Poco::StringTokenizer& tokens) override; + virtual void disconnect() override; + virtual bool handleDisconnect() override; /** * Return the URL of the saved-as document when it's ready. If called commit c9796255ae0a79598f27f4b82d1694480a39579a Author: Tor Lillqvist <[email protected]> Date: Wed Apr 13 15:50:09 2016 +0300 Don't check for undocumented "eof" frames read from a websocket either Now with "disconnect" frames gone, surely checks for "eof" ones can go away too, whatever they were. diff --git a/loolwsd/Admin.cpp b/loolwsd/Admin.cpp index 4a3ded3..899e902 100644 --- a/loolwsd/Admin.cpp +++ b/loolwsd/Admin.cpp @@ -110,12 +110,6 @@ void AdminRequestHandler::handleWSRequests(HTTPServerRequest& request, HTTPServe StringTokenizer tokens(firstLine, " ", StringTokenizer::TOK_IGNORE_EMPTY | StringTokenizer::TOK_TRIM); Log::trace("Recv: " + firstLine); - if (firstLine == "eof") - { - Log::info("Received EOF. Finishing."); - break; - } - if (tokens.count() < 1) continue; diff --git a/loolwsd/IoUtil.cpp b/loolwsd/IoUtil.cpp index c002230..081e13e 100644 --- a/loolwsd/IoUtil.cpp +++ b/loolwsd/IoUtil.cpp @@ -144,12 +144,6 @@ void SocketProcessor(std::shared_ptr<WebSocket> ws, break; } - if (firstLine == "eof") - { - Log::info("Received EOF. Finishing."); - break; - } - // Call the handler. const auto success = handler(payload); payload.resize(0); diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp index 2f34568..ce32008 100644 --- a/loolwsd/LOOLKit.cpp +++ b/loolwsd/LOOLKit.cpp @@ -301,12 +301,6 @@ public: if (n > 0 && (flags & WebSocket::FRAME_OP_BITMASK) != WebSocket::FRAME_OP_CLOSE) { std::string firstLine = getFirstLine(buffer, n); - if (firstLine == "eof") - { - Log::info("Received EOF. Finishing."); - break; - } - StringTokenizer tokens(firstLine, " ", StringTokenizer::TOK_IGNORE_EMPTY | StringTokenizer::TOK_TRIM); // Check if it is a "nextmessage:" and in that case read the large commit e5de11113b624895cec77f2598fb436b3006c660 Author: Tor Lillqvist <[email protected]> Date: Wed Apr 13 15:39:14 2016 +0300 Don't check for or send "disconnect" frames anywhere and don't document them Follow-up to 68b3a2c81e92de7be3d1b6bd503ff78b5c924422. diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp index 2b16af4..2f34568 100644 --- a/loolwsd/LOOLKit.cpp +++ b/loolwsd/LOOLKit.cpp @@ -309,12 +309,6 @@ public: StringTokenizer tokens(firstLine, " ", StringTokenizer::TOK_IGNORE_EMPTY | StringTokenizer::TOK_TRIM); - if (firstLine == "disconnect") - { - Log::info("Client disconnected [" + (tokens.count() == 2 ? tokens[1] : std::string("no reason")) + "]."); - break; - } - // Check if it is a "nextmessage:" and in that case read the large // follow-up message separately, and handle that only. int size; diff --git a/loolwsd/LOOLSession.cpp b/loolwsd/LOOLSession.cpp index 1b744f4..2e32ee1 100644 --- a/loolwsd/LOOLSession.cpp +++ b/loolwsd/LOOLSession.cpp @@ -179,16 +179,12 @@ void LOOLSession::parseDocOptions(const StringTokenizer& tokens, int& part, std: } } -void LOOLSession::disconnect(const std::string& reason) +void LOOLSession::disconnect(const std::string&) { try { if (!_disconnected) { - if (reason != "") - sendTextFrame("disconnect " + reason); - else - sendTextFrame("disconnect"); _disconnected = true; IoUtil::shutdownWebSocket(_ws); } diff --git a/loolwsd/PROBLEMS b/loolwsd/PROBLEMS index 89ed2c7..9b8507a 100644 --- a/loolwsd/PROBLEMS +++ b/loolwsd/PROBLEMS @@ -7,22 +7,6 @@ one has no idea how it works. That was hopefully not the case when recursive mutexes were introduced here? But probably it is by now... -- The use of separate "disconnect" messages over the WebSocket - connections is not good. It should be perfectly enough to just close - the WebSocket connection gracefully using the WebSocket mechanism, - i.e. a frame with the CLOSE opcode. Or just tearing down the socket - without a CLOSE frame. The code needs to be prepared to handle these - situations anyway, especially for the socket talking to the - client. For the internal communication in the process tree, - "disconnect" messages are barely acceptable, if the code is already - written to generate and expect them. - - For the moment, we send them from the client too - to distinguish the - browser crash or connection loss from a deliberate shutdown. - We are saving the document in the case we don't get the explicit - 'disconnect'. When we move to actually saving the document all - the time automatically, the 'disconnect' message should be removed. - - Occasionally Control-C (SIGINT) doesn't shut fown loolwsd. One has to kill it with SIGKILL. Which of course leaves all the chroot jails around. diff --git a/loolwsd/protocol.txt b/loolwsd/protocol.txt index c2c0acb..a7e36c6 100644 --- a/loolwsd/protocol.txt +++ b/loolwsd/protocol.txt @@ -17,12 +17,6 @@ canceltiles dropped and will not be handled. There is no guarantee of exactly which tile: messages might still be sent back to the client. -disconnect - - The socket is going to be shut down normally. This is to distinguish - deliberate shutdown from the case when the connection was lost, or - the browser was killed etc. - downloadas name=<fileName> id=<id> format=<document format> options=<SkipImages, etc> Exports the current document to the desired format and returns a download URL @@ -279,11 +273,6 @@ curpart: part=<partNumber> needs to act on in addition to passing them on to the client, like invalidatetiles: -disconnect [reason] - - Sent before disconnecting gracefully. - reason: optional human-readable reason to disconnect. - nextmessage: size=<upperlimit> each tile: message sent from the child to the parent is preceded diff --git a/loolwsd/test/httpwstest.cpp b/loolwsd/test/httpwstest.cpp index 1db58ef..cf015b5 100644 --- a/loolwsd/test/httpwstest.cpp +++ b/loolwsd/test/httpwstest.cpp @@ -216,7 +216,6 @@ void HTTPWSTest::testLoad() } while (n > 0 && (flags & Poco::Net::WebSocket::FRAME_OP_BITMASK) != Poco::Net::WebSocket::FRAME_OP_CLOSE); - sendTextFrame(socket, "disconnect"); socket.shutdown(); Util::removeFile(documentPath); } @@ -268,7 +267,6 @@ void HTTPWSTest::testBadLoad() } while (n > 0 && (flags & Poco::Net::WebSocket::FRAME_OP_BITMASK) != Poco::Net::WebSocket::FRAME_OP_CLOSE); - sendTextFrame(socket, "disconnect"); socket.shutdown(); Util::removeFile(documentPath); } commit db14143d72cd4c2c9284edbf0b2ae853dd57b15d Author: Tor Lillqvist <[email protected]> Date: Wed Apr 13 15:33:56 2016 +0300 Make the loolkit process counting more reliable Sleep in both places before counting them, and catch Poco exceptions while counting (can happen for instance when a /proc entry goes away while we are looking for its 'comm' file). diff --git a/loolwsd/test/httpwstest.cpp b/loolwsd/test/httpwstest.cpp index e064ab2..1db58ef 100644 --- a/loolwsd/test/httpwstest.cpp +++ b/loolwsd/test/httpwstest.cpp @@ -819,9 +819,6 @@ void HTTPWSTest::testImpressPartCountChanged() void HTTPWSTest::testNoExtraLoolKitsLeft() { - // Give polls in the lool processes time to time out - Poco::Thread::sleep(POLL_TIMEOUT_MS*2); - int countNow = countLoolKitProcesses(); CPPUNIT_ASSERT_EQUAL(countNow, _initialLoolKitCount); @@ -933,32 +930,41 @@ void HTTPWSTest::getResponseMessage(Poco::Net::WebSocket& ws, const std::string& int HTTPWSTest::countLoolKitProcesses() { + // Give polls in the lool processes time to time out + Poco::Thread::sleep(POLL_TIMEOUT_MS*3); + int result = 0; for (auto i = Poco::DirectoryIterator(std::string("/proc")); i != Poco::DirectoryIterator(); ++i) { - Poco::Path procEntry = i.path(); - const std::string& fileName = procEntry.getFileName(); - int pid; - std::size_t endPos = 0; try { - pid = std::stoi(fileName, &endPos); - } - catch (const std::invalid_argument&) - { - pid = 0; + Poco::Path procEntry = i.path(); + const std::string& fileName = procEntry.getFileName(); + int pid; + std::size_t endPos = 0; + try + { + pid = std::stoi(fileName, &endPos); + } + catch (const std::invalid_argument&) + { + pid = 0; + } + if (pid > 1 && endPos == fileName.length()) + { + Poco::FileInputStream comm(procEntry.toString() + "/comm"); + std::string command; + Poco::StreamCopier::copyToString(comm, command); + if (command.length() > 0 && command.back() == '\n') + command.pop_back(); + // std::cout << "For process " << pid << " comm is '" << command << "'" << std::endl; + if (command == "loolkit") + result++; + } } - if (pid > 1 && endPos == fileName.length()) + catch (const Poco::Exception&) { - Poco::FileInputStream comm(procEntry.toString() + "/comm"); - std::string command; - Poco::StreamCopier::copyToString(comm, command); - if (command.length() > 0 && command.back() == '\n') - command.pop_back(); - // std::cout << "For process " << pid << " comm is '" << command << "'" << std::endl; - if (command == "loolkit") - result++; } } _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
