[Libreoffice-commits] online.git: 2 commits - wsd/ClientSession.cpp wsd/DocumentBroker.cpp wsd/DocumentBroker.hpp wsd/Storage.hpp
Ashod Nakashian
ashod.nakashian at collabora.co.uk
Mon Jan 15 14:02:45 UTC 2018
wsd/ClientSession.cpp | 4 --
wsd/DocumentBroker.cpp | 87 +++++++++++++++++++++----------------------------
wsd/DocumentBroker.hpp | 20 +++++++++--
wsd/Storage.hpp | 2 -
4 files changed, 56 insertions(+), 57 deletions(-)
New commits:
commit 8ad29ee05006899a256016ed2ba00720dfc9b79f
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date: Sun Jan 14 18:59:11 2018 -0500
wsd: separate lastSaveTime from lastSaveResponseTime
Previously we assumed we are saving based on
lastSaveTime, which is incorrect because it
is set only upon successful saving and storing,
which might fail.
Now lastSaveResponseTime is used to track whether
there are saving requests in flight. And lastSaveTime
is only used when we do store the document in storage.
Change-Id: I73e5c04432981d0cca11b8cf854414738bd894de
Reviewed-on: https://gerrit.libreoffice.org/47884
Reviewed-by: Michael Meeks <michael.meeks at collabora.com>
Tested-by: Michael Meeks <michael.meeks at collabora.com>
diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index bc363bc4..44574aab 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -258,7 +258,7 @@ void DocumentBroker::pollThread()
adminRecv = recv;
}
- if (_lastSaveTime < _lastSaveRequestTime &&
+ if (isSaving() &&
std::chrono::duration_cast<std::chrono::milliseconds>
(now - _lastSaveRequestTime).count() <= COMMAND_TIMEOUT_MS)
{
@@ -698,6 +698,9 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
{
assertCorrectThread();
+ // Record that we got a response to avoid timeing out on saving.
+ _lastSaveResponseTime = std::chrono::steady_clock::now();
+
const bool isSaveAs = !saveAsPath.empty();
// If save requested, but core didn't save because document was unmodified
@@ -737,7 +740,6 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
// Nothing to do.
LOG_DBG("Skipping unnecessary saving to URI [" << uri << "] with docKey [" << _docKey <<
"]. File last modified " << _lastFileModifiedTime.elapsed() / 1000000 << " seconds ago.");
- _lastSaveTime = std::chrono::steady_clock::now();
_poll->wakeup();
return true;
}
@@ -750,13 +752,13 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
{
if (!isSaveAs)
{
+ // Saved and stored; update flags.
setModified(false);
_lastFileModifiedTime = newFileModifiedTime;
_tileCache->saveLastModified(_lastFileModifiedTime);
_lastSaveTime = std::chrono::steady_clock::now();
- _poll->wakeup();
- // So set _documentLastModifiedTime then
+ // Save the storage timestamp.
_documentLastModifiedTime = _storage->getFileInfo()._modifiedTime;
// After a successful save, we are sure that document in the storage is same as ours
@@ -765,6 +767,9 @@ bool DocumentBroker::saveToStorageInternal(const std::string& sessionId,
Log::debug() << "Saved docKey [" << _docKey << "] to URI [" << uri
<< "] and updated tile cache. Document modified timestamp: "
<< _documentLastModifiedTime << Log::end;
+
+ // Resume polling.
+ _poll->wakeup();
}
else
{
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index 18c6e624..0b8153ad 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -364,6 +364,9 @@ 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 a save is in progress (requested but not completed).
+ bool isSaving() const { return _lastSaveResponseTime < _lastSaveRequestTime; }
+
/// 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
@@ -408,14 +411,19 @@ private:
/// document was modified and saved or not.
std::chrono::steady_clock::time_point _lastSaveTime;
- /// The last time we sent a save request.
+ /// The last time we sent a save request to lokit.
std::chrono::steady_clock::time_point _lastSaveRequestTime;
+ /// The last time we received a response for a save request from lokit.
+ std::chrono::steady_clock::time_point _lastSaveResponseTime;
+
/// The document's last-modified time on storage.
Poco::Timestamp _documentLastModifiedTime;
/// The jailed file last-modified time.
Poco::Timestamp _lastFileModifiedTime;
+
+ /// All session of this DocBroker by ID.
std::map<std::string, std::shared_ptr<ClientSession> > _sessions;
std::unique_ptr<StorageBase> _storage;
diff --git a/wsd/Storage.hpp b/wsd/Storage.hpp
index c4aff64f..e55e3047 100644
--- a/wsd/Storage.hpp
+++ b/wsd/Storage.hpp
@@ -137,7 +137,7 @@ public:
{
// Could assert here that it is in the same directory?
_jailedFilePath = newPath;
- };
+ }
bool isLoaded() const { return _isLoaded; }
commit a3a8551f2dbce4cef978b1a8373cd7413f422517
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>
diff --git a/wsd/ClientSession.cpp b/wsd/ClientSession.cpp
index 9fdf8ba7..061504c3 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 deb2cfc9..bc363bc4 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),
@@ -731,10 +730,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 <<
@@ -1004,12 +1002,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.
@@ -1031,21 +1023,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)
{
@@ -1381,40 +1383,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)
@@ -1533,7 +1518,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)
{
@@ -1645,7 +1630,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