From 2d7e4da181e5b35c1bd88027cef68c888ce7ae9e Mon Sep 17 00:00:00 2001 From: Ashod Nakashian Date: Tue, 1 Feb 2022 21:07:36 -0500 Subject: [PATCH] wsd: do not force uploading when in conflict When the document has been changed in storage, we should be extra careful not to inadvertently clobber it because we needed to force the upload for some unrelated reason. When in conflict, only a user can force clobbering. Change-Id: I498a6d1c86242b059ac722d3e48c31a04a79591b Signed-off-by: Ashod Nakashian --- wsd/DocumentBroker.cpp | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp index f677293fb..797a958aa 100644 --- a/wsd/DocumentBroker.cpp +++ b/wsd/DocumentBroker.cpp @@ -1081,6 +1081,12 @@ bool DocumentBroker::attemptLock(const ClientSession& session, std::string& fail DocumentBroker::NeedToUpload DocumentBroker::needToUploadToStorage() const { + if (_storage == nullptr) + { + // This can happen when we reject the connection (unauthorized). + return NeedToUpload::No; + } + // When destroying, we might have to force uploading if always_save_on_exit=true. // If unloadRequested is set, assume we will unload after uploading and exit. if (isMarkedToDestroy() || _docState.isUnloadRequested()) @@ -1089,26 +1095,35 @@ DocumentBroker::NeedToUpload DocumentBroker::needToUploadToStorage() const = COOLWSD::getConfigValue("per_document.always_save_on_exit", false); if (always_save) { - LOG_INF("Need to upload per always_save_on_exit config (MarkedToDestroy=true)."); + if (_documentChangedInStorage) + { + LOG_WRN("Need to upload per always_save_on_exit config, but the document has a " + "conflict. Cannot force uploading."); + return NeedToUpload::Yes; // Still try. + } + + LOG_INF("Need to upload per always_save_on_exit config " + << (isMarkedToDestroy() ? "MarkedToDestroy" : "Unloading")); return NeedToUpload::Force; } } - if (!_storageManager.lastUploadSuccessful()) + // Force uploading only for retryable failures, not conflicts. See FIXME below. + if (!_storageManager.lastUploadSuccessful() && !_documentChangedInStorage) { //FIXME: Forcing is used when overwriting a 'document conflict' and // for uploading when otherwise we might not have an immediate // reason. We shouldn't use a single flag for both these uses. + if (_documentChangedInStorage) + { + LOG_WRN("Last upload failed due to a conflict. Cannot force uploading."); + return NeedToUpload::Yes; // Still try. + } + LOG_INF("Enabling forced uploading to storage as last attempt had failed."); return NeedToUpload::Force; } - if (_storage == nullptr) - { - // This can happen when we reject the connection (unauthorized). - return NeedToUpload::No; - } - // Get the modified-time of the file on disk. const auto st = FileUtil::Stat(_storage->getRootFilePathUploading()); const std::chrono::system_clock::time_point currentModifiedTime = st.modifiedTimepoint();