From 702dd48f1f7f671d918fbadcf98fb4ae4743dbbe Mon Sep 17 00:00:00 2001 From: Ashod Nakashian Date: Sun, 8 May 2016 10:07:17 -0400 Subject: [PATCH] loolwsd: safer document saving Relying on the filesystem to tell us when the document was last modified (to decide whether to upload to storage or not,) proved unreliable. Now we always upload to storage if there is only one client. This both minimizes the risk and also avoids the file timestamp check as a workaround to the problem of re-uploading documents as many time as there were clients. Since with one client we can only upload no more than once per save, which is reasonable. Furthermore, when a client disconnects we auto-save automatically as a matter of precaution. However, when there are other clients still connected, we don't wait for the save to complete, rather we let that job to the very last one. Change-Id: I94a2e4bddaed30a6c9c0e69f8006667d33c5b8ee Reviewed-on: https://gerrit.libreoffice.org/24767 Reviewed-by: Ashod Nakashian Tested-by: Ashod Nakashian --- loolwsd/DocumentBroker.cpp | 16 ++++++++-------- loolwsd/LOOLWSD.cpp | 12 ++++++++---- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/loolwsd/DocumentBroker.cpp b/loolwsd/DocumentBroker.cpp index 4c8591a92..adde590f4 100644 --- a/loolwsd/DocumentBroker.cpp +++ b/loolwsd/DocumentBroker.cpp @@ -177,7 +177,9 @@ bool DocumentBroker::save() const auto uri = _uriPublic.toString(); - // If the file hasn't been modified within the past 10 seconds, skip saving. + // If we aren't potentially destroying just yet, and the file hasn't been + // modified within the past 10 seconds, skip saving. + // // FIXME this is because currently the ChildProcessSession broadcasts the // unocommandresult, so we get called several times here, and have no real // possibility to distinguish who was the 1st caller. @@ -187,10 +189,11 @@ bool DocumentBroker::save() // is planned post-release. const auto newFileModifiedTime = Poco::File(_storage->getLocalRootPath()).getLastModified(); const auto elapsed = newFileModifiedTime - _lastFileModifiedTime; - if (std::abs(elapsed) > 10 * 1000 * 1000) + if (!canDestroy() && std::abs(elapsed) > 10 * 1000 * 1000) { // Nothing to do. - Log::debug("Skipping unnecessary saving to URI [" + uri + "]."); + Log::debug() << "Skipping unnecessary saving to URI [" << uri + << "]. File last modified " << elapsed << " ms ago." << Log::end; return true; } @@ -502,11 +505,8 @@ bool DocumentBroker::canDestroy() { std::unique_lock lock(_mutex); - if (_sessions.size() == 1) - { - // Last view going away, can destroy. - _markToDestroy = true; - } + // Last view going away, can destroy. + _markToDestroy = (_sessions.size() <= 1); return _markToDestroy; } diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp index 37c71a9fb..25a38012b 100644 --- a/loolwsd/LOOLWSD.cpp +++ b/loolwsd/LOOLWSD.cpp @@ -648,15 +648,19 @@ private: [&session]() { session->closeFrame(); }, [&queueHandlerThread]() { return TerminationFlag || !queueHandlerThread.isRunning(); }); - const bool canDestroy = docBroker->canDestroy(); - if (canDestroy && !session->_bLoadError) + if (!session->_bLoadError) { - Log::info("Shutdown of the last session, saving the document before tearing down."); + // If we are the last, we must wait for the save to complete. + const bool canDestroy = docBroker->canDestroy(); + if (canDestroy) + { + Log::info("Shutdown of the last session, saving the document before tearing down."); + } // Use auto-save to save only when there are modifications since last save. // We also need to wait until the save notification reaches us // and Storage persists the document. - if (!docBroker->autoSave(true, COMMAND_TIMEOUT_MS)) + if (!docBroker->autoSave(canDestroy, COMMAND_TIMEOUT_MS)) { Log::error("Auto-save before closing failed."); }