From b141677810c8c45d024ffb95f90bdbb0b26a128d Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 10 Mar 2024 18:45:03 +0400 Subject: [PATCH] Safer ContentManager::reallyCancelBook() Before this change `ContentManager::reallyCancelBook()` could throw (and thus result in a crash) if the confirmation to cancel the download was timed to perform the actual download cancellation on a completed download. This change fixes that problem. Besides it also fixes a subtle issue where `ContentManager::reallyCancelBook()` is called on a completed download and has the effect of removing the downloaded book from the local library. I considered preserving that dubious behaviour, but it couldn't be achieved in a simple and consistent fashion (because of the two ways an actually completed download can be seen by the `ContentManager::reallyCancelBook()` function). --- src/contentmanager.cpp | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 45acd73..818405d 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -722,10 +722,24 @@ void ContentManager::cancelBook(const QString& id) void ContentManager::reallyCancelBook(const QString& id) { - auto& b = mp_library->getBookById(id); - auto download = mp_downloader->getDownload(b.getDownloadId()); - if (download->getStatus() != kiwix::Download::K_COMPLETE) { + const auto downloadId = mp_library->getBookById(id).getDownloadId(); + if ( downloadId.empty() ) { + // Completion of the download has been detected (and its id was reset) + // before the confirmation to cancel the download was granted. + return; + } + + auto download = mp_downloader->getDownload(downloadId); + try { download->cancelDownload(); + } catch (const kiwix::AriaError&) { + // Download has completed before the cancel request was handled. + // Most likely the download was already complete at the time + // when ContentManager::reallyCancelBook() started executing, but + // its completion was not yet detected (and/or handled) by the + // download updater thread (letting the code pass past the empty + // downloadId check above). + return; } removeDownload(id);