[Libreoffice-commits] online.git: loolwsd/DocumentBroker.cpp loolwsd/test loolwsd/Unit.hpp

[Ashod Nakashian]

 loolwsd/DocumentBroker.cpp   |    6 ++++
 loolwsd/Unit.hpp             |    4 ++
 loolwsd/test/UnitPrefork.cpp |   61 +++++++++++++++++++++++++------------------
 3 files changed, 46 insertions(+), 25 deletions(-)

New commits:
commit 958f6ffcbdbeae39b78acfb7592096851388fb52
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Fri May 20 22:31:53 2016 -0400

    loolwsd: fix UnitPrefork deadlock/corruption
    
    Reading from the socket in the test is not
    thread-safe, and was causing all sorts of
    problems.
    
    The new code adds a test API and reads the
    incoming data through it and not directly
    from the socket. In addition, the read is
    synchronized.
    
    Change-Id: Id13821a40a59e32fd8a14f733a47306aee42ada8
    Reviewed-on: https://gerrit.libreoffice.org/25244
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/loolwsd/DocumentBroker.cpp b/loolwsd/DocumentBroker.cpp
index 3bb2bcc..de4d984 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -20,6 +20,7 @@
 #include "LOOLProtocol.hpp"
 #include "ClientSession.hpp"
 #include "PrisonerSession.hpp"
+#include "Unit.hpp"
 
 using namespace LOOLProtocol;
 
@@ -28,6 +29,11 @@ void ChildProcess::socketProcessor()
     IoUtil::SocketProcessor(_ws,
         [this](const std::vector<char>& payload)
         {
+            if (UnitWSD::get().filterChildMessage(payload))
+            {
+                return true;
+            }
+
             auto docBroker = this->_docBroker.lock();
             if (docBroker)
             {
diff --git a/loolwsd/Unit.hpp b/loolwsd/Unit.hpp
index dcdd36c..9cebfaa 100644
--- a/loolwsd/Unit.hpp
+++ b/loolwsd/Unit.hpp
@@ -134,6 +134,10 @@ public:
                      Poco::Net::HTTPServerResponse& /* response */)
         { return false; }
 
+    /// Child sent a message
+    virtual bool filterChildMessage(const std::vector<char>& /* payload */)
+        { return false; }
+
     // ---------------- TileCache hooks ----------------
     /// Called before the lookupTile call returns. Should always be called to fire events.
     virtual void lookupTile(int part, int width, int height, int tilePosX, int tilePosY,
diff --git a/loolwsd/test/UnitPrefork.cpp b/loolwsd/test/UnitPrefork.cpp
index c265831..f9437bb 100644
--- a/loolwsd/test/UnitPrefork.cpp
+++ b/loolwsd/test/UnitPrefork.cpp
@@ -14,6 +14,9 @@
 #include <sys/types.h>
 #include <dirent.h>
 
+#include <mutex>
+#include <condition_variable>
+
 #include "Common.hpp"
 #include "IoUtil.hpp"
 #include "LOOLProtocol.hpp"
@@ -32,11 +35,17 @@ class UnitPrefork : public UnitWSD
     int _numStarted;
     std::string _failure;
     Poco::Timestamp _startTime;
+    size_t _totalPSS;
+    size_t _totalDirty;
+    std::mutex _mutex;
+    std::condition_variable _cv;
     std::vector< std::shared_ptr<Poco::Net::WebSocket> > _childSockets;
 
 public:
     UnitPrefork()
-        : _numStarted(0)
+        : _numStarted(0),
+          _totalPSS(0),
+          _totalDirty(0)
     {
         setHasKitHooks();
     }
@@ -52,29 +61,9 @@ public:
         numPrefork = NumToPrefork;
     }
 
-    void getMemory(const std::shared_ptr<Poco::Net::WebSocket> &socket,
-                   size_t &totalPSS, size_t &totalDirty)
+    virtual bool filterChildMessage(const std::vector<char>& payload)
     {
-        /// Fetch memory usage data from the last process ...
-        socket->sendFrame("unit-memdump: \n", sizeof("unit-memdump: \n")-1);
-
-        static const Poco::Timespan waitTime(COMMAND_TIMEOUT_MS * 1000);
-        if (!socket->poll(waitTime, Poco::Net::Socket::SELECT_READ))
-        {
-            _failure = "Timed out waiting for child to respond to unit-memdump command.";
-            return;
-        }
-
-        int flags;
-        char buffer[4096];
-        const int length = IoUtil::receiveFrame(*socket, buffer, sizeof (buffer), flags);
-        if (length <= 0 || ((flags & Poco::Net::WebSocket::FRAME_OP_BITMASK) == Poco::Net::WebSocket::FRAME_OP_CLOSE))
-        {
-            _failure = "Failed to read child response to unit-memdump command.";
-            return;
-        }
-
-        const std::string memory = LOOLProtocol::getFirstLine(buffer, length);
+        const std::string memory = LOOLProtocol::getFirstLine(payload);
         if (!memory.compare(0,6,"Error:"))
         {
             _failure = memory;
@@ -84,9 +73,31 @@ public:
             Log::info("Got memory stats [" + memory + "].");
             Poco::StringTokenizer tokens(memory, " ");
             assert(tokens.count() == 2);
-            totalPSS += atoi(tokens[0].c_str());
-            totalDirty += atoi(tokens[1].c_str());
+            _totalPSS += atoi(tokens[0].c_str());
+            _totalDirty += atoi(tokens[1].c_str());
         }
+
+        // Don't signal before wait.
+        std::unique_lock<std::mutex> lock(_mutex);
+        _cv.notify_one();
+        return true;
+    }
+
+    void getMemory(const std::shared_ptr<Poco::Net::WebSocket> &socket,
+                   size_t &totalPSS, size_t &totalDirty)
+    {
+        std::unique_lock<std::mutex> lock(_mutex);
+
+        /// Fetch memory usage data from the last process ...
+        socket->sendFrame("unit-memdump: \n", sizeof("unit-memdump: \n"));
+
+        if (_cv.wait_for(lock, std::chrono::milliseconds(5 * 1000)) == std::cv_status::timeout)
+        {
+            _failure = "Timed out waiting for child to respond to unit-memdump.";
+        }
+
+        totalPSS = _totalPSS;
+        totalDirty = _totalDirty;
     }
 
     virtual void newChild(const std::shared_ptr<Poco::Net::WebSocket> &socket) override