[Libreoffice-commits] online.git: Branch 'distro/cib/libreoffice-6-2' - 6 commits - configure.ac wsd/ClientSession.cpp wsd/DocumentBroker.cpp wsd/DocumentBroker.hpp wsd/LOOLWSD.cpp

Samuel Mehrbrodt (via logerrit) logerrit at kemper.freedesktop.org
Tue Apr 14 07:13:38 UTC 2020


 configure.ac           |    2 +-
 wsd/ClientSession.cpp  |    5 -----
 wsd/DocumentBroker.cpp |   44 ++++++++++++++++++++++++++++++++++++++------
 wsd/DocumentBroker.hpp |   24 ++++++++++++++++++++----
 wsd/LOOLWSD.cpp        |   44 ++++++++++++++++++++++++++++++--------------
 5 files changed, 89 insertions(+), 30 deletions(-)

New commits:
commit 49c65bbc306cabf71323cb9f5e8b47528cb76f2f
Author:     Samuel Mehrbrodt <Samuel.Mehrbrodt at cib.de>
AuthorDate: Tue Apr 14 09:10:12 2020 +0200
Commit:     Samuel Mehrbrodt <Samuel.Mehrbrodt at cib.de>
CommitDate: Tue Apr 14 09:10:12 2020 +0200

    Release 6.2.7.0

diff --git a/configure.ac b/configure.ac
index 5aebd9ba5..9a8a64c96 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3,7 +3,7 @@
 
 AC_PREREQ([2.63])
 
-AC_INIT([loolwsd], [6.2.6.0], [libreoffice at lists.freedesktop.org])
+AC_INIT([loolwsd], [6.2.7.0], [libreoffice at lists.freedesktop.org])
 LT_INIT([shared, disable-static, dlopen])
 
 AM_INIT_AUTOMAKE([1.10 subdir-objects tar-pax -Wno-portability])
commit c6e72be9fcdf3a3439266f4e3b7a4f87565f2c3c
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Tue May 21 19:50:17 2019 +0100
Commit:     Samuel Mehrbrodt <Samuel.Mehrbrodt at cib.de>
CommitDate: Tue Apr 14 08:25:11 2020 +0200

    tdf#123482 - cleanup convert-to folder even more reliably.
    
    Problems could occur if exceptiosn thrown when parsing the input stream.
    
    Change-Id: Id82b3816450194164fc2093554c730b4a94acef1
    Reviewed-on: https://gerrit.libreoffice.org/72695
    Reviewed-by: Michael Meeks <michael.meeks at collabora.com>
    Tested-by: Michael Meeks <michael.meeks at collabora.com>
    (cherry picked from commit 1845ed42af661dbb7b10a2fb4686ce2c8ef7e521)

diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 86524325f..61691133f 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -1880,13 +1880,18 @@ ConvertToBroker::ConvertToBroker(const std::string& uri,
 ConvertToBroker::~ConvertToBroker()
 {
     NumConverters--;
-    if (!_uriOrig.empty())
+    removeFile(_uriOrig);
+}
+
+void ConvertToBroker::removeFile(const std::string &uriOrig)
+{
+    if (!uriOrig.empty())
     {
         // Remove source file and directory
-        Poco::Path path = _uriOrig;
+        Poco::Path path = uriOrig;
         Poco::File(path).remove();
         Poco::File(path.makeParent()).remove();
-        FileUtil::removeFile(_uriOrig);
+        FileUtil::removeFile(uriOrig);
     }
 }
 
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index 4e628af12..b2c7543ba 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -484,6 +484,9 @@ public:
 
     /// How many live conversions are running.
     static size_t getInstanceCount();
+
+    /// Cleanup path and its parent
+    static void removeFile(const std::string &uri);
 };
 
 #endif
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 4878e116e..5415224fa 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -570,6 +570,9 @@ class ConvertToPartHandler : public PartHandler
 public:
     std::string getFilename() const { return _filename; }
 
+    /// Afterwards someone else is responsible for cleaning that up.
+    void takeFile() { _filename.clear(); }
+
     ConvertToPartHandler(bool convertTo = false)
         : _convertTo(convertTo)
     {
@@ -577,6 +580,11 @@ public:
 
     virtual ~ConvertToPartHandler()
     {
+        if (!_filename.empty())
+        {
+            LOG_TRC("Remove un-handled temporary file '" << _filename << "'");
+            ConvertToBroker::removeFile(_filename);
+        }
     }
 
     virtual void handlePart(const MessageHeader& header, std::istream& stream) override
@@ -2362,6 +2370,7 @@ private:
 
                     LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
                     auto docBroker = std::make_shared<ConvertToBroker>(fromPath, uriPublic, docKey);
+                    handler.takeFile();
 
                     cleanupDocBrokers();
 
commit dab5603fc6bdb9ffbc576c9ae693724484f31b37
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Tue Mar 12 15:41:54 2019 +0100
Commit:     Samuel Mehrbrodt <Samuel.Mehrbrodt at cib.de>
CommitDate: Tue Apr 14 08:23:49 2020 +0200

    Don't count convert-to connections vs. the document count.
    
    Change-Id: I350905fb98c503ae8f22a377e4af5cbcb9f3c52d
    (cherry picked from commit d8bb92cdcf83216ae5a56b8602cbd9f7c72b4975)

diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 2d00ac568..86524325f 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -1862,8 +1862,24 @@ void DocumentBroker::getIOStats(uint64_t &sent, uint64_t &recv)
     }
 }
 
+static std::atomic<size_t> NumConverters;
+
+size_t ConvertToBroker::getInstanceCount()
+{
+    return NumConverters;
+}
+
+ConvertToBroker::ConvertToBroker(const std::string& uri,
+                                 const Poco::URI& uriPublic,
+                                 const std::string& docKey)
+    : DocumentBroker(uri, uriPublic, docKey)
+{
+    NumConverters++;
+}
+
 ConvertToBroker::~ConvertToBroker()
 {
+    NumConverters--;
     if (!_uriOrig.empty())
     {
         // Remove source file and directory
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index ddf5ec10b..4e628af12 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -479,11 +479,11 @@ public:
     /// Construct DocumentBroker with URI and docKey
     ConvertToBroker(const std::string& uri,
                     const Poco::URI& uriPublic,
-                    const std::string& docKey)
-        : DocumentBroker(uri, uriPublic, docKey)
-    {
-    }
+                    const std::string& docKey);
     virtual ~ConvertToBroker();
+
+    /// How many live conversions are running.
+    static size_t getInstanceCount();
 };
 
 #endif
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index c8b20552c..4878e116e 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -254,8 +254,8 @@ inline void shutdownLimitReached(WebSocketHandler& ws)
 inline void checkSessionLimitsAndWarnClients()
 {
 #ifndef MOBILEAPP
-
-    if (DocBrokers.size() > LOOLWSD::MaxDocuments || LOOLWSD::NumConnections >= LOOLWSD::MaxConnections)
+    size_t docBrokerCount = DocBrokers.size() - ConvertToBroker::getInstanceCount();
+    if (docBrokerCount > LOOLWSD::MaxDocuments || LOOLWSD::NumConnections >= LOOLWSD::MaxConnections)
     {
         const std::string info = Poco::format(PAYLOAD_INFO_LIMIT_REACHED, LOOLWSD::MaxDocuments, LOOLWSD::MaxConnections);
         LOG_INF("Sending client 'limitreached' message: " << info);
@@ -2931,6 +2931,7 @@ public:
                   << "[ " << DocBrokers.size() << " ]:\n";
         for (auto &i : DocBrokers)
             i.second->dumpState(os);
+        os << "Converter count: " << ConvertToBroker::getInstanceCount() << "\n";
 
         Socket::InhibitThreadChecks = false;
         SocketPoll::InhibitThreadChecks = false;
commit 3d5ccd6186e6b6ee55da938b8ac0c9b8068779e4
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Fri Mar 1 22:25:44 2019 +0100
Commit:     Samuel Mehrbrodt <Samuel.Mehrbrodt at cib.de>
CommitDate: Tue Apr 14 08:17:42 2020 +0200

    tdf#123482 - cleanup convert-to folder more reliably.
    
    Change-Id: I029bb4136984e05485e462c92da80b92b00fdebc
    (cherry picked from commit 2d473222e4cad399b131345395d6506b26e0e134)

diff --git a/wsd/ClientSession.cpp b/wsd/ClientSession.cpp
index 102fc0325..e3cb678e5 100644
--- a/wsd/ClientSession.cpp
+++ b/wsd/ClientSession.cpp
@@ -887,11 +887,6 @@ bool ClientSession::handleKitToClientMessage(const char* buffer, const int lengt
 
             // Now terminate.
             docBroker->stop("Finished saveas handler.");
-
-            // Remove file and directory
-            Poco::Path path = docBroker->getDocKey();
-            Poco::File(path).remove();
-            Poco::File(path.makeParent()).remove();
         }
 
         return true;
diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 934c2ce0b..2d00ac568 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -37,6 +37,7 @@
 #include <common/Message.hpp>
 #include <common/Protocol.hpp>
 #include <common/Unit.hpp>
+#include <common/FileUtil.hpp>
 
 #include <sys/types.h>
 #include <sys/wait.h>
@@ -1861,6 +1862,18 @@ void DocumentBroker::getIOStats(uint64_t &sent, uint64_t &recv)
     }
 }
 
+ConvertToBroker::~ConvertToBroker()
+{
+    if (!_uriOrig.empty())
+    {
+        // Remove source file and directory
+        Poco::Path path = _uriOrig;
+        Poco::File(path).remove();
+        Poco::File(path.makeParent()).remove();
+        FileUtil::removeFile(_uriOrig);
+    }
+}
+
 void DocumentBroker::dumpState(std::ostream& os)
 {
     std::unique_lock<std::mutex> lock(_mutex);
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index 15d3b95b7..ddf5ec10b 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -223,7 +223,7 @@ public:
                    const Poco::URI& uriPublic,
                    const std::string& docKey);
 
-    ~DocumentBroker();
+    virtual ~DocumentBroker();
 
     /// Start processing events
     void startThread();
@@ -402,8 +402,9 @@ private:
     /// Sum the I/O stats from all connected sessions
     void getIOStats(uint64_t &sent, uint64_t &recv);
 
-private:
+protected:
     const std::string _uriOrig;
+private:
     const Poco::URI _uriPublic;
     /// URL-based key. May be repeated during the lifetime of WSD.
     const std::string _docKey;
@@ -472,6 +473,19 @@ private:
     static std::atomic<unsigned> DocBrokerId;
 };
 
+class ConvertToBroker : public DocumentBroker
+{
+public:
+    /// Construct DocumentBroker with URI and docKey
+    ConvertToBroker(const std::string& uri,
+                    const Poco::URI& uriPublic,
+                    const std::string& docKey)
+        : DocumentBroker(uri, uriPublic, docKey)
+    {
+    }
+    virtual ~ConvertToBroker();
+};
+
 #endif
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index a86a996a5..c8b20552c 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -593,6 +593,7 @@ public:
         if (!params.has("filename"))
             return;
 
+        // FIXME: needs wrapping - until then - keep in sync with ~ConvertToBroker
         Path tempPath = _convertTo? Path::forDirectory(Poco::TemporaryFile::tempName("/tmp/convert-to") + "/") :
                                     Path::forDirectory(Poco::TemporaryFile::tempName() + "/");
         File(tempPath).createDirectories();
@@ -2360,7 +2361,7 @@ private:
                     std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
 
                     LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
-                    auto docBroker = std::make_shared<DocumentBroker>(fromPath, uriPublic, docKey);
+                    auto docBroker = std::make_shared<ConvertToBroker>(fromPath, uriPublic, docKey);
 
                     cleanupDocBrokers();
 
commit 75bd2359807fa8a6e7c3d14cb22b937c326790eb
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Fri Mar 1 22:03:35 2019 +0100
Commit:     Samuel Mehrbrodt <Samuel.Mehrbrodt at cib.de>
CommitDate: Tue Apr 14 08:15:49 2020 +0200

    Simpify DocumentBroker constructor.
    
    Change-Id: I0bf29df9316b129d34862c7464bb6636d42a850d
    (cherry picked from commit 51b72b04a9e0c39df525caeb0744ca44a8ba0143)

diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 661dbb2a2..934c2ce0b 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -145,13 +145,11 @@ std::atomic<unsigned> DocumentBroker::DocBrokerId(1);
 
 DocumentBroker::DocumentBroker(const std::string& uri,
                                const Poco::URI& uriPublic,
-                               const std::string& docKey,
-                               const std::string& childRoot) :
+                               const std::string& docKey) :
     _uriOrig(uri),
     _uriPublic(uriPublic),
     _docKey(docKey),
     _docId(Util::encodeId(DocBrokerId++, 3)),
-    _childRoot(childRoot),
     _cacheRoot(getCachePath(uriPublic.toString())),
     _documentChangedInStorage(false),
     _lastSaveTime(std::chrono::steady_clock::now()),
@@ -171,10 +169,10 @@ DocumentBroker::DocumentBroker(const std::string& uri,
     _debugRenderedTileCount(0)
 {
     assert(!_docKey.empty());
-    assert(!_childRoot.empty());
+    assert(!LOOLWSD::ChildRoot.empty());
 
     LOG_INF("DocumentBroker [" << LOOLWSD::anonymizeUrl(_uriPublic.toString()) <<
-            "] created with docKey [" << _docKey << "] and root [" << _childRoot << "]");
+            "] created with docKey [" << _docKey << "]");
 }
 
 void DocumentBroker::startThread()
@@ -1051,7 +1049,7 @@ bool DocumentBroker::sendUnoSave(const std::string& sessionId, bool dontTerminat
 std::string DocumentBroker::getJailRoot() const
 {
     assert(!_jailId.empty());
-    return Poco::Path(_childRoot, _jailId).toString();
+    return Poco::Path(LOOLWSD::ChildRoot, _jailId).toString();
 }
 
 size_t DocumentBroker::addSession(const std::shared_ptr<ClientSession>& session)
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index 45f37edd1..15d3b95b7 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -221,8 +221,7 @@ public:
     /// Construct DocumentBroker with URI, docKey, and root path.
     DocumentBroker(const std::string& uri,
                    const Poco::URI& uriPublic,
-                   const std::string& docKey,
-                   const std::string& childRoot);
+                   const std::string& docKey);
 
     ~DocumentBroker();
 
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 115441065..a86a996a5 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -1696,7 +1696,7 @@ static std::shared_ptr<DocumentBroker> findOrCreateDocBroker(WebSocketHandler& w
 
         // Set the one we just created.
         LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
-        docBroker = std::make_shared<DocumentBroker>(uri, uriPublic, docKey, LOOLWSD::ChildRoot);
+        docBroker = std::make_shared<DocumentBroker>(uri, uriPublic, docKey);
         DocBrokers.emplace(docKey, docBroker);
         LOG_TRC("Have " << DocBrokers.size() << " DocBrokers after inserting [" << docKey << "].");
     }
@@ -2360,7 +2360,7 @@ private:
                     std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
 
                     LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
-                    auto docBroker = std::make_shared<DocumentBroker>(fromPath, uriPublic, docKey, LOOLWSD::ChildRoot);
+                    auto docBroker = std::make_shared<DocumentBroker>(fromPath, uriPublic, docKey);
 
                     cleanupDocBrokers();
 
commit 6d7d4ed4662be8b9c8607358e81ac1fed051fa74
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Fri Mar 1 18:59:40 2019 +0100
Commit:     Samuel Mehrbrodt <Samuel.Mehrbrodt at cib.de>
CommitDate: Tue Apr 14 08:04:34 2020 +0200

    Avoid using un-necessary reference.
    
    Change-Id: Id5a9fed8fb790f2af8facac119e9e0da476b1e47
    (cherry picked from commit b255ce528bb6c7d037c99ff612a8efba92182abd)

diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index a4a5533fe..115441065 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -558,18 +558,24 @@ std::shared_ptr<ChildProcess> getNewChild_Blocks(
 
 #ifndef MOBILEAPP
 
-/// Handles the filename part of the convert-to POST request payload.
+/// Handles the filename part of the convert-to POST request payload,
+/// Also owns the file - cleaning it up when destroyed.
 class ConvertToPartHandler : public PartHandler
 {
-    std::string& _filename;
+    std::string _filename;
 
     /// Is it really a convert-to, ie. use an especially formed path?
     bool _convertTo;
 
 public:
-    ConvertToPartHandler(std::string& filename, bool convertTo = false)
-        : _filename(filename)
-        , _convertTo(convertTo)
+    std::string getFilename() const { return _filename; }
+
+    ConvertToPartHandler(bool convertTo = false)
+        : _convertTo(convertTo)
+    {
+    }
+
+    virtual ~ConvertToPartHandler()
     {
     }
 
@@ -2318,8 +2324,7 @@ private:
         StringTokenizer tokens(request.getURI(), "/?");
         if (tokens.count() > 2 && tokens[2] == "convert-to")
         {
-            std::string fromPath;
-            ConvertToPartHandler handler(fromPath, /*convertTo =*/ true);
+            ConvertToPartHandler handler(/*convertTo =*/ true);
             HTMLForm form(request, message, handler);
 
             std::string format = (form.has("format") ? form.get("format") : "");
@@ -2343,6 +2348,7 @@ private:
                 format = tokens[3];
 
             bool sent = false;
+            std::string fromPath = handler.getFilename();
             LOG_INF("Conversion request for URI [" << fromPath << "] format [" << format << "].");
             if (!fromPath.empty() && !format.empty())
             {
@@ -2428,8 +2434,7 @@ private:
         {
             LOG_INF("Insert file request.");
 
-            std::string tmpPath;
-            ConvertToPartHandler handler(tmpPath);
+            ConvertToPartHandler handler;
             HTMLForm form(request, message, handler);
 
             if (form.has("childid") && form.has("name"))
@@ -2459,7 +2464,7 @@ private:
                                               + JAILED_DOCUMENT_ROOT + "insertfile";
                     File(dirPath).createDirectories();
                     std::string fileName = dirPath + "/" + form.get("name");
-                    File(tmpPath).moveTo(fileName);
+                    File(handler.getFilename()).moveTo(fileName);
                     response.setContentLength(0);
                     socket->send(response);
                     return;


More information about the Libreoffice-commits mailing list