[Libreoffice-commits] online.git: loolwsd/DocumentBroker.cpp loolwsd/DocumentBroker.hpp loolwsd/LOOLWSD.cpp
Ashod Nakashian
ashod.nakashian at collabora.co.uk
Mon Nov 7 06:31:45 UTC 2016
loolwsd/DocumentBroker.cpp | 9 +-
loolwsd/DocumentBroker.hpp | 6 +
loolwsd/LOOLWSD.cpp | 147 ++++++++++++++++++++++-----------------------
3 files changed, 81 insertions(+), 81 deletions(-)
New commits:
commit 3993757ee806d7bdde2408852f6227b8f848f1d8
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date: Sat Nov 5 22:15:44 2016 -0400
loolwsd: fix race while creating new documents
This fixes the race rather than trying to patch it.
It still minimizes the locking necessary to a minimum
to maximize parallelism.
The approach is to have at least the DocBrokers lock
or the DocumentBroker lock (if we already have a doc)
while creating new document views.
Change-Id: I96b4f17b3be3d03cd5e6f4d17d39e2165fe008a7
Reviewed-on: https://gerrit.libreoffice.org/30628
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 97e21b4..19dbb97 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -358,9 +358,10 @@ bool DocumentBroker::save(const std::string& sessionId, bool success, const std:
return false;
}
-bool DocumentBroker::autoSave(const bool force, const size_t waitTimeoutMs)
+bool DocumentBroker::autoSave(const bool force, const size_t waitTimeoutMs, std::unique_lock<std::mutex>& lock)
{
- std::unique_lock<std::mutex> lock(_mutex);
+ Util::assertIsLocked(lock);
+
if (_sessions.empty() || _storage == nullptr || !_isLoaded ||
!_childProcess->isAlive() || (!_isModified && !force))
{
@@ -549,7 +550,7 @@ bool DocumentBroker::connectPeers(std::shared_ptr<PrisonerSession>& session)
size_t DocumentBroker::removeSession(const std::string& id)
{
- std::lock_guard<std::mutex> lock(_mutex);
+ Util::assertIsLocked(_mutex);
auto it = _sessions.find(id);
if (it != _sessions.end())
@@ -804,7 +805,7 @@ void DocumentBroker::handleTileCombinedResponse(const std::vector<char>& payload
bool DocumentBroker::startDestroy(const std::string& id)
{
- std::unique_lock<std::mutex> lock(_mutex);
+ Util::assertIsLocked(_mutex);
const auto currentSession = _sessions.find(id);
assert(currentSession != _sessions.end());
diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp
index 17de968..e58b642 100644
--- a/loolwsd/DocumentBroker.hpp
+++ b/loolwsd/DocumentBroker.hpp
@@ -195,7 +195,7 @@ public:
/// complete before returning, or timeout.
/// @return true if attempts to save or it also waits
/// and receives save notification. Otherwise, false.
- bool autoSave(const bool force, const size_t waitTimeoutMs);
+ bool autoSave(const bool force, const size_t waitTimeoutMs, std::unique_lock<std::mutex>& lock);
Poco::URI getPublicUri() const { return _uriPublic; }
Poco::URI getJailedUri() const { return _uriJailed; }
@@ -206,7 +206,7 @@ public:
bool isAlive() const { return _childProcess && _childProcess->isAlive(); }
size_t getSessionsCount() const
{
- std::lock_guard<std::mutex> lock(_mutex);
+ Util::assertIsLocked(_mutex);
return _sessions.size();
}
@@ -273,6 +273,8 @@ public:
/// Get the PID of the associated child process
Poco::Process::PID getPid() const { return _childProcess->getPid(); }
+ std::unique_lock<std::mutex> getLock() { return std::unique_lock<std::mutex>(_mutex); }
+
private:
/// Sends the .uno:Save command to LoKit.
bool sendUnoSave(const bool dontSaveIfUnmodified);
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 9b91e1c..7ec35a1 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -271,6 +271,8 @@ static void forkChildren(const int number)
/// Returns true if removed at least one.
static bool cleanupChildren()
{
+ Util::assertIsLocked(NewChildrenMutex);
+
bool removed = false;
for (int i = NewChildren.size() - 1; i >= 0; --i)
{
@@ -558,6 +560,7 @@ private:
}
docBrokersLock.lock();
+ auto docLock = docBroker->getLock();
sessionsCount = docBroker->removeSession(id);
if (sessionsCount == 0)
{
@@ -723,93 +726,92 @@ private:
cleanupDocBrokers();
// Lookup this document.
- auto it = DocBrokers.find(docKey);
- if (it != DocBrokers.end())
+ auto it = DocBrokers.lower_bound(docKey);
+ if (it != DocBrokers.end() && it->first == docKey)
{
// Get the DocumentBroker from the Cache.
Log::debug("Found DocumentBroker for docKey [" + docKey + "].");
docBroker = it->second;
assert(docBroker);
- }
- else
- {
- // New Document.
-#if MAX_DOCUMENTS > 0
- if (DocBrokers.size() + 1 > MAX_DOCUMENTS)
- {
- Log::error() << "Limit on maximum number of open documents of "
- << MAX_DOCUMENTS << " reached." << Log::end;
- shutdownLimitReached(*ws);
- return;
- }
-#endif
-
- // Store a dummy (marked to destroy) document broker until we
- // have the real one, so that the other requests block
- Log::debug("Inserting a dummy DocumentBroker for docKey [" + docKey + "] temporarily.");
- std::shared_ptr<DocumentBroker> tempBroker = std::make_shared<DocumentBroker>();
- DocBrokers.emplace(docKey, tempBroker);
- }
-
- docBrokersLock.unlock();
-
- if (docBroker && docBroker->isMarkedToDestroy())
- {
- // If this document is going out, wait.
- Log::debug("Document [" + docKey + "] is marked to destroy, waiting to reload.");
-
- bool timedOut = true;
- for (size_t i = 0; i < COMMAND_TIMEOUT_MS / POLL_TIMEOUT_MS; ++i)
+ if (docBroker->isMarkedToDestroy())
{
- std::this_thread::sleep_for(std::chrono::milliseconds(POLL_TIMEOUT_MS));
+ // Let the waiting happen in parallel to new requests.
+ docBrokersLock.unlock();
+
+ // If this document is going out, wait.
+ Log::debug("Document [" + docKey + "] is marked to destroy, waiting to reload.");
- std::unique_lock<std::mutex> lock(DocBrokersMutex);
- it = DocBrokers.find(docKey);
- if (it == DocBrokers.end())
+ bool timedOut = true;
+ for (size_t i = 0; i < COMMAND_TIMEOUT_MS / POLL_TIMEOUT_MS; ++i)
{
- // went away successfully
- docBroker.reset();
- Log::debug("Inserting a dummy DocumentBroker for docKey [" + docKey + "] temporarily after the other instance is gone.");
+ std::this_thread::sleep_for(std::chrono::milliseconds(POLL_TIMEOUT_MS));
- std::shared_ptr<DocumentBroker> tempBroker = std::make_shared<DocumentBroker>();
- DocBrokers.emplace(docKey, tempBroker);
+ docBrokersLock.lock();
+ it = DocBrokers.find(docKey);
+ if (it == DocBrokers.end())
+ {
+ // went away successfully
+ docBroker.reset();
+ docBrokersLock.unlock();
+ timedOut = false;
+ break;
+ }
+ else if (it->second && !it->second->isMarkedToDestroy())
+ {
+ // was actually replaced by a real document
+ docBroker = it->second;
+ docBrokersLock.unlock();
+ timedOut = false;
+ break;
+ }
- timedOut = false;
- break;
+ docBrokersLock.unlock();
+ if (TerminationFlag)
+ {
+ Log::error("Termination flag set. Not loading new session [" + id + "]");
+ return;
+ }
}
- else if (it->second && !it->second->isMarkedToDestroy())
+
+ if (timedOut)
{
- // was actually replaced by a real document
- docBroker = it->second;
- timedOut = false;
- break;
+ // Still here, but marked to destroy. Proceed and hope to recover.
+ Log::error("Timed out while waiting for document to unload before loading.");
}
- if (TerminationFlag)
+ // Retake the lock and recheck if another thread created the DocBroker.
+ docBrokersLock.lock();
+ it = DocBrokers.lower_bound(docKey);
+ if (it != DocBrokers.end() && it->first == docKey)
{
- Log::error("Termination flag set. Not loading new session [" + id + "]");
- return;
+ // Get the DocumentBroker from the Cache.
+ Log::debug("Found DocumentBroker for docKey [" + docKey + "].");
+ docBroker = it->second;
+ assert(docBroker);
}
}
-
- if (timedOut)
- {
- // Still here, but marked to destroy. Proceed and hope to recover.
- Log::error("Timed out while waiting for document to unload before loading.");
- }
}
+ Util::assertIsLocked(docBrokersLock);
+
if (TerminationFlag)
{
Log::error("Termination flag set. No loading new session [" + id + "]");
return;
}
- bool newDoc = false;
if (!docBroker)
{
- newDoc = true;
+#if MAX_DOCUMENTS > 0
+ if (DocBrokers.size() + 1 > MAX_DOCUMENTS)
+ {
+ Log::error("Maximum number of open documents reached.");
+ shutdownLimitReached(*ws);
+ return;
+ }
+#endif
+
// Request a kit process for this doc.
auto child = getNewChild();
if (!child)
@@ -823,28 +825,22 @@ private:
Log::debug("New DocumentBroker for docKey [" + docKey + "].");
docBroker = std::make_shared<DocumentBroker>(uriPublic, docKey, LOOLWSD::ChildRoot, child);
child->setDocumentBroker(docBroker);
+ DocBrokers.insert(it, std::make_pair(docKey, docBroker));
}
+ docBrokersLock.unlock();
+
// Validate the broker.
if (!docBroker || !docBroker->isAlive())
{
- Log::error("DocBroker is invalid or premature termination of child process. Service Unavailable.");
- if (!newDoc)
- {
- // Remove.
- std::unique_lock<std::mutex> lock(DocBrokersMutex);
- DocBrokers.erase(docKey);
- }
+ // Cleanup later.
+ Log::error("DocBroker is invalid or premature termination of child "
+ "process. Service Unavailable.");
+ DocBrokers.erase(docKey);
throw WebSocketErrorMessageException(SERVICE_UNAVAILABLE_INTERNAL_ERROR);
}
- if (newDoc)
- {
- std::unique_lock<std::mutex> lock(DocBrokersMutex);
- DocBrokers[docKey] = docBroker;
- }
-
// Check if readonly session is required
bool isReadOnly = false;
for (const auto& param : uriPublic.getQueryParameters())
@@ -893,7 +889,7 @@ private:
// Connection terminated. Destroy session.
Log::debug("Client session [" + id + "] terminated. Cleaning up.");
{
- std::unique_lock<std::mutex> DocBrokersLock(DocBrokersMutex);
+ auto docLock = docBroker->getLock();
// We cannot destroy it, before save, if this is the last session.
// Otherwise, we may end up removing the one and only session.
@@ -918,7 +914,7 @@ private:
// We need to wait until the save notification reaches us
// and Storage persists the document.
- if (!docBroker->autoSave(forceSave, COMMAND_TIMEOUT_MS))
+ if (!docBroker->autoSave(forceSave, COMMAND_TIMEOUT_MS, docLock))
{
Log::error("Auto-save before closing failed.");
}
@@ -2023,7 +2019,8 @@ int LOOLWSD::main(const std::vector<std::string>& /*args*/)
cleanupDocBrokers();
for (auto& pair : DocBrokers)
{
- pair.second->autoSave(false, 0);
+ auto docLock = pair.second->getLock();
+ pair.second->autoSave(false, 0, docLock);
}
}
catch (const std::exception& exc)
More information about the Libreoffice-commits
mailing list