[Libreoffice-commits] online.git: loolwsd/Admin.cpp loolwsd/Common.hpp loolwsd/LOOLBroker.cpp loolwsd/LOOLKit.cpp loolwsd/LOOLWSD.cpp loolwsd/LOOLWSD.hpp loolwsd/MasterProcessSession.cpp

Ashod Nakashian ashod.nakashian at collabora.co.uk
Mon Apr 4 04:01:14 UTC 2016


 loolwsd/Admin.cpp                |    2 -
 loolwsd/Common.hpp               |    2 -
 loolwsd/LOOLBroker.cpp           |   58 ++++++++----------------------
 loolwsd/LOOLKit.cpp              |    7 ---
 loolwsd/LOOLWSD.cpp              |   75 ++++++++++++++++-----------------------
 loolwsd/LOOLWSD.hpp              |    2 -
 loolwsd/MasterProcessSession.cpp |    6 +--
 7 files changed, 53 insertions(+), 99 deletions(-)

New commits:
commit ceaefabc912f45da88753f88b27afd816ff3ed8b
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sun Apr 3 23:40:56 2016 -0400

    Revert "loolwsd: replace fifo for socket WSD -> Broker"
    
    This reverts commit 97c8f35ddffe9b363bb2544a40d81f623934a0ed.
    
    Since the Broker design has been extremely simplified,
    all communication between Broker <-> Kit are gone.
    Only a pipe between WSD and Broker remain.
    
    Temporarily reverting this to apply the Broker redesign,
    after which this patch can be reviewed and merged.
    This will be easier than trying to merge the redesigned
    Broker on top of this.
    
    Change-Id: Ia901fad604008654c01841df62e88918adad45e1
    Reviewed-on: https://gerrit.libreoffice.org/23769
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/loolwsd/Admin.cpp b/loolwsd/Admin.cpp
index 800c2bc..2ad3d3d 100644
--- a/loolwsd/Admin.cpp
+++ b/loolwsd/Admin.cpp
@@ -194,7 +194,7 @@ void AdminRequestHandler::handleWSRequests(HTTPServerRequest& request, HTTPServe
                         {
                             if (std::stoi(tokens[1]))
                             {
-                                LOOLWSD::DlgBroker->sendMessage(firstLine);
+                                IoUtil::writeFIFO(LOOLWSD::BrokerWritePipe, firstLine + " \r\n");
                             }
                         }
                         catch(std::exception& e)
diff --git a/loolwsd/Common.hpp b/loolwsd/Common.hpp
index 27279c5..40613e3 100644
--- a/loolwsd/Common.hpp
+++ b/loolwsd/Common.hpp
@@ -18,7 +18,6 @@ constexpr int MAX_SESSIONS = 1024;
 
 constexpr int DEFAULT_CLIENT_PORT_NUMBER = 9980;
 constexpr int MASTER_PORT_NUMBER = 9981;
-constexpr int COMMAND_PORT_NUMBER = 9982;
 constexpr int INTERVAL_PROBES = 10;
 constexpr int MAINTENANCE_INTERVAL = 1;
 constexpr int CHILD_TIMEOUT_SECS = 10;
@@ -35,6 +34,7 @@ constexpr int READ_BUFFER_SIZE = 2048;
 constexpr int SMALL_MESSAGE_SIZE = READ_BUFFER_SIZE / 2;
 
 constexpr auto CHILD_URI = "/loolws/child?";
+constexpr auto FIFO_LOOLWSD = "loolwsdfifo";
 constexpr auto FIFO_PATH = "pipe";
 constexpr auto JAILED_DOCUMENT_ROOT = "/user/docs/";
 constexpr auto SSL_KEY_FILE = "key.pem";
diff --git a/loolwsd/LOOLBroker.cpp b/loolwsd/LOOLBroker.cpp
index ac46e84..d79b032 100644
--- a/loolwsd/LOOLBroker.cpp
+++ b/loolwsd/LOOLBroker.cpp
@@ -35,6 +35,7 @@ const std::string BROKER_SUFIX = ".fifo";
 const std::string BROKER_PREFIX = "lokit";
 
 static int readerChild = -1;
+static int readerBroker = -1;
 
 static std::string loolkitPath;
 static std::atomic<unsigned> forkCounter;
@@ -173,12 +174,11 @@ namespace
     }
 }
 
-class CommandRunnable: public Runnable
+class PipeRunnable: public Runnable
 {
 public:
-    CommandRunnable(const std::shared_ptr<DialogSocket>& dlgWsd) :
-        _childPipeReader("child_pipe_rd", readerChild),
-        _dlgWsd(dlgWsd)
+    PipeRunnable() :
+        _childPipeReader("child_pipe_rd", readerChild)
     {
     }
 
@@ -271,39 +271,21 @@ public:
     void run() override
     {
         static const std::string thread_name = "brk_pipe_reader";
-        const Poco::Timespan waitTime(POLL_TIMEOUT_MS * 1000);
 
         if (prctl(PR_SET_NAME, reinterpret_cast<unsigned long>(thread_name.c_str()), 0, 0, 0) != 0)
             Log::error("Cannot set thread name to " + thread_name + ".");
 
         Log::debug("Thread [" + thread_name + "] started.");
 
-        try
-        {
-            while (!TerminationFlag)
-            {
-                if (_dlgWsd->poll(waitTime, Socket::SELECT_READ))
-                {
-                    std::string message;
-                    _dlgWsd->receiveMessage(message);
-                    handleInput(message);
-                }
-            }
-            _dlgWsd->shutdown();
-        }
-        catch (const Exception& exc)
-        {
-            Log::error() << "CommandRunnable::run: Exception: " << exc.displayText()
-                         << (exc.nested() ? " (" + exc.nested()->displayText() + ")" : "")
-                         << Log::end;
-        }
+        IoUtil::PipeReader pipeReader(FIFO_LOOLWSD, readerBroker);
+        pipeReader.process([this](std::string& message) { handleInput(message); return true; },
+                           []() { return TerminationFlag; });
 
         Log::debug("Thread [" + thread_name + "] finished.");
     }
 
 private:
     IoUtil::PipeReader _childPipeReader;
-    std::shared_ptr<DialogSocket> _dlgWsd;
 };
 
 /// Initializes LibreOfficeKit for cross-fork re-use.
@@ -571,18 +553,11 @@ int main(int argc, char** argv)
     assert(!childRoot.empty());
     assert(numPreSpawnedChildren >= 1);
 
-    std::shared_ptr<DialogSocket> dlgWsd = std::make_shared<DialogSocket>();
-    const Poco::Timespan waitTime(POLL_TIMEOUT_MS * 1000);
-
-    try
-    {
-        dlgWsd->connect(SocketAddress("localhost", COMMAND_PORT_NUMBER), waitTime);
-    }
-    catch (const Exception& exc)
+    const Path pipePath = Path::forDirectory(childRoot + Path::separator() + FIFO_PATH);
+    const std::string pipeLoolwsd = Path(pipePath, FIFO_LOOLWSD).toString();
+    if ( (readerBroker = open(pipeLoolwsd.c_str(), O_RDONLY) ) < 0 )
     {
-        Log::error() << "LOOLBroker::main: Exception: " << exc.displayText()
-                     << (exc.nested() ? " (" + exc.nested()->displayText() + ")" : "")
-                     << Log::end;
+        Log::error("Error: failed to open pipe [" + pipeLoolwsd + "] read only. Exiting.");
         std::exit(Application::EXIT_SOFTWARE);
     }
 
@@ -593,8 +568,6 @@ int main(int argc, char** argv)
         Log::info("Note: LOK_VIEW_CALLBACK is not set.");
 
     int pipeFlags = O_RDONLY | O_NONBLOCK;
-
-    const Path pipePath = Path::forDirectory(childRoot + Path::separator() + FIFO_PATH);
     const std::string pipeBroker = Path(pipePath, FIFO_BROKER).toString();
     if (mkfifo(pipeBroker.c_str(), 0666) < 0 && errno != EEXIST)
     {
@@ -657,10 +630,10 @@ int main(int argc, char** argv)
         dropCapability(CAP_FOWNER);
     }
 
-    CommandRunnable commandHandler(dlgWsd);
-    Poco::Thread commandThread;
+    PipeRunnable pipeHandler;
+    Poco::Thread pipeThread;
 
-    commandThread.start(commandHandler);
+    pipeThread.start(pipeHandler);
 
     Log::info("loolbroker is ready.");
 
@@ -834,9 +807,10 @@ int main(int argc, char** argv)
     _childProcesses.clear();
     _newChildProcesses.clear();
 
-    commandThread.join();
+    pipeThread.join();
     close(writerNotify);
     close(readerChild);
+    close(readerBroker);
 
     Log::info("Process [loolbroker] finished.");
     return Application::EXIT_OK;
diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp
index cb46505..f8699b3 100644
--- a/loolwsd/LOOLKit.cpp
+++ b/loolwsd/LOOLKit.cpp
@@ -34,9 +34,6 @@
 #include <Poco/Net/HTTPResponse.h>
 #include <Poco/Net/NetException.h>
 #include <Poco/Net/WebSocket.h>
-#include <Poco/Net/ServerSocket.h>
-#include <Poco/Net/DialogSocket.h>
-#include <Poco/Net/SocketAddress.h>
 #include <Poco/Process.h>
 #include <Poco/Runnable.h>
 #include <Poco/StringTokenizer.h>
@@ -65,11 +62,7 @@ using Poco::File;
 using Poco::Net::HTTPClientSession;
 using Poco::Net::HTTPRequest;
 using Poco::Net::HTTPResponse;
-using Poco::Net::Socket;
 using Poco::Net::WebSocket;
-using Poco::Net::ServerSocket;
-using Poco::Net::DialogSocket;
-using Poco::Net::SocketAddress;
 using Poco::Path;
 using Poco::Process;
 using Poco::Runnable;
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 5fe1bd4..1dd6eef 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -94,7 +94,6 @@ DEALINGS IN THE SOFTWARE.
 #include <Poco/Net/SecureServerSocket.h>
 #include <Poco/Net/ServerSocket.h>
 #include <Poco/Net/SocketAddress.h>
-#include <Poco/Net/DialogSocket.h>
 #include <Poco/Net/WebSocket.h>
 #include <Poco/Path.h>
 #include <Poco/Process.h>
@@ -148,7 +147,6 @@ using Poco::Net::SecureServerSocket;
 using Poco::Net::ServerSocket;
 using Poco::Net::Socket;
 using Poco::Net::SocketAddress;
-using Poco::Net::DialogSocket;
 using Poco::Net::WebSocket;
 using Poco::Net::WebSocketException;
 using Poco::Path;
@@ -466,9 +464,9 @@ private:
             session->setEditLock(true);
 
         // Request a kit process for this doc.
-        const std::string message = "request " + id + " " + docKey;
-        Log::debug("MasterToBroker: " + message);
-        LOOLWSD::DlgBroker->sendMessage(message);
+        const std::string aMessage = "request " + id + " " + docKey + "\n";
+        Log::debug("MasterToBroker: " + aMessage.substr(0, aMessage.length() - 1));
+        IoUtil::writeFIFO(LOOLWSD::BrokerWritePipe, aMessage);
 
         QueueHandler handler(queue, session, "wsd_queue_" + session->getId());
 
@@ -881,7 +879,7 @@ private:
 };
 
 std::atomic<unsigned> LOOLWSD::NextSessionId;
-std::unique_ptr<DialogSocket> LOOLWSD::DlgBroker;
+int LOOLWSD::BrokerWritePipe = -1;
 std::string LOOLWSD::Cache = LOOLWSD_CACHEDIR;
 std::string LOOLWSD::SysTemplate;
 std::string LOOLWSD::LoTemplate;
@@ -1082,47 +1080,23 @@ void LOOLWSD::displayVersion()
 
 Process::PID LOOLWSD::createBroker()
 {
-    Process::PID brokerPid = -1;
+    Process::Args args;
 
-    try
-    {
-        Process::Args args;
-
-        args.push_back("--losubpath=" + LOOLWSD::LoSubPath);
-        args.push_back("--systemplate=" + SysTemplate);
-        args.push_back("--lotemplate=" + LoTemplate);
-        args.push_back("--childroot=" + ChildRoot);
-        args.push_back("--numprespawns=" + std::to_string(NumPreSpawnedChildren));
-        args.push_back("--clientport=" + std::to_string(ClientPortNumber));
-
-        const std::string brokerPath = Path(Application::instance().commandPath()).parent().toString() + "loolbroker";
-        const Poco::Timespan waitTime(POLL_TIMEOUT_MS * 1000);
+    args.push_back("--losubpath=" + LOOLWSD::LoSubPath);
+    args.push_back("--systemplate=" + SysTemplate);
+    args.push_back("--lotemplate=" + LoTemplate);
+    args.push_back("--childroot=" + ChildRoot);
+    args.push_back("--numprespawns=" + std::to_string(NumPreSpawnedChildren));
+    args.push_back("--clientport=" + std::to_string(ClientPortNumber));
 
-        Log::info("Launching Broker #1: " + brokerPath + " " +
-                  Poco::cat(std::string(" "), args.begin(), args.end()));
+    const std::string brokerPath = Path(Application::instance().commandPath()).parent().toString() + "loolbroker";
 
-        ServerSocket srvCommand(COMMAND_PORT_NUMBER);
-        ProcessHandle child = Process::launch(brokerPath, args);
+    Log::info("Launching Broker #1: " + brokerPath + " " +
+              Poco::cat(std::string(" "), args.begin(), args.end()));
 
-        if (srvCommand.poll(waitTime, Socket::SELECT_READ))
-        {
-            DlgBroker.reset(new DialogSocket(srvCommand.acceptConnection()));
-            brokerPid = child.id();
-        }
-        else
-        {
-            Log::error("Error: failed to socket connection with broker.");
-            Util::requestTermination(child.id());
-        }
-    }
-    catch (const Exception& exc)
-    {
-        Log::error() << "LOOLWSD::createBroker: Exception: " << exc.displayText()
-                     << (exc.nested() ? " (" + exc.nested()->displayText() + ")" : "")
-                     << Log::end;
-    }
+    ProcessHandle child = Process::launch(brokerPath, args);
 
-    return brokerPid;
+    return child.id();
 }
 
 int LOOLWSD::main(const std::vector<std::string>& /*args*/)
@@ -1188,6 +1162,13 @@ int LOOLWSD::main(const std::vector<std::string>& /*args*/)
         return Application::EXIT_SOFTWARE;
     }
 
+    const std::string pipeLoolwsd = Path(pipePath, FIFO_LOOLWSD).toString();
+    if (mkfifo(pipeLoolwsd.c_str(), 0666) < 0 && errno != EEXIST)
+    {
+        Log::error("Error: Failed to create pipe FIFO [" + pipeLoolwsd + "].");
+        return Application::EXIT_SOFTWARE;
+    }
+
     // Open notify pipe
     int pipeFlags = O_RDONLY | O_NONBLOCK;
     int notifyPipe = -1;
@@ -1256,6 +1237,12 @@ int LOOLWSD::main(const std::vector<std::string>& /*args*/)
 
     srv2.start();
 
+    if ( (BrokerWritePipe = open(pipeLoolwsd.c_str(), O_WRONLY) ) < 0 )
+    {
+        Log::error("Error: failed to open pipe [" + pipeLoolwsd + "] write only.");
+        return Application::EXIT_SOFTWARE;
+    }
+
     threadPool.start(admin);
 
     TestInput input(*this, svs, srv);
@@ -1393,13 +1380,15 @@ int LOOLWSD::main(const std::vector<std::string>& /*args*/)
     threadPool.joinAll();
 
     // Terminate child processes
-    DlgBroker->shutdown();
+    IoUtil::writeFIFO(LOOLWSD::BrokerWritePipe, "eof\n");
     Log::info("Requesting child process " + std::to_string(brokerPid) + " to terminate");
     Util::requestTermination(brokerPid);
 
     // wait broker process finish
     waitpid(brokerPid, &status, WUNTRACED);
 
+    close(BrokerWritePipe);
+
     Log::info("Cleaning up childroot directory [" + ChildRoot + "].");
     std::vector<std::string> jails;
     File(ChildRoot).list(jails);
diff --git a/loolwsd/LOOLWSD.hpp b/loolwsd/LOOLWSD.hpp
index 9d5e5d3..59d7111 100644
--- a/loolwsd/LOOLWSD.hpp
+++ b/loolwsd/LOOLWSD.hpp
@@ -19,7 +19,6 @@
 #include <Poco/Random.h>
 #include <Poco/Util/OptionSet.h>
 #include <Poco/Util/ServerApplication.h>
-#include <Poco/Net/DialogSocket.h>
 
 #include "Auth.hpp"
 #include "Common.hpp"
@@ -37,7 +36,6 @@ public:
     // An Application is a singleton anyway,
     // so just keep these as statics.
     static std::atomic<unsigned> NextSessionId;
-    static std::unique_ptr<Poco::Net::DialogSocket> DlgBroker;
     static int NumPreSpawnedChildren;
     static int BrokerWritePipe;
     static bool DoTest;
diff --git a/loolwsd/MasterProcessSession.cpp b/loolwsd/MasterProcessSession.cpp
index f8a21b6..432e751 100644
--- a/loolwsd/MasterProcessSession.cpp
+++ b/loolwsd/MasterProcessSession.cpp
@@ -782,9 +782,9 @@ void MasterProcessSession::dispatchChild()
         {
             Log::info() << "Retrying child permission... " << retries << Log::end;
             // request again new URL session
-            const std::string message = "request " + getId() + " " + _docBroker->getDocKey();
-            Log::trace("MasterToBroker: " + message);
-            LOOLWSD::DlgBroker->sendMessage(message);
+            const std::string message = "request " + getId() + " " + _docBroker->getDocKey() + '\n';
+            Log::trace("MasterToBroker: " + message.substr(0, message.length()-1));
+            IoUtil::writeFIFO(LOOLWSD::BrokerWritePipe, message);
         }
     }
 


More information about the Libreoffice-commits mailing list