wsd: terminate DocBrokers asynchronously

Remove locks and replace with isCorrectThread
assertions instead.

Crash recovery still needs some work, but
otherwise tests are clean (91/94 pass).

Change-Id: I9ac3e21854447d19a8e6106487dfd8be00fcf5ef

wsd/ClientSession.cpp

@@ -611,8 +611,7 @@ bool ClientSession::handleKitToClientMessage(const char* buffer, const int lengt
             docBroker->removeSession(getId());
 
             // Now terminate.
-            auto lock = docBroker->getLock();
-            docBroker->terminateChild(lock, "", true);
+            docBroker->stop();
         }
 
         return true;
@@ -731,6 +730,7 @@ void ClientSession::onDisconnect()
 
     const auto docBroker = getDocumentBroker();
     LOG_CHECK_RET(docBroker && "Null DocumentBroker instance", );
+    assert(docBroker->isCorrectThread());
     const auto docKey = docBroker->getDocKey();
 
     try

wsd/DocumentBroker.cpp

@@ -259,6 +259,10 @@ void DocumentBroker::pollThread()
         }
     }
 
+    // Terminate properly while we can.
+    //TODO: pass some sensible reason.
+    terminateChild("", false);
+
     // Flush socket data.
     const int flushTimeoutMs = POLL_TIMEOUT_MS * 2; // ~1000ms
     const auto flushStartTime = std::chrono::steady_clock::now();
@@ -272,9 +276,6 @@ void DocumentBroker::pollThread()
         _poll->poll(std::min(flushTimeoutMs - elapsedMs, POLL_TIMEOUT_MS / 5));
     }
 
-    // Terminate properly while we can.
-    auto lock = getLock();
-    terminateChild(lock, "", false);
     LOG_INF("Finished docBroker polling thread for docKey [" << _docKey << "].");
 }
 
@@ -501,6 +502,8 @@ bool DocumentBroker::load(const std::shared_ptr<ClientSession>& session, const s
 bool DocumentBroker::saveToStorage(const std::string& sessionId,
                                    bool success, const std::string& result)
 {
+    assert(isCorrectThread());
+
     const bool res = saveToStorageInternal(sessionId, success, result);
 
     // If marked to destroy, then this was the last session.
@@ -537,8 +540,6 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
         return true;
     }
 
-    std::unique_lock<std::mutex> lock(_mutex);
-
     const auto it = _sessions.find(sessionId);
     if (it == _sessions.end())
     {
@@ -805,7 +806,7 @@ size_t DocumentBroker::addSession(const std::shared_ptr<ClientSession>& session)
 
 size_t DocumentBroker::removeSession(const std::string& id, bool destroyIfLast)
 {
-    auto guard = getLock();
+    assert(isCorrectThread());
 
     if (destroyIfLast)
         destroyIfLastEditor(id);
@@ -828,6 +829,7 @@ size_t DocumentBroker::removeSession(const std::string& id, bool destroyIfLast)
 
 size_t DocumentBroker::removeSessionInternal(const std::string& id)
 {
+    assert(isCorrectThread());
     try
     {
         Admin::instance().rmDoc(_docKey, id);
@@ -1139,7 +1141,7 @@ void DocumentBroker::handleTileCombinedResponse(const std::vector<char>& payload
 
 void DocumentBroker::destroyIfLastEditor(const std::string& id)
 {
-    Util::assertIsLocked(_mutex);
+    assert(isCorrectThread());
 
     const auto currentSession = _sessions.find(id);
     if (currentSession == _sessions.end())
@@ -1275,10 +1277,9 @@ void DocumentBroker::childSocketTerminated()
     }
 }
 
-void DocumentBroker::terminateChild(std::unique_lock<std::mutex>& lock, const std::string& closeReason, const bool rude)
+void DocumentBroker::terminateChild(const std::string& closeReason, const bool rude)
 {
-    Util::assertIsLocked(_mutex);
-    Util::assertIsLocked(lock);
+    assert(isCorrectThread());
 
     LOG_INF("Terminating doc [" << _docKey << "].");
 
@@ -1309,26 +1310,20 @@ void DocumentBroker::terminateChild(std::unique_lock<std::mutex>& lock, const st
             _childProcess->stop();
         }
 
-        // Release the lock and wait for the thread to finish.
-        lock.unlock();
-
         _childProcess->close(rude);
     }
 
     // Stop the polling thread.
     _poll->stop();
     _stop = true;
-
-    // Trigger cleanup.
-    LOOLWSD::triggerChildAndDocHousekeeping();
 }
 
 void DocumentBroker::closeDocument(const std::string& reason)
 {
-    auto lock = getLock();
+    assert(isCorrectThread());
 
     LOG_DBG("Closing DocumentBroker for docKey [" << _docKey << "] with reason: " << reason);
-    terminateChild(lock, reason, true);
+    terminateChild(reason, true);
 }
 
 void DocumentBroker::updateLastActivityTime()

wsd/DocumentBroker.hpp

@@ -224,6 +224,10 @@ public:
     /// Start processing events
     void startThread();
 
+    /// Flag for termination.
+    //TODO: Take reason to broadcast to clients.
+    void stop() { _stop = true; }
+
     /// Loads a document from the public URI into the jail.
     bool load(const std::shared_ptr<ClientSession>& session, const std::string& jailId);
     bool isLoaded() const { return _isLoaded; }
@@ -319,13 +323,6 @@ public:
     /// or upon failing to process an incoming message.
     void childSocketTerminated();
 
-    /// This gracefully terminates the connection
-    /// with the child and cleans up ChildProcess etc.
-    /// We must be called under lock and it must be
-    /// passed to us so we unlock before waiting on
-    /// the ChildProcess thread, which can take our lock.
-    void terminateChild(std::unique_lock<std::mutex>& lock, const std::string& closeReason, const bool rude);
-
     /// Get the PID of the associated child process
     Poco::Process::PID getPid() const { return _childProcess->getPid(); }
 
@@ -341,6 +338,10 @@ public:
     }
 
 private:
+    /// This gracefully terminates the connection
+    /// with the child and cleans up ChildProcess etc.
+    void terminateChild(const std::string& closeReason, const bool rude);
+
     /// Sends the .uno:Save command to LoKit.
     bool sendUnoSave(const bool dontSaveIfUnmodified);
 

wsd/LOOLWSD.cpp

@@ -248,7 +248,7 @@ bool cleanupDocBrokers()
             {
                 LOG_INF("Terminating " << (idle ? "idle" : "dead") <<
                         " DocumentBroker for docKey [" << it->first << "].");
-                docBroker->terminateChild(lock, idle ? "idle" : "", true);
+                docBroker->stop();
 
                 // Remove only when not alive.
                 if (!docBroker->isAlive())
@@ -1351,8 +1351,7 @@ public:
         if (docBroker)
         {
             // FIXME: No need to notify if asked to stop.
-            auto lock = docBroker->getLock();
-            docBroker->terminateChild(lock, "Service unavailable", true);
+            docBroker->stop();
         }
     }
 
@@ -1379,7 +1378,7 @@ private:
         {
             auto lock = docBroker->getLock();
             assert(docBroker->isCorrectThread());
-            docBroker->terminateChild(lock, "Service unavailable", false);
+            docBroker->stop();
         }
     }