diff --git a/wsd/ClientSession.cpp b/wsd/ClientSession.cpp index 10a113438..9178dff73 100644 --- a/wsd/ClientSession.cpp +++ b/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 diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp index c92c1d8f7..6d87fb7be 100644 --- a/wsd/DocumentBroker.cpp +++ b/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& 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 lock(_mutex); - const auto it = _sessions.find(sessionId); if (it == _sessions.end()) { @@ -805,7 +806,7 @@ size_t DocumentBroker::addSession(const std::shared_ptr& 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& 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& 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& 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() diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp index ceb3db482..a74735317 100644 --- a/wsd/DocumentBroker.hpp +++ b/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& 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& 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); diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp index ffd7c6128..b5491e51a 100644 --- a/wsd/LOOLWSD.cpp +++ b/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(); } }