[Libreoffice-commits] online.git: Branch 'distro/collabora/collabora-online-3-0' - wsd/ClientSession.cpp wsd/DocumentBroker.cpp wsd/DocumentBroker.hpp
Ashod Nakashian
ashod.nakashian at collabora.co.uk
Mon Jan 22 16:37:15 UTC 2018
wsd/ClientSession.cpp | 4 --
wsd/DocumentBroker.cpp | 74 +++++++++++++++++++------------------------------
wsd/DocumentBroker.hpp | 10 ++++--
3 files changed, 37 insertions(+), 51 deletions(-)
New commits:
commit 41e8a526b798226c27beb7be93d39ef9397ca58c
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date: Sun Jan 14 17:17:00 2018 -0500
wsd: remove lastEditableSession flag
Maintaining a flag is problematic, at least the way
it was reset on adding new sessions. Luckily there
should be no reason for having it, since we should
check it only when removing sessions. Also, we
need to check for every case of removal, and not, as
was, just on disconnection, which is incomplete.
Change-Id: I878766701228c41fc93eeaff21852fa887de9eff
Reviewed-on: https://gerrit.libreoffice.org/47883
Reviewed-by: Michael Meeks <michael.meeks at collabora.com>
Tested-by: Michael Meeks <michael.meeks at collabora.com>
(cherry picked from commit a3a8551f2dbce4cef978b1a8373cd7413f422517)
Reviewed-on: https://gerrit.libreoffice.org/48341
Reviewed-by: Jan Holesovsky <kendy at collabora.com>
Tested-by: Jan Holesovsky <kendy at collabora.com>
diff --git a/wsd/ClientSession.cpp b/wsd/ClientSession.cpp
index 10fe42f5..4a596c6e 100644
--- a/wsd/ClientSession.cpp
+++ b/wsd/ClientSession.cpp
@@ -872,9 +872,7 @@ void ClientSession::onDisconnect()
// Connection terminated. Destroy session.
LOG_DBG(getName() << " on docKey [" << docKey << "] terminated. Cleaning up.");
- // We issue a force-save when last editable (non-readonly) session is going away
- // and defer destroying the last session and the docBroker.
- docBroker->removeSession(getId(), true);
+ docBroker->removeSession(getId());
}
catch (const UnauthorizedRequestException& exc)
{
diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 189b6091..67c406b4 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -152,7 +152,6 @@ DocumentBroker::DocumentBroker(const std::string& uri,
_lastSaveTime(std::chrono::steady_clock::now()),
_lastSaveRequestTime(std::chrono::steady_clock::now() - std::chrono::milliseconds(COMMAND_TIMEOUT_MS)),
_markToDestroy(false),
- _lastEditableSession(false),
_isLoaded(false),
_isModified(false),
_cursorPosX(0),
@@ -726,10 +725,9 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
const Authorization auth = it->second->getAuthorization();
const auto uri = isSaveAs? saveAsPath: it->second->getPublicUri().toString();
- // If we aren't destroying the last editable session just yet,
- // and the file timestamp hasn't changed, skip saving.
+ // If the file timestamp hasn't changed, skip saving.
const auto newFileModifiedTime = Poco::File(_storage->getRootFilePath()).getLastModified();
- if (!isSaveAs && !_lastEditableSession && newFileModifiedTime == _lastFileModifiedTime)
+ if (!isSaveAs && newFileModifiedTime == _lastFileModifiedTime)
{
// Nothing to do.
LOG_DBG("Skipping unnecessary saving to URI [" << uri << "] with docKey [" << _docKey <<
@@ -998,12 +996,6 @@ size_t DocumentBroker::addSessionInternal(const std::shared_ptr<ClientSession>&
throw;
}
- // Below values are recalculated when destroyIfLastEditor() is called (before destroying the
- // document). It is safe to reset their values to their defaults whenever a new session is added.
- _lastEditableSession = false;
- _markToDestroy = false;
- _stop = false;
-
const auto id = session->getId();
// Request a new session from the child kit.
@@ -1025,21 +1017,31 @@ size_t DocumentBroker::addSessionInternal(const std::shared_ptr<ClientSession>&
return count;
}
-size_t DocumentBroker::removeSession(const std::string& id, bool destroyIfLast)
+size_t DocumentBroker::removeSession(const std::string& id)
{
assertCorrectThread();
- if (destroyIfLast)
- destroyIfLastEditor(id);
-
try
{
+ const auto it = _sessions.find(id);
+ if (it == _sessions.end())
+ {
+ LOG_ERR("Invalid or unknown session [" << id << "] to remove.");
+ return _sessions.size();
+ }
+
+ // Last view going away, can destroy.
+ _markToDestroy = (_sessions.size() <= 1);
+
+ const bool lastEditableSession = !it->second->isReadOnly() && !haveAnotherEditableSession(id);
+
LOG_INF("Removing session [" << id << "] on docKey [" << _docKey <<
"]. Have " << _sessions.size() << " sessions. markToDestroy: " << _markToDestroy <<
- ", LastEditableSession: " << _lastEditableSession);
+ ", LastEditableSession: " << lastEditableSession);
- if (!_lastEditableSession || !autoSave(true))
- return removeSessionInternal(id);
+ // If last editable, save and don't remove until after uploading to storage.
+ if (!lastEditableSession || !autoSave(true))
+ removeSessionInternal(id);
}
catch (const std::exception& ex)
{
@@ -1375,40 +1377,23 @@ void DocumentBroker::handleTileCombinedResponse(const std::vector<char>& payload
}
}
-void DocumentBroker::destroyIfLastEditor(const std::string& id)
+bool DocumentBroker::haveAnotherEditableSession(const std::string& id) const
{
assertCorrectThread();
- const auto currentSession = _sessions.find(id);
- if (currentSession == _sessions.end())
+ for (const auto& it : _sessions)
{
- // We could be called before adding any sessions.
- // For example when a socket disconnects before loading.
- return;
- }
-
- // Check if the session being destroyed is the last non-readonly session or not.
- _lastEditableSession = !currentSession->second->isReadOnly();
- if (_lastEditableSession && !_sessions.empty())
- {
- for (const auto& it : _sessions)
+ if (it.second->getId() != id &&
+ it.second->isViewLoaded() &&
+ !it.second->isReadOnly())
{
- if (it.second->getId() != id &&
- it.second->isViewLoaded() &&
- !it.second->isReadOnly())
- {
- // Found another editable.
- _lastEditableSession = false;
- break;
- }
+ // This is a loaded session that is non-readonly.
+ return true;
}
}
- // Last view going away, can destroy.
- _markToDestroy = (_sessions.size() <= 1);
- LOG_DBG("startDestroy on session [" << id << "] on docKey [" << _docKey <<
- "], sessions: " << _sessions.size() << " markToDestroy: " << _markToDestroy <<
- ", lastEditableSession: " << _lastEditableSession);
+ // None found.
+ return false;
}
void DocumentBroker::setModified(const bool value)
@@ -1527,7 +1512,7 @@ void DocumentBroker::shutdownClients(const std::string& closeReason)
session->shutdown(WebSocketHandler::StatusCodes::ENDPOINT_GOING_AWAY, closeReason);
// Remove session, save, and mark to destroy.
- removeSession(session->getId(), true);
+ removeSession(session->getId());
}
catch (const std::exception& exc)
{
@@ -1639,7 +1624,6 @@ void DocumentBroker::dumpState(std::ostream& os)
os << "\n doc key: " << _docKey;
os << "\n doc id: " << _docId;
os << "\n num sessions: " << _sessions.size();
- os << "\n last editable?: " << _lastEditableSession;
const std::time_t t = std::chrono::system_clock::to_time_t(
std::chrono::system_clock::now()
+ (_lastSaveTime - std::chrono::steady_clock::now()));
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index 4b4b458d..18c6e624 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -277,7 +277,7 @@ public:
size_t addSession(const std::shared_ptr<ClientSession>& session);
/// Removes a session by ID. Returns the new number of sessions.
- size_t removeSession(const std::string& id, bool destroyIfLast = false);
+ size_t removeSession(const std::string& id);
/// Add a callback to be invoked in our polling thread.
void addCallback(const SocketPoll::CallbackFn& fn);
@@ -312,7 +312,6 @@ public:
void handleDialogPaintResponse(const std::vector<char>& payload, bool child);
void handleTileCombinedResponse(const std::vector<char>& payload);
- void destroyIfLastEditor(const std::string& id);
bool isMarkedToDestroy() const { return _markToDestroy || _stop; }
bool handleInput(const std::vector<char>& payload);
@@ -365,6 +364,12 @@ private:
/// Saves the doc to the storage.
bool saveToStorageInternal(const std::string& sesionId, bool success, const std::string& result = "", const std::string& saveAsPath = std::string(), const std::string& saveAsFilename = std::string());
+ /// True iff there is at least one non-readonly session other than the given.
+ /// Since only editable sessions can save, we need to use the last to
+ /// save modified documents, otherwise we'll potentially have to save on
+ /// every editable session disconnect, lest we lose data due to racing.
+ bool haveAnotherEditableSession(const std::string& id) const;
+
/// Loads a new session and adds to the sessions container.
size_t addSessionInternal(const std::shared_ptr<ClientSession>& session);
@@ -416,7 +421,6 @@ private:
std::unique_ptr<StorageBase> _storage;
std::unique_ptr<TileCache> _tileCache;
std::atomic<bool> _markToDestroy;
- std::atomic<bool> _lastEditableSession;
std::atomic<bool> _isLoaded;
std::atomic<bool> _isModified;
int _cursorPosX;
More information about the Libreoffice-commits
mailing list