[Libreoffice-commits] online.git: loolwsd/ChildProcessSession.cpp loolwsd/IoUtil.cpp loolwsd/IoUtil.hpp loolwsd/LOOLKit.cpp loolwsd/LOOLSession.cpp loolwsd/LOOLSession.hpp loolwsd/LOOLWSD.cpp loolwsd/MasterProcessSession.cpp loolwsd/MasterProcessSession.hpp loolwsd/test

Henry Castro hcastro at collabora.com
Tue Apr 19 00:28:34 UTC 2016


 loolwsd/ChildProcessSession.cpp  |    7 +++-
 loolwsd/IoUtil.cpp               |    4 ++
 loolwsd/IoUtil.hpp               |    1 
 loolwsd/LOOLKit.cpp              |   21 +++++++++++--
 loolwsd/LOOLSession.cpp          |    4 --
 loolwsd/LOOLSession.hpp          |    7 ++++
 loolwsd/LOOLWSD.cpp              |   53 ++++++++++++++++++++++++++++++++-
 loolwsd/MasterProcessSession.cpp |   61 +++++++++++----------------------------
 loolwsd/MasterProcessSession.hpp |    5 +--
 loolwsd/test/httpwstest.cpp      |   47 ++++++++++++++++++++++++++++++
 10 files changed, 155 insertions(+), 55 deletions(-)

New commits:
commit c29944a386badbd7093d81ed2842e73b59f40cce
Author: Henry Castro <hcastro at collabora.com>
Date:   Mon Apr 18 19:12:26 2016 -0400

    loolwsd: fix close after close
    
    The closing handshake.
    Either peer can send a control frame with data containing
    a specified control sequence to begin the closing handshake.
    
    Upon receiving such a frame, the other peer sends a
    Close frame in response, if it hasn't already sent one.

diff --git a/loolwsd/ChildProcessSession.cpp b/loolwsd/ChildProcessSession.cpp
index ac8b8a0..18bdfb6 100644
--- a/loolwsd/ChildProcessSession.cpp
+++ b/loolwsd/ChildProcessSession.cpp
@@ -78,7 +78,12 @@ public:
         Log::trace() << "CallbackWorker::callback [" << _session.getViewId() << "] "
                      << LOKitHelper::kitCallbackTypeToString(nType)
                      << " [" << rPayload << "]." << Log::end;
-        if (_session.isDisconnected())
+        if (_session.isCloseFrame())
+        {
+            Log::trace("LOKit document begin the closing handshake");
+            return;
+        }
+        else if (_session.isDisconnected())
         {
             Log::trace("Skipping callback on disconnected session " + _session.getName());
             return;
diff --git a/loolwsd/IoUtil.cpp b/loolwsd/IoUtil.cpp
index 93a61ea..92b0254 100644
--- a/loolwsd/IoUtil.cpp
+++ b/loolwsd/IoUtil.cpp
@@ -41,6 +41,7 @@ namespace IoUtil
 // Handler returns false to end.
 void SocketProcessor(std::shared_ptr<WebSocket> ws,
                      std::function<bool(const std::vector<char>&)> handler,
+                     std::function<void()> closeFrame,
                      std::function<bool()> stopPredicate)
 {
     Log::info("SocketProcessor starting.");
@@ -93,6 +94,7 @@ void SocketProcessor(std::shared_ptr<WebSocket> ws,
             }
             else if (n <= 0 || ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_CLOSE))
             {
+                closeFrame();
                 Log::warn("Connection closed.");
                 break;
             }
@@ -109,6 +111,7 @@ void SocketProcessor(std::shared_ptr<WebSocket> ws,
                     n = ws->receiveFrame(buffer, sizeof(buffer), flags);
                     if (n <= 0 || (flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_CLOSE)
                     {
+                        closeFrame();
                         Log::warn("Connection closed while reading multiframe message.");
                         break;
                     }
@@ -138,6 +141,7 @@ void SocketProcessor(std::shared_ptr<WebSocket> ws,
 
             if (n <= 0 || (flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_CLOSE)
             {
+                closeFrame();
                 Log::warn("Connection closed.");
                 break;
             }
diff --git a/loolwsd/IoUtil.hpp b/loolwsd/IoUtil.hpp
index 476f515..2b49658 100644
--- a/loolwsd/IoUtil.hpp
+++ b/loolwsd/IoUtil.hpp
@@ -25,6 +25,7 @@ namespace IoUtil
     //. Handler returns false to end.
     void SocketProcessor(std::shared_ptr<Poco::Net::WebSocket> ws,
                          std::function<bool(const std::vector<char>&)> handler,
+                         std::function<void()> closeFrame,
                          std::function<bool()> stopPredicate);
 
     /// Call WebSocket::shutdown() ignoring Poco::IOException.
diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp
index 28e1d68..7a95bcb 100644
--- a/loolwsd/LOOLKit.cpp
+++ b/loolwsd/LOOLKit.cpp
@@ -51,6 +51,7 @@
 #include "LOOLProtocol.hpp"
 #include "QueueHandler.hpp"
 #include "Unit.hpp"
+#include "UserMessages.hpp"
 #include "Util.hpp"
 
 #define LIB_SOFFICEAPP  "lib" "sofficeapp" ".so"
@@ -280,18 +281,31 @@ public:
 
             Thread queueHandlerThread;
             queueHandlerThread.start(handler);
+            std::shared_ptr<ChildProcessSession> session = _session;
 
             IoUtil::SocketProcessor(_ws,
-                    [&queue](const std::vector<char>& payload)
+                [&queue](const std::vector<char>& payload)
                 {
                     queue->put(payload);
                     return true;
                 },
-                []() { return TerminationFlag; });
+                [&session]() { session->closeFrame(); },
+                [&queueHandlerThread]() { return TerminationFlag && queueHandlerThread.isRunning(); });
 
             queue->clear();
             queue->put("eof");
             queueHandlerThread.join();
+
+            if (session->isCloseFrame())
+            {
+                Log::trace("Normal close handshake.");
+                _ws->shutdown();
+            }
+            else
+            {
+                Log::trace("Abnormal close handshake.");
+               _ws->shutdown(WebSocket::WS_ENDPOINT_GOING_AWAY, SERVICE_UNAVALABLE_INTERNAL_ERROR);
+            }
         }
         catch (const Exception& exc)
         {
@@ -1047,7 +1061,8 @@ void lokit_main(const std::string& childRoot,
 
                     return true;
                 },
-                                [&document]()
+                []() {},
+                [&document]()
                 {
                     if (document && document->canDiscard())
                         TerminationFlag = true;
diff --git a/loolwsd/LOOLSession.cpp b/loolwsd/LOOLSession.cpp
index 9dda2e6..090f2ed 100644
--- a/loolwsd/LOOLSession.cpp
+++ b/loolwsd/LOOLSession.cpp
@@ -56,6 +56,7 @@ LOOLSession::LOOLSession(const std::string& id, const Kind kind,
     _isDocPasswordProvided(false),
     _isDocLoaded(false),
     _isDocPasswordProtected(false),
+    _isCloseFrame(false),
     _disconnected(false),
     _lastActivityTime(std::chrono::steady_clock::now())
 {
@@ -68,7 +69,6 @@ LOOLSession::LOOLSession(const std::string& id, const Kind kind,
 
 LOOLSession::~LOOLSession()
 {
-    IoUtil::shutdownWebSocket(_ws);
 }
 
 void LOOLSession::sendTextFrame(const std::string& text)
@@ -99,7 +99,6 @@ void LOOLSession::sendTextFrame(const std::string& text)
         Log::warn() << "LOOLSession::sendTextFrame: "
                     << "Exception: " << exc.displayText()
                     << (exc.nested() ? "( " + exc.nested()->displayText() + ")" : "");
-        IoUtil::shutdownWebSocket(_ws);
     }
 }
 
@@ -130,7 +129,6 @@ void LOOLSession::sendBinaryFrame(const char *buffer, int length)
         Log::warn() << "LOOLSession::sendBinaryFrame: "
                     << "Exception: " << exc.displayText()
                     << (exc.nested() ? "( " + exc.nested()->displayText() + ")" : "");
-        IoUtil::shutdownWebSocket(_ws);
     }
 }
 
diff --git a/loolwsd/LOOLSession.hpp b/loolwsd/LOOLSession.hpp
index 8a5b503..617b159 100644
--- a/loolwsd/LOOLSession.hpp
+++ b/loolwsd/LOOLSession.hpp
@@ -10,6 +10,7 @@
 #ifndef INCLUDED_LOOLSESSION_HPP
 #define INCLUDED_LOOLSESSION_HPP
 
+#include <atomic>
 #include <cassert>
 #include <memory>
 #include <mutex>
@@ -63,6 +64,9 @@ public:
         return std::chrono::duration_cast<std::chrono::milliseconds>(duration).count();
     }
 
+    void closeFrame() { _isCloseFrame = true; };
+    bool isCloseFrame() const { return _isCloseFrame; }
+
 protected:
     LOOLSession(const std::string& id, const Kind kind,
                 std::shared_ptr<Poco::Net::WebSocket> ws);
@@ -125,6 +129,9 @@ protected:
     /// Document options: a JSON string, containing options (rendering, also possibly load in the future).
     std::string _docOptions;
 
+    // Whether websocket received close frame.  Closing Handshake
+    std::atomic<bool> _isCloseFrame;
+
 private:
 
     virtual bool _handleInput(const char *buffer, int length) = 0;
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 0d9c516..5248b38 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -577,7 +577,8 @@ private:
                 queue->put(payload);
                 return true;
             },
-            []() { return TerminationFlag; });
+            [&session]() { session->closeFrame(); },
+            [&queueHandlerThread]() { return TerminationFlag && queueHandlerThread.isRunning(); });
 
         docBrokersLock.lock();
         const bool canDestroy = docBroker->canDestroy();
@@ -618,6 +619,26 @@ private:
             Log::info("Removing complete doc [" + docKey + "] from Admin.");
             Admin::instance().rmDoc(docKey);
         }
+        docBrokersLock.unlock();
+
+        if (session->isCloseFrame())
+        {
+            Log::trace("Normal close handshake.");
+            if (session->shutdownPeer(WebSocket::WS_NORMAL_CLOSE, ""))
+            {
+                // Client initiated close handshake
+                // respond close frame
+                ws->shutdown();
+            }
+        }
+        else
+        {
+            // something wrong, with internal exceptions
+            Log::trace("Abnormal close handshake.");
+            session->closeFrame();
+            ws->shutdown(WebSocket::WS_ENDPOINT_GOING_AWAY, SERVICE_UNAVALABLE_INTERNAL_ERROR);
+            session->shutdownPeer(WebSocket::WS_ENDPOINT_GOING_AWAY, SERVICE_UNAVALABLE_INTERNAL_ERROR);
+        }
     }
 
     /// Sends back the WOPI Discovery XML.
@@ -742,6 +763,10 @@ public:
             Log::error(std::string("ClientRequestHandler::handleRequest: Exception: ") + exc.what());
             response.setStatusAndReason(HTTPResponse::HTTP_SERVICE_UNAVAILABLE);
         }
+        catch (...)
+        {
+            Log::error("ClientRequestHandler::handleRequest:: Unexpected exception");
+        }
 
         if (!responded)
         {
@@ -925,11 +950,31 @@ public:
             UnitWSD::get().onChildConnected(pid, sessionId);
 
             IoUtil::SocketProcessor(ws,
-                    [&session](const std::vector<char>& payload)
+                [&session](const std::vector<char>& payload)
                 {
                     return session->handleInput(payload.data(), payload.size());
                 },
+                [&session]() { session->closeFrame(); },
                 []() { return TerminationFlag; });
+
+            if (session->isCloseFrame())
+            {
+                Log::trace("Normal close handshake.");
+                if (session->shutdownPeer(WebSocket::WS_NORMAL_CLOSE, ""))
+                {
+                    // LOKit initiated close handshake
+                    // respond close frame
+                    ws->shutdown();
+                }
+            }
+            else
+            {
+                // something wrong, with internal exceptions
+                Log::trace("Abnormal close handshake.");
+                session->closeFrame();
+                ws->shutdown(WebSocket::WS_ENDPOINT_GOING_AWAY, SERVICE_UNAVALABLE_INTERNAL_ERROR);
+                session->shutdownPeer(WebSocket::WS_ENDPOINT_GOING_AWAY, SERVICE_UNAVALABLE_INTERNAL_ERROR);
+            }
         }
         catch (const Exception& exc)
         {
@@ -941,6 +986,10 @@ public:
         {
             Log::error(std::string("PrisonerRequestHandler::handleRequest: Exception: ") + exc.what());
         }
+        catch (...)
+        {
+            Log::error("PrisonerRequestHandler::handleRequest:: Unexpected exception");
+        }
 
         if (!jailId.empty())
         {
diff --git a/loolwsd/MasterProcessSession.cpp b/loolwsd/MasterProcessSession.cpp
index 5c053c7..c2d4856 100644
--- a/loolwsd/MasterProcessSession.cpp
+++ b/loolwsd/MasterProcessSession.cpp
@@ -47,46 +47,8 @@ MasterProcessSession::~MasterProcessSession()
 {
     Log::info("~MasterProcessSession dtor [" + getName() + "].");
 
-    try
-    {
-        // We could be unwinding because our peer's connection
-        // died. Handle I/O errors in that case.
-        disconnect();
-    }
-    catch (const std::exception& exc)
-    {
-        Log::error(std::string("MasterProcessSession::~MasterProcessSession: Exception: ") + exc.what());
-    }
-}
-
-void MasterProcessSession::disconnect()
-{
-    if (!isDisconnected())
-    {
-        LOOLSession::disconnect();
-
-        // Release the save-as queue.
-        _saveAsQueue.put("");
-
-        auto peer = _peer.lock();
-        if (peer)
-        {
-            peer->disconnect();
-        }
-    }
-}
-
-bool MasterProcessSession::handleDisconnect()
-{
-    Log::info("Graceful disconnect on " + getName() + ".");
-
-    LOOLSession::handleDisconnect();
-
-    auto peer = _peer.lock();
-    if (peer)
-        peer->disconnect();
-
-    return false;
+    // Release the save-as queue.
+    _saveAsQueue.put("");
 }
 
 bool MasterProcessSession::_handleInput(const char *buffer, int length)
@@ -126,8 +88,7 @@ bool MasterProcessSession::_handleInput(const char *buffer, int length)
         {
             if (!peer)
             {
-                LOOLSession::disconnect();
-                return false;
+                throw Poco::ProtocolException("The session has not been assigned a peer.");
             }
 
             if (tokens[0] == "unocommandresult:")
@@ -768,11 +729,25 @@ void MasterProcessSession::forwardToPeer(const char *buffer, int length)
     auto peer = _peer.lock();
     if (!peer)
     {
-        Log::error(getName() + ": no peer to forward to.");
+        throw Poco::ProtocolException(getName() + ": no peer to forward to.");
+    }
+    else if (peer->isCloseFrame())
+    {
+        Log::trace(getName() + ": peer begin the closing handshake");
         return;
     }
 
     peer->sendBinaryFrame(buffer, length);
 }
 
+bool MasterProcessSession::shutdownPeer(Poco::UInt16 statusCode, const std::string& message)
+{
+    auto peer = _peer.lock();
+    if (peer && !peer->isCloseFrame())
+    {
+        peer->_ws->shutdown(statusCode, message);
+    }
+    return peer != nullptr;
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/loolwsd/MasterProcessSession.hpp b/loolwsd/MasterProcessSession.hpp
index 47b2d97..c38b20f 100644
--- a/loolwsd/MasterProcessSession.hpp
+++ b/loolwsd/MasterProcessSession.hpp
@@ -36,9 +36,6 @@ class MasterProcessSession final : public LOOLSession, public std::enable_shared
 
     virtual bool getPartPageRectangles(const char *buffer, int length) override;
 
-    virtual void disconnect() override;
-    virtual bool handleDisconnect() override;
-
     /**
      * Return the URL of the saved-as document when it's ready. If called
      * before it's ready, the call blocks till then.
@@ -55,6 +52,8 @@ class MasterProcessSession final : public LOOLSession, public std::enable_shared
 
     bool isEditLocked() const { return _bEditLock; }
 
+    bool shutdownPeer(Poco::UInt16 statusCode, const std::string& message);
+
 public:
     // Raise this flag on ToClient from ToPrisoner to let ToClient know of load failures
     bool _bLoadError = false;
diff --git a/loolwsd/test/httpwstest.cpp b/loolwsd/test/httpwstest.cpp
index 81d6a16..59dfda4 100644
--- a/loolwsd/test/httpwstest.cpp
+++ b/loolwsd/test/httpwstest.cpp
@@ -53,6 +53,7 @@ class HTTPWSTest : public CPPUNIT_NS::TestFixture
 
     CPPUNIT_TEST(testBadRequest);
     CPPUNIT_TEST(testHandShake);
+    CPPUNIT_TEST(testCloseAfterClose);
     CPPUNIT_TEST(testLoad);
     CPPUNIT_TEST(testBadLoad);
     CPPUNIT_TEST(testReload);
@@ -76,6 +77,7 @@ class HTTPWSTest : public CPPUNIT_NS::TestFixture
     void testCountHowManyLoolkits();
     void testBadRequest();
     void testHandShake();
+    void testCloseAfterClose();
     void testLoad();
     void testBadLoad();
     void testReload();
@@ -272,6 +274,51 @@ void HTTPWSTest::testHandShake()
     }
 }
 
+void HTTPWSTest::testCloseAfterClose()
+{
+    try
+    {
+        int bytes;
+        int flags;
+        char buffer[READ_BUFFER_SIZE];
+
+        // Load a document and get its status.
+        const std::string documentPath = Util::getTempFilePath(TDOC, "hello.odt");
+        const std::string documentURL = "file://" + Poco::Path(documentPath).makeAbsolute().toString();
+
+        Poco::Net::HTTPRequest request(Poco::Net::HTTPRequest::HTTP_GET, documentURL);
+        Poco::Net::WebSocket socket = *connectLOKit(request, _response);
+
+        sendTextFrame(socket, "load url=" + documentURL);
+        sendTextFrame(socket, "status");
+        CPPUNIT_ASSERT_MESSAGE("cannot load the document " + documentURL, isDocumentLoaded(socket));
+
+        // send normal socket shutdown
+        socket.shutdown();
+
+        // 5 seconds timeout
+        socket.setReceiveTimeout(5000000);
+
+        // receive close frame handshake
+        do
+        {
+            bytes = socket.receiveFrame(buffer, sizeof(buffer), flags);
+        }
+        while ((flags & Poco::Net::WebSocket::FRAME_OP_BITMASK) != Poco::Net::WebSocket::FRAME_OP_CLOSE);
+
+        // no more messages is received.
+        bytes = socket.receiveFrame(buffer, sizeof(buffer), flags);
+        std::string received(buffer);
+        std::cout << received << "received " << bytes << " flags "<< flags << std::endl;
+        CPPUNIT_ASSERT_EQUAL(0, bytes);
+        CPPUNIT_ASSERT_EQUAL(0, flags);
+    }
+    catch (const Poco::Exception& exc)
+    {
+        CPPUNIT_FAIL(exc.displayText());
+    }
+}
+
 void HTTPWSTest::loadDoc(const std::string& documentURL)
 {
     try


More information about the Libreoffice-commits mailing list