diff --git a/platform/country_defines.cpp b/platform/country_defines.cpp index 927f883709..8ee9691ae5 100644 --- a/platform/country_defines.cpp +++ b/platform/country_defines.cpp @@ -8,6 +8,11 @@ bool HasOptions(TMapOptions mask, TMapOptions options) static_cast(options); } +TMapOptions IntersectOptions(TMapOptions lhs, TMapOptions rhs) +{ + return static_cast(static_cast(lhs) & static_cast(rhs)); +} + TMapOptions SetOptions(TMapOptions mask, TMapOptions options) { return static_cast(static_cast(mask) | static_cast(options)); diff --git a/platform/country_defines.hpp b/platform/country_defines.hpp index 5c010cbd13..1273b8e04c 100644 --- a/platform/country_defines.hpp +++ b/platform/country_defines.hpp @@ -12,6 +12,8 @@ enum class TMapOptions : uint8_t bool HasOptions(TMapOptions mask, TMapOptions options); +TMapOptions IntersectOptions(TMapOptions lhs, TMapOptions rhs); + TMapOptions SetOptions(TMapOptions mask, TMapOptions options); TMapOptions UnsetOptions(TMapOptions mask, TMapOptions options); diff --git a/storage/storage.cpp b/storage/storage.cpp index 2ce3ff0dd3..cd317cbe50 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -321,7 +321,6 @@ void Storage::DeleteCountry(TIndex const & index, TMapOptions opt) opt = NormalizeDeleteFileSet(opt); DeleteCountryFiles(index, opt); DeleteCountryFilesFromDownloader(index, opt); - KickDownloaderAfterDeletionOfCountryFiles(index); NotifyStatusChanged(index); } @@ -359,7 +358,7 @@ void Storage::DeleteCustomCountryVersion(LocalCountryFile const & localFile) return; } - auto equalsToLocalFile = [&localFile](shared_ptr const & rhs) + auto const equalsToLocalFile = [&localFile](shared_ptr const & rhs) { return localFile == *rhs; }; @@ -397,7 +396,6 @@ bool Storage::DeleteFromDownloader(TIndex const & index) { if (!DeleteCountryFilesFromDownloader(index, TMapOptions::EMapWithCarRouting)) return false; - KickDownloaderAfterDeletionOfCountryFiles(index); NotifyStatusChanged(index); return true; } @@ -465,45 +463,10 @@ void Storage::OnMapFileDownloadFinished(bool success, return; } - { - string optionsName; - switch (queuedCountry.GetInitOptions()) - { - case TMapOptions::ENothing: - optionsName = "Nothing"; - break; - case TMapOptions::EMap: - optionsName = "Map"; - break; - case TMapOptions::ECarRouting: - optionsName = "CarRouting"; - break; - case TMapOptions::EMapWithCarRouting: - optionsName = "MapWithCarRouting"; - break; - } - alohalytics::LogEvent( - "$OnMapDownloadFinished", - alohalytics::TStringMap({{"name", GetCountryFile(index).GetNameWithoutExt()}, - {"status", success ? "ok" : "failed"}, - {"version", strings::to_string(GetCurrentDataVersion())}, - {"option", optionsName}})); - } - if (success) - { - shared_ptr localFile = GetLocalFile(index, GetCurrentDataVersion()); - ASSERT(localFile.get(), ()); - OnMapDownloadFinished(localFile); - } - else - { - OnMapDownloadFailed(); - } - + OnMapDownloadFinished(index, success, queuedCountry.GetInitOptions()); m_queue.pop_front(); NotifyStatusChanged(index); - m_downloader->Reset(); DownloadNextCountryFromQueue(); } @@ -575,22 +538,46 @@ bool Storage::RegisterDownloadedFile(string const & path, uint64_t size, int64_t return true; } -void Storage::OnMapDownloadFinished(shared_ptr localFile) +void Storage::OnMapDownloadFinished(TIndex const & index, bool success, TMapOptions opt) { - platform::CountryIndexes::DeleteFromDisk(*localFile); + ASSERT_NOT_EQUAL(TMapOptions::ENothing, opt, + ("This method should not be called for empty files set.")); + { + string optionsName; + switch (opt) + { + case TMapOptions::ENothing: + optionsName = "Nothing"; + break; + case TMapOptions::EMap: + optionsName = "Map"; + break; + case TMapOptions::ECarRouting: + optionsName = "CarRouting"; + break; + case TMapOptions::EMapWithCarRouting: + optionsName = "MapWithCarRouting"; + break; + } + alohalytics::LogEvent( + "$OnMapDownloadFinished", + alohalytics::TStringMap({{"name", GetCountryFile(index).GetNameWithoutExt()}, + {"status", success ? "ok" : "failed"}, + {"version", strings::to_string(GetCurrentDataVersion())}, + {"option", optionsName}})); + } - // Notify framework that all requested files for the country were downloaded. - m_updateAfterDownload(*localFile); -} - -void Storage::OnMapDownloadFailed() -{ - TIndex const & index = m_queue.front().GetIndex(); - - // Add country to the failed countries set. - m_failedCountries.insert(index); - m_downloader->Reset(); - DownloadNextCountryFromQueue(); + if (success) + { + shared_ptr localFile = GetLocalFile(index, GetCurrentDataVersion()); + ASSERT(localFile.get(), ()); + platform::CountryIndexes::DeleteFromDisk(*localFile); + m_updateAfterDownload(*localFile); + } + else + { + m_failedCountries.insert(index); + } } string Storage::GetFileDownloadUrl(string const & baseUrl, TIndex const & index, @@ -825,29 +812,46 @@ bool Storage::DeleteCountryFilesFromDownloader(TIndex const & index, TMapOptions QueuedCountry * queuedCountry = FindCountryInQueue(index); if (!queuedCountry) return false; + + opt = IntersectOptions(opt, queuedCountry->GetInitOptions()); + if (IsCountryFirstInQueue(index)) { // Abrupt downloading of the current file if it should be removed. if (HasOptions(opt, queuedCountry->GetCurrentFile())) m_downloader->Reset(); } + + // Remove already downloaded files. + if (shared_ptr localFile = GetLocalFile(index, GetCurrentDataVersion())) + { + localFile->DeleteFromDisk(opt); + localFile->SyncWithDisk(); + if (localFile->GetFiles() == TMapOptions::ENothing) + { + auto equalsToLocalFile = [&localFile](shared_ptr const & rhs) + { + return *localFile == *rhs; + }; + m_localFiles[index].remove_if(equalsToLocalFile); + } + } + queuedCountry->RemoveOptions(opt); // Remove country from the queue if there's nothing to download. if (queuedCountry->GetInitOptions() == TMapOptions::ENothing) m_queue.erase(find(m_queue.begin(), m_queue.end(), index)); - return true; -} -void Storage::KickDownloaderAfterDeletionOfCountryFiles(TIndex const & index) -{ - // Do nothing when there're no countries to download or when downloader is busy. - if (m_queue.empty() || !m_downloader->IsIdle()) - return; - if (IsCountryFirstInQueue(index)) - DownloadNextFile(m_queue.front()); - else - DownloadNextCountryFromQueue(); + if (!m_queue.empty() && m_downloader->IsIdle()) + { + // Kick possibly interrupted downloader. + if (IsCountryFirstInQueue(index)) + DownloadNextFile(m_queue.front()); + else + DownloadNextCountryFromQueue(); + } + return true; } uint64_t Storage::GetDownloadSize(QueuedCountry const & queuedCountry) const diff --git a/storage/storage.hpp b/storage/storage.hpp index d025e631a6..a8f8161852 100644 --- a/storage/storage.hpp +++ b/storage/storage.hpp @@ -92,8 +92,7 @@ class Storage void OnMapFileDownloadProgress(MapFilesDownloader::TProgress const & progress); bool RegisterDownloadedFile(string const & path, uint64_t size, int64_t version); - void OnMapDownloadFinished(shared_ptr localFile); - void OnMapDownloadFailed(); + void OnMapDownloadFinished(TIndex const & index, bool success, TMapOptions opt); /// Initiates downloading of the next file from the queue. void DownloadNextFile(QueuedCountry const & country); @@ -223,10 +222,6 @@ private: // Removes country files from downloader. bool DeleteCountryFilesFromDownloader(TIndex const & index, TMapOptions opt); - // Resumes possibly cancelled downloading of countries after - // deletion of country files. - void KickDownloaderAfterDeletionOfCountryFiles(TIndex const & index); - // Returns download size of the currently downloading file for the // queued country. uint64_t GetDownloadSize(QueuedCountry const & queuedCountry) const; diff --git a/storage/storage_tests/fake_map_files_downloader.cpp b/storage/storage_tests/fake_map_files_downloader.cpp index 0e72edde1b..752b36e691 100644 --- a/storage/storage_tests/fake_map_files_downloader.cpp +++ b/storage/storage_tests/fake_map_files_downloader.cpp @@ -2,8 +2,6 @@ #include "storage/storage_tests/task_runner.hpp" -#include "coding/file_writer.hpp" - #include "base/assert.hpp" #include "base/scope_guard.hpp" @@ -12,13 +10,10 @@ namespace storage { -namespace -{ -int64_t const kBlockSize = 1024 * 1024; -} // namespace +int64_t const FakeMapFilesDownloader::kBlockSize; FakeMapFilesDownloader::FakeMapFilesDownloader(TaskRunner & taskRunner) - : m_progress(make_pair(0, 0)), m_idle(true), m_taskRunner(taskRunner) + : m_progress(make_pair(0, 0)), m_idle(true), m_timestamp(0), m_taskRunner(taskRunner) { m_servers.push_back("http://test-url/"); } @@ -39,29 +34,23 @@ void FakeMapFilesDownloader::DownloadMapFile(vector const & urls, string TFileDownloadedCallback const & onDownloaded, TDownloadingProgressCallback const & onProgress) { - static string kZeroes(kBlockSize, '\0'); + CHECK(m_checker.CalledOnOriginalThread(), ()); m_progress.first = 0; m_progress.second = size; m_idle = false; - MY_SCOPE_GUARD(resetIdle, bind(&FakeMapFilesDownloader::Reset, this)); - { - FileWriter writer(path); - while (size != 0) - { - int64_t const blockSize = min(size, kBlockSize); - writer.Write(kZeroes.data(), blockSize); - size -= blockSize; - m_progress.first += blockSize; - m_taskRunner.PostTask(bind(onProgress, m_progress)); - } - } - m_taskRunner.PostTask(bind(onDownloaded, true /* success */, m_progress)); + m_writer.reset(new FileWriter(path)); + m_onDownloaded = onDownloaded; + m_onProgress = onProgress; + + ++m_timestamp; + m_taskRunner.PostTask(bind(&FakeMapFilesDownloader::DownloadNextChunk, this, m_timestamp)); } MapFilesDownloader::TProgress FakeMapFilesDownloader::GetDownloadingProgress() { + CHECK(m_checker.CalledOnOriginalThread(), ()); return m_progress; } @@ -75,6 +64,36 @@ void FakeMapFilesDownloader::Reset() { CHECK(m_checker.CalledOnOriginalThread(), ()); m_idle = true; + m_writer.reset(); + ++m_timestamp; } +void FakeMapFilesDownloader::DownloadNextChunk(uint64_t timestamp) +{ + CHECK(m_checker.CalledOnOriginalThread(), ()); + + static string kZeroes(kBlockSize, '\0'); + + if (timestamp != m_timestamp) + return; + + ASSERT_LESS_OR_EQUAL(m_progress.first, m_progress.second, ()); + ASSERT(m_writer, ()); + + if (m_progress.first == m_progress.second) + { + m_taskRunner.PostTask(bind(m_onDownloaded, true /* success */, m_progress)); + Reset(); + return; + } + + int64_t const bs = min(m_progress.second - m_progress.first, kBlockSize); + + m_progress.first += bs; + m_writer->Write(kZeroes.data(), bs); + m_writer->Flush(); + + m_taskRunner.PostTask(bind(m_onProgress, m_progress)); + m_taskRunner.PostTask(bind(&FakeMapFilesDownloader::DownloadNextChunk, this, timestamp)); +} } // namespace storage diff --git a/storage/storage_tests/fake_map_files_downloader.hpp b/storage/storage_tests/fake_map_files_downloader.hpp index cb69bcb2fa..bff370bb5b 100644 --- a/storage/storage_tests/fake_map_files_downloader.hpp +++ b/storage/storage_tests/fake_map_files_downloader.hpp @@ -1,7 +1,9 @@ #pragma once #include "storage/map_files_downloader.hpp" +#include "coding/file_writer.hpp" #include "base/thread_checker.hpp" +#include "std/unique_ptr.hpp" namespace storage { @@ -17,6 +19,8 @@ class TaskRunner; class FakeMapFilesDownloader : public MapFilesDownloader { public: + static int64_t const kBlockSize = 1024 * 1024; + FakeMapFilesDownloader(TaskRunner & taskRunner); virtual ~FakeMapFilesDownloader(); @@ -31,10 +35,18 @@ public: void Reset() override; private: + void DownloadNextChunk(uint64_t requestId); + vector m_servers; TProgress m_progress; bool m_idle; + unique_ptr m_writer; + TFileDownloadedCallback m_onDownloaded; + TDownloadingProgressCallback m_onProgress; + + uint64_t m_timestamp; + TaskRunner & m_taskRunner; ThreadChecker m_checker; }; diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index 9bc206024c..a5ec6086af 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -52,6 +52,12 @@ public: TEST(!m_transitionList.empty(), (m_countryFile)); } + virtual ~CountryDownloaderChecker() + { + TEST_EQUAL(m_currStatus + 1, m_transitionList.size(), (m_countryFile)); + m_storage.Unsubscribe(m_slot); + } + void StartDownload() { TEST_EQUAL(0, m_currStatus, (m_countryFile)); @@ -61,14 +67,8 @@ public: m_storage.DownloadCountry(m_index, m_files); } - virtual ~CountryDownloaderChecker() - { - TEST_EQUAL(m_currStatus + 1, m_transitionList.size(), (m_countryFile)); - m_storage.Unsubscribe(m_slot); - } - -private: - void OnCountryStatusChanged(TIndex const & index) +protected: + virtual void OnCountryStatusChanged(TIndex const & index) { if (index != m_index) return; @@ -86,7 +86,8 @@ private: } } - void OnCountryDownloadingProgress(TIndex const & index, LocalAndRemoteSizeT const & progress) + virtual void OnCountryDownloadingProgress(TIndex const & index, + LocalAndRemoteSizeT const & progress) { if (index != m_index) return; @@ -113,6 +114,38 @@ private: vector m_transitionList; }; +class CancelDownloadingWhenAlmostDoneChecker : public CountryDownloaderChecker +{ +public: + CancelDownloadingWhenAlmostDoneChecker(Storage & storage, TIndex const & index, + TaskRunner & runner) + : CountryDownloaderChecker(storage, index, TMapOptions::EMapWithCarRouting, + vector{TStatus::ENotDownloaded, TStatus::EDownloading, + TStatus::ENotDownloaded}), + m_runner(runner) + { + } + +protected: + // CountryDownloaderChecker overrides: + void OnCountryDownloadingProgress(TIndex const & index, + LocalAndRemoteSizeT const & progress) override + { + CountryDownloaderChecker::OnCountryDownloadingProgress(index, progress); + + // Cancel downloading when almost done. + if (progress.first + 2 * FakeMapFilesDownloader::kBlockSize >= progress.second) + { + m_runner.PostTask([&]() + { + m_storage.DeleteFromDownloader(m_index); + }); + } + } + + TaskRunner & m_runner; +}; + // Checks following state transitions: // NotDownloaded -> Downloading -> OnDisk. unique_ptr AbsentCountryDownloaderChecker(Storage & storage, @@ -484,7 +517,7 @@ UNIT_TEST(StorageTest_DownloadTwoCountriesAndDelete) unique_ptr venezuelaChecker = make_unique( storage, venezuelaIndex, TMapOptions::EMapWithCarRouting, vector{TStatus::ENotDownloaded, TStatus::EInQueue, TStatus::EDownloading, - TStatus::EDownloading, TStatus::EOnDisk}); + TStatus::EOnDisk}); uruguayChecker->StartDownload(); venezuelaChecker->StartDownload(); storage.DeleteCountry(uruguayIndex, TMapOptions::EMap); @@ -498,3 +531,24 @@ UNIT_TEST(StorageTest_DownloadTwoCountriesAndDelete) TEST(venezuelaFile.get(), ()); TEST_EQUAL(TMapOptions::EMap, venezuelaFile->GetFiles(), ()); } + +UNIT_TEST(StorageTest_CancelDownloadingWhenAlmostDone) +{ + Storage storage; + TaskRunner runner; + InitStorage(storage, runner); + + TIndex const index = storage.FindIndexByFile("Uruguay"); + TEST(index.IsValid(), ()); + storage.DeleteCountry(index, TMapOptions::EMapWithCarRouting); + MY_SCOPE_GUARD(cleanupUruguayFiles, + bind(&Storage::DeleteCountry, &storage, index, TMapOptions::EMapWithCarRouting)); + + { + CancelDownloadingWhenAlmostDoneChecker checker(storage, index, runner); + checker.StartDownload(); + runner.Run(); + } + shared_ptr file = storage.GetLatestLocalFile(index); + TEST(!file, (*file)); +}