[Libreoffice-commits] online.git: 9 commits - loolwsd/Connect.cpp loolwsd/LOOLBroker.cpp loolwsd/LOOLKit.cpp loolwsd/LOOLWSD.cpp loolwsd/MasterProcessSession.cpp loolwsd/MasterProcessSession.hpp loolwsd/MessageQueue.hpp loolwsd/QueueHandler.hpp loolwsd/TileCache.cpp

Tor Lillqvist tml at collabora.com
Fri Apr 1 16:02:08 UTC 2016


 loolwsd/Connect.cpp              |   20 ++++++++++---
 loolwsd/LOOLBroker.cpp           |   56 ---------------------------------------
 loolwsd/LOOLKit.cpp              |   20 ++++---------
 loolwsd/LOOLWSD.cpp              |   28 +++++++++----------
 loolwsd/MasterProcessSession.cpp |    6 ++--
 loolwsd/MasterProcessSession.hpp |    7 ++++
 loolwsd/MessageQueue.hpp         |   12 ++++----
 loolwsd/QueueHandler.hpp         |    7 ++--
 loolwsd/TileCache.cpp            |   26 ++++++++++++------
 9 files changed, 73 insertions(+), 109 deletions(-)

New commits:
commit 7a80ca8aa174ef4709a9908c2f6514a1426a4952
Author: Tor Lillqvist <tml at collabora.com>
Date:   Fri Apr 1 18:56:08 2016 +0300

    Make use of 'using' consistent in this file

diff --git a/loolwsd/TileCache.cpp b/loolwsd/TileCache.cpp
index bf23cd9..3e9e408 100644
--- a/loolwsd/TileCache.cpp
+++ b/loolwsd/TileCache.cpp
@@ -32,24 +32,23 @@
 #include "TileCache.hpp"
 #include "Util.hpp"
 
-using Poco::DigestEngine;
 using Poco::DirectoryIterator;
 using Poco::File;
 using Poco::FileException;
+using Poco::Path;
 using Poco::StringTokenizer;
-using Poco::SyntaxException;
 using Poco::Timestamp;
 using Poco::URI;
 
 using namespace LOOLProtocol;
 
 TileCache::TileCache(const std::string& docURL,
-                     const Poco::Timestamp& modifiedTime,
+                     const Timestamp& modifiedTime,
                      const std::string& rootCacheDir) :
     _docURL(docURL),
     _rootCacheDir(rootCacheDir),
-    _persCacheDir(Poco::Path(rootCacheDir, "persistent").toString()),
-    _editCacheDir(Poco::Path(rootCacheDir, "editing").toString()),
+    _persCacheDir(Path(rootCacheDir, "persistent").toString()),
+    _editCacheDir(Path(rootCacheDir, "editing").toString()),
     _isEditing(false),
     _hasUnsavedChanges(false)
 {
@@ -394,7 +393,7 @@ Timestamp TileCache::getLastModified()
     return result;
 }
 
-void TileCache::saveLastModified(const Poco::Timestamp& timestamp)
+void TileCache::saveLastModified(const Timestamp& timestamp)
 {
     std::fstream modTimeFile(_rootCacheDir + "/modtime.txt", std::ios::out);
     modTimeFile << timestamp.raw() << std::endl;
commit 9a874e7cda195a155d14e84765d20b265391f48c
Author: Tor Lillqvist <tml at collabora.com>
Date:   Fri Apr 1 18:52:17 2016 +0300

    Catch and ignore FileExceptions when persisting editing tiles
    
    There is nothing that says a client has even requested any tiles, so
    there might be none to persist. Don't let an exception thrown by the
    DirectoryIterator propagate upwards and cause potential
    issues.
    
    Noticed the issue when testing using the 'connect' test program,
    giving it input that did not request any tiles.

diff --git a/loolwsd/TileCache.cpp b/loolwsd/TileCache.cpp
index 988e9c2..bf23cd9 100644
--- a/loolwsd/TileCache.cpp
+++ b/loolwsd/TileCache.cpp
@@ -35,6 +35,7 @@
 using Poco::DigestEngine;
 using Poco::DirectoryIterator;
 using Poco::File;
+using Poco::FileException;
 using Poco::StringTokenizer;
 using Poco::SyntaxException;
 using Poco::Timestamp;
@@ -192,9 +193,19 @@ void TileCache::documentSaved()
 
     _cacheMutex.lock();
     // then move the new tiles from the Editing cache to Persistent
-    for (auto tileIterator = DirectoryIterator(_editCacheDir); tileIterator != DirectoryIterator(); ++tileIterator)
+    try
     {
-        tileIterator->moveTo(_persCacheDir);
+        for (auto tileIterator = DirectoryIterator(_editCacheDir); tileIterator != DirectoryIterator(); ++tileIterator)
+        {
+            tileIterator->moveTo(_persCacheDir);
+        }
+    }
+    catch (const FileException& exc)
+    {
+        // Just log this exception, ignore it otherwise
+        Log::error() << "TileCache::documentSaved: Exception: " << exc.displayText()
+                     << (exc.nested() ? " (" + exc.nested()->displayText() + ")" : "")
+                     << Log::end;
     }
 
     _cacheMutex.unlock();
commit 593d5d7e0820575171451fbbec05176c36c51fb2
Author: Tor Lillqvist <tml at collabora.com>
Date:   Fri Apr 1 18:33:08 2016 +0300

    Actually propagate idle/auto save requests to the kit processes
    
    Had to add a shared pointer to the BasicTileQueue for the session to
    the MasterProcessSession object, and restructure the coe a a bit to
    allocate BasicTileQueue objects dynamically. Possibly just passing a
    reference to a BasicTileQueue in the stack would have worked, but why
    risk it?
    
    The actual logic when to do auto / idle save is not quite right still,
    did not change that in this commit.

diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp
index 7599128..f8699b3 100644
--- a/loolwsd/LOOLKit.cpp
+++ b/loolwsd/LOOLKit.cpp
@@ -190,16 +190,16 @@ public:
         _thread.join();
     }
 
-    void handle(TileQueue& queue, const std::string& firstLine, char* buffer, int n)
+    void handle(std::shared_ptr<TileQueue> queue, const std::string& firstLine, char* buffer, int n)
     {
         if (firstLine.find("paste") != 0)
         {
             // Everything else is expected to be a single line.
             assert(firstLine.size() == static_cast<std::string::size_type>(n));
-            queue.put(firstLine);
+            queue->put(firstLine);
         }
         else
-            queue.put(std::string(buffer, n));
+            queue->put(std::string(buffer, n));
     }
 
     void run() override
@@ -213,7 +213,7 @@ public:
 
         try
         {
-            TileQueue queue;
+            auto queue = std::make_shared<TileQueue>();
             QueueHandler handler(queue, _session, "kit_queue_" + _session->getId());
 
             Thread queueHandlerThread;
@@ -264,8 +264,8 @@ public:
                          << ", payload size: " << n
                          << ", flags: " << std::hex << flags << Log::end;
 
-            queue.clear();
-            queue.put("eof");
+            queue->clear();
+            queue->put("eof");
             queueHandlerThread.join();
 
             _session->disconnect();
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 8174add..1dd6eef 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -250,8 +250,7 @@ private:
 
                     // Load the document.
                     std::shared_ptr<WebSocket> ws;
-                    const LOOLSession::Kind kind = LOOLSession::Kind::ToClient;
-                    auto session = std::make_shared<MasterProcessSession>(id, kind, ws, docBroker);
+                    auto session = std::make_shared<MasterProcessSession>(id, LOOLSession::Kind::ToClient, ws, docBroker, nullptr);
                     session->setEditLock(true);
                     docBroker->incSessions();
                     lock.unlock();
@@ -448,8 +447,13 @@ private:
         docBroker->validate(uriPublic);
         Log::debug("Validated [" + uriPublic.toString() + "].");
 
+        // For ToClient sessions, we store incoming messages in a queue and have a separate
+        // thread that handles them. This is so that we can empty the queue when we get a
+        // "canceltiles" message.
+        auto queue = std::make_shared<BasicTileQueue>();
+
         auto ws = std::make_shared<WebSocket>(request, response);
-        auto session = std::make_shared<MasterProcessSession>(id, LOOLSession::Kind::ToClient, ws, docBroker);
+        auto session = std::make_shared<MasterProcessSession>(id, LOOLSession::Kind::ToClient, ws, docBroker, queue);
         docBroker->incSessions();
         docBrokersLock.unlock();
 
@@ -464,10 +468,6 @@ private:
         Log::debug("MasterToBroker: " + aMessage.substr(0, aMessage.length() - 1));
         IoUtil::writeFIFO(LOOLWSD::BrokerWritePipe, aMessage);
 
-        // For ToClient sessions, we store incoming messages in a queue and have a separate
-        // thread that handles them. This is so that we can empty the queue when we get a
-        // "canceltiles" message.
-        BasicTileQueue queue;
         QueueHandler handler(queue, session, "wsd_queue_" + session->getId());
 
         Thread queueHandlerThread;
@@ -485,7 +485,7 @@ private:
                 }
                 else
                 {
-                    queue.put(payload);
+                    queue->put(payload);
                 }
 
                 return true;
@@ -500,12 +500,12 @@ private:
             // of save so Storage can persist the save (if necessary).
             // In addition, we shouldn't issue save when opening of the doc fails.
             Log::info("Non-deliberate shutdown of the last session, saving the document before tearing down.");
-            queue.put("uno .uno:Save");
+            queue->put("uno .uno:Save");
         }
         else
         {
             Log::info("Clearing the queue.");
-            queue.clear();
+            queue->clear();
         }
 
         docBroker->removeWSSession(id);
@@ -513,7 +513,7 @@ private:
         Log::warn(docKey + ", ws_sessions--: " + std::to_string(wsSessionsCount));
 
         Log::info("Finishing GET request handler for session [" + id + "]. Joining the queue.");
-        queue.put("eof");
+        queue->put("eof");
         queueHandlerThread.join();
 
         docBrokersLock.lock();
@@ -678,7 +678,7 @@ public:
             docBroker->load(jailId);
 
             auto ws = std::make_shared<WebSocket>(request, response);
-            auto session = std::make_shared<MasterProcessSession>(sessionId, LOOLSession::Kind::ToPrisoner, ws, docBroker);
+            auto session = std::make_shared<MasterProcessSession>(sessionId, LOOLSession::Kind::ToPrisoner, ws, docBroker, nullptr);
 
             std::unique_lock<std::mutex> lock(MasterProcessSession::AvailableChildSessionMutex);
             MasterProcessSession::AvailableChildSessions.emplace(sessionId, session);
@@ -1334,8 +1334,8 @@ int LOOLWSD::main(const std::vector<std::string>& /*args*/)
                         if (sessionIt.second->_lastMessageTime > sessionIt.second->_idleSaveTime &&
                             sessionIt.second->_lastMessageTime < now - 30)
                         {
-                            // Trigger a .uno:Save
                             Log::info("Idle save triggered for session " + sessionIt.second->getId());
+                            sessionIt.second->getQueue()->put("uno .uno:Save");
 
                             sessionIt.second->_idleSaveTime = now;
                         }
@@ -1356,8 +1356,8 @@ int LOOLWSD::main(const std::vector<std::string>& /*args*/)
                         if (sessionIt.second->_lastMessageTime >= sessionIt.second->_idleSaveTime &&
                             sessionIt.second->_lastMessageTime >= sessionIt.second->_autoSaveTime)
                         {
-                            // Trigger a .uno:Save
                             Log::info("Auto-save triggered for session " + sessionIt.second->getId());
+                            sessionIt.second->getQueue()->put("uno .uno:Save");
 
                             sessionIt.second->_autoSaveTime = now;
                         }
diff --git a/loolwsd/MasterProcessSession.cpp b/loolwsd/MasterProcessSession.cpp
index a494c5e..432e751 100644
--- a/loolwsd/MasterProcessSession.cpp
+++ b/loolwsd/MasterProcessSession.cpp
@@ -36,14 +36,16 @@ std::condition_variable MasterProcessSession::AvailableChildSessionCV;
 MasterProcessSession::MasterProcessSession(const std::string& id,
                                            const Kind kind,
                                            std::shared_ptr<Poco::Net::WebSocket> ws,
-                                           std::shared_ptr<DocumentBroker> docBroker) :
+                                           std::shared_ptr<DocumentBroker> docBroker,
+                                           std::shared_ptr<BasicTileQueue> queue) :
     LOOLSession(id, kind, ws),
     _lastMessageTime(0),
     _idleSaveTime(0),
     _autoSaveTime(0),
     _curPart(0),
     _loadPart(-1),
-    _docBroker(docBroker)
+    _docBroker(docBroker),
+    _queue(queue)
 {
     Log::info("MasterProcessSession ctor [" + getName() + "].");
 }
diff --git a/loolwsd/MasterProcessSession.hpp b/loolwsd/MasterProcessSession.hpp
index 7b57709..7838fd4 100644
--- a/loolwsd/MasterProcessSession.hpp
+++ b/loolwsd/MasterProcessSession.hpp
@@ -15,6 +15,7 @@
 #include <Poco/Random.h>
 
 #include "LOOLSession.hpp"
+#include "MessageQueue.hpp"
 #include "TileCache.hpp"
 
 class DocumentBroker;
@@ -25,7 +26,8 @@ class MasterProcessSession final : public LOOLSession, public std::enable_shared
     MasterProcessSession(const std::string& id,
                          const Kind kind,
                          std::shared_ptr<Poco::Net::WebSocket> ws,
-                         std::shared_ptr<DocumentBroker> docBroker);
+                         std::shared_ptr<DocumentBroker> docBroker,
+                         std::shared_ptr<BasicTileQueue> queue);
     virtual ~MasterProcessSession();
 
     virtual bool getStatus(const char *buffer, int length) override;
@@ -45,6 +47,8 @@ class MasterProcessSession final : public LOOLSession, public std::enable_shared
 
     std::shared_ptr<DocumentBroker> getDocumentBroker() const { return _docBroker; }
 
+    std::shared_ptr<BasicTileQueue> getQueue() const { return _queue; }
+
     void setEditLock(const bool value) { _bEditLock = value; }
 
     bool isEditLocked() const { return _bEditLock; }
@@ -94,6 +98,7 @@ public:
     /// Kind::ToClient instances store URLs of completed 'save as' documents.
     MessageQueue _saveAsQueue;
     std::shared_ptr<DocumentBroker> _docBroker;
+    std::shared_ptr<BasicTileQueue> _queue;
 
     // If this document holds the edit lock.
     // An edit lock will only allow the current session to make edits,
diff --git a/loolwsd/QueueHandler.hpp b/loolwsd/QueueHandler.hpp
index 4d2fd18..dbe7ac1 100644
--- a/loolwsd/QueueHandler.hpp
+++ b/loolwsd/QueueHandler.hpp
@@ -18,7 +18,8 @@
 class QueueHandler: public Poco::Runnable
 {
 public:
-    QueueHandler(MessageQueue& queue, const std::shared_ptr<LOOLSession>& session,
+    QueueHandler(std::shared_ptr<MessageQueue> queue,
+                 const std::shared_ptr<LOOLSession>& session,
                  const std::string& name):
         _queue(queue),
         _session(session),
@@ -37,7 +38,7 @@ public:
         {
             while (true)
             {
-                const auto input = _queue.get();
+                const auto input = _queue->get();
                 if (LOOLProtocol::getFirstToken(input) == "eof")
                 {
                     Log::info("Received EOF. Finishing.");
@@ -60,7 +61,7 @@ public:
     }
 
 private:
-    MessageQueue& _queue;
+    std::shared_ptr<MessageQueue> _queue;
     std::shared_ptr<LOOLSession> _session;
     const std::string _name;
 };
commit d379ed21132ec553b2fe572cb5e94e42d6612693
Author: Tor Lillqvist <tml at collabora.com>
Date:   Fri Apr 1 17:07:33 2016 +0300

    Improve logging and orderly shutdown

diff --git a/loolwsd/Connect.cpp b/loolwsd/Connect.cpp
index d53ae43..e5f1c51 100644
--- a/loolwsd/Connect.cpp
+++ b/loolwsd/Connect.cpp
@@ -102,10 +102,11 @@ public:
                 }
             }
             while (n > 0 && (flags & WebSocket::FRAME_OP_BITMASK) != WebSocket::FRAME_OP_CLOSE);
+            std::cout << "Websocket closed" << std::endl;
         }
         catch (WebSocketException& exc)
         {
-            _ws.close();
+            std::cout << "Got exception " << exc.message() << std::endl;
         }
     }
 
@@ -155,21 +156,30 @@ protected:
         Output output(ws);
         thread.start(output);
 
-        while (!std::cin.eof())
+        while (true)
         {
             std::string line;
             std::getline(std::cin, line);
-            // Accept an input line "sleep <n>" that makes us sleep a number of seconds. Useful for
-            // debugging. Interrupt with Control-C.
-            if (line.find("sleep ") == 0)
+            if (std::cin.eof())
             {
+                break;
+            }
+            else if (line.find("sleep ") == 0)
+            {
+                // Accept an input line "sleep <n>" that makes us sleep a number of seconds.
                 long sleepTime = std::stol(line.substr(std::string("sleep").length()));
+                std::cout << "Sleeping " << sleepTime << " seconds" << std::endl;
                 Thread::sleep(sleepTime * 1000);
             }
             else
+            {
+                std::cout << "Sending: '" << line << "'" << std::endl;
                 ws.sendFrame(line.c_str(), line.size());
+            }
         }
 
+        std::cout << "Shutting down websocket" << std::endl;
+        ws.shutdown();
         thread.join();
 
         return Application::EXIT_OK;
commit 9e0a8cc43f5fa6db39dc965d6402bbee4dcbe64e
Author: Tor Lillqvist <tml at collabora.com>
Date:   Fri Apr 1 15:33:02 2016 +0300

    These are not overridden in derived classes, no need to be virtual

diff --git a/loolwsd/MessageQueue.hpp b/loolwsd/MessageQueue.hpp
index 1dd91e1..9e39a2a 100644
--- a/loolwsd/MessageQueue.hpp
+++ b/loolwsd/MessageQueue.hpp
@@ -55,11 +55,11 @@ private:
 protected:
     virtual void put_impl(const Payload& value);
 
-    virtual bool wait_impl() const;
+    bool wait_impl() const;
 
-    virtual Payload get_impl();
+    Payload get_impl();
 
-    virtual void clear_impl();
+    void clear_impl();
 
     std::deque<Payload> _queue;
 };
commit e3f8fc41a182c7b3c00bc0f3cf053e679d56c26b
Author: Tor Lillqvist <tml at collabora.com>
Date:   Fri Apr 1 15:31:26 2016 +0300

    Add 'override'

diff --git a/loolwsd/MessageQueue.hpp b/loolwsd/MessageQueue.hpp
index 6d2c32e..1dd91e1 100644
--- a/loolwsd/MessageQueue.hpp
+++ b/loolwsd/MessageQueue.hpp
@@ -72,7 +72,7 @@ gets a "canceltiles" command.
 class BasicTileQueue : public MessageQueue
 {
 protected:
-    virtual void put_impl(const Payload& value);
+    virtual void put_impl(const Payload& value) override;
 };
 
 /** MessageQueue specialized for priority handling of tiles.
@@ -86,7 +86,7 @@ that the ones closest to the cursor position are returned first.
 class TileQueue : public BasicTileQueue
 {
 protected:
-    virtual void put_impl(const Payload& value);
+    virtual void put_impl(const Payload& value) override;
 };
 
 #endif
commit 6c78a8f633c28f83f93bab80a683c20334f664f7
Author: Tor Lillqvist <tml at collabora.com>
Date:   Fri Apr 1 15:29:43 2016 +0300

    Typo

diff --git a/loolwsd/MessageQueue.hpp b/loolwsd/MessageQueue.hpp
index 5a0c439..6d2c32e 100644
--- a/loolwsd/MessageQueue.hpp
+++ b/loolwsd/MessageQueue.hpp
@@ -77,7 +77,7 @@ protected:
 
 /** MessageQueue specialized for priority handling of tiles.
 
-This class builds on BasicTileQueuee, and additonaly provides de-duplication
+This class builds on BasicTileQueue, and additonaly provides de-duplication
 of tile requests.
 
 TODO: we'll need to add reordering of the tiles at some stage here too - so
commit 14be412f8f6ea75bfc27862081de52fad06f6aa4
Author: Tor Lillqvist <tml at collabora.com>
Date:   Fri Apr 1 14:04:08 2016 +0300

    It was PipeRunnable::syncChildren() that wrote "query url" messages to the pipe

diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp
index b10a08f..7599128 100644
--- a/loolwsd/LOOLKit.cpp
+++ b/loolwsd/LOOLKit.cpp
@@ -1045,14 +1045,6 @@ void lokit_main(const std::string& childRoot,
                         TerminationFlag = true;
                         response += "down\n";
                     }
-                    else if (tokens[0] == "query" && tokens.count() > 1)
-                    {
-                        if (tokens[1] == "url")
-                        {
-                            response += (document ? document->getUrl() : "empty");
-                            response += "\n";
-                        }
-                    }
                     else
                     {
                         response += "bad unknown token [" + tokens[0] + "]\n";
commit a20344b52011a757517caab2421bcd9f360d8fdb
Author: Tor Lillqvist <tml at collabora.com>
Date:   Fri Apr 1 14:01:24 2016 +0300

    Bin PipeRunnable::syncChildren() which died in cbabd6177d0e8aaaf77053d4ba17b069fc6dd4da

diff --git a/loolwsd/LOOLBroker.cpp b/loolwsd/LOOLBroker.cpp
index 49a9e1f..d79b032 100644
--- a/loolwsd/LOOLBroker.cpp
+++ b/loolwsd/LOOLBroker.cpp
@@ -213,61 +213,6 @@ public:
         }
     }
 
-    /// Sync ChildProcess instances with its child.
-    /// Returns the number of empty children.
-    size_t syncChildren()
-    {
-        Log::trace("Synching children.");
-        size_t empty_count = 0;
-        for (auto it = _childProcesses.begin(); it != _childProcesses.end(); )
-        {
-            const auto message = "query url\n";
-            std::string response;
-            if (IoUtil::writeFIFO(it->second->getWritePipe(), message) < 0 ||
-                _childPipeReader.readLine(response, [](){ return TerminationFlag; }) < 0)
-            {
-                auto log = Log::error();
-                log << "Error querying child [" << std::to_string(it->second->getPid()) << "].";
-                if (it->second->getUrl().empty())
-                {
-                    log << " Removing empty child." << Log::end;
-                    it = _childProcesses.erase(it);
-                }
-                else
-                {
-                    ++it;
-                }
-                continue;
-            }
-
-            StringTokenizer tokens(response, " ", StringTokenizer::TOK_IGNORE_EMPTY | StringTokenizer::TOK_TRIM);
-            if (tokens.count() >= 2 && tokens[0] == std::to_string(it->second->getPid()))
-            {
-                Log::debug("Child [" + std::to_string(it->second->getPid()) + "] hosts [" + tokens[1] + "].");
-                if (tokens[1] == "empty")
-                {
-                    it->second->setUrl("");
-                    ++empty_count;
-                }
-                else
-                {
-                    it->second->setUrl(tokens[1]);
-                }
-            }
-            else
-            {
-                Log::error("Unexpected response from child [" + std::to_string(it->second->getPid()) +
-                           "] to url query: [" + response + "].");
-            }
-
-            ++it;
-        }
-
-        Log::trace("Synching children done.");
-
-        return empty_count;
-    }
-
     void handleInput(const std::string& message)
     {
         Log::info("Broker command: [" + message + "].");
@@ -751,7 +696,6 @@ int main(int argc, char** argv)
                 Util::removeFile(childPath, true);
             }
 
-            //pipeHandler.syncChildren();
             timeoutCounter = 0;
         }
         else if (pid < 0)


More information about the Libreoffice-commits mailing list