From 0f9a4fea92130a76d51ea9b296714c5c2c447659 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Tue, 13 Oct 2015 18:18:53 +0300 Subject: [PATCH] [storage, platform] Fixed abrupted downloading. --- defines.hpp | 4 +- map/framework.cpp | 2 +- platform/local_country_file_utils.cpp | 83 ++++++++---- platform/local_country_file_utils.hpp | 10 +- .../local_country_file_tests.cpp | 29 ++-- storage/storage.cpp | 42 +++++- storage/storage.hpp | 1 + storage/storage_tests/storage_tests.cpp | 127 +++++++++++++++--- storage/storage_tests/storage_tests.pro | 13 +- tools/ResponseProvider.py | 3 + tools/run_desktop_tests.py | 5 +- 11 files changed, 253 insertions(+), 66 deletions(-) diff --git a/defines.hpp b/defines.hpp index f955343247..0465b0c87d 100644 --- a/defines.hpp +++ b/defines.hpp @@ -36,8 +36,8 @@ #define ROUTING_NODEIND_TO_FTSEGIND_FILE_TAG "node2ftseg" #define READY_FILE_EXTENSION ".ready" -#define RESUME_FILE_EXTENSION ".resume3" -#define DOWNLOADING_FILE_EXTENSION ".downloading3" +#define RESUME_FILE_EXTENSION ".resume" +#define DOWNLOADING_FILE_EXTENSION ".downloading" #define BOOKMARKS_FILE_EXTENSION ".kml" #define ROUTING_FILE_EXTENSION ".routing" diff --git a/map/framework.cpp b/map/framework.cpp index 111bf62cb6..0a13bf3a83 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -470,7 +470,7 @@ void Framework::RegisterAllMaps() ("Registering maps while map downloading leads to removing downloading maps from " "ActiveMapsListener::m_items.")); - platform::CleanupMapsDirectory(); + platform::CleanupMapsDirectory(m_storage.GetCurrentDataVersion()); m_storage.RegisterAllLocalMaps(); int minFormat = numeric_limits::max(); diff --git a/platform/local_country_file_utils.cpp b/platform/local_country_file_utils.cpp index 86286867e0..ab7d6f99c4 100644 --- a/platform/local_country_file_utils.cpp +++ b/platform/local_country_file_utils.cpp @@ -8,12 +8,14 @@ #include "base/string_utils.hpp" #include "base/logging.hpp" +#include "base/regexp.hpp" #include "std/algorithm.hpp" #include "std/cctype.hpp" #include "std/sstream.hpp" #include "std/unique_ptr.hpp" +#include "defines.hpp" namespace platform { @@ -27,6 +29,13 @@ size_t const kMaxTimestampLength = 18; bool IsSpecialFile(string const & file) { return file == "." || file == ".."; } +bool IsEmptyDirectory(string const & directory) +{ + Platform::FilesList files; + Platform::GetFilesByRegExp(directory, ".*", files); + return all_of(files.begin(), files.end(), &IsSpecialFile); +} + bool GetFileTypeChecked(string const & path, Platform::EFileType & type) { Platform::EError const ret = Platform::GetFileType(path, type); @@ -72,24 +81,34 @@ string GetSpecialFilesSearchScope() return "r"; #endif // defined(OMIM_OS_ANDROID) } + +void DeleteDownloaderFilesForAllCountries(string const & directory) +{ + static string const regexp = "\\.(downloading|resume|ready)[0-9]?$"; + Platform::FilesList files; + Platform::GetFilesByRegExp(directory, regexp, files); + for (auto const & file : files) + my::DeleteFileX(my::JoinFoldersToPath(directory, file)); +} } // namespace -void CleanupMapsDirectory() +void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t version) +{ + for (MapOptions file : {MapOptions::Map, MapOptions::CarRouting}) + { + string const path = GetFileDownloadPath(countryFile, file, version); + my::DeleteFileX(path); + my::DeleteFileX(path + RESUME_FILE_EXTENSION); + my::DeleteFileX(path + DOWNLOADING_FILE_EXTENSION); + } +} + +void CleanupMapsDirectory(int64_t latestVersion) { Platform & platform = GetPlatform(); string const mapsDir = platform.WritableDir(); - // Remove partially downloaded maps. - { - Platform::FilesList files; - // .(downloading|resume|ready)[0-9]?$ - string const regexp = "\\.(downloading|resume|ready)[0-9]?$"; - platform.GetFilesByRegExp(mapsDir, regexp, files); - for (string const & file : files) - my::DeleteFileX(my::JoinFoldersToPath(mapsDir, file)); - } - { // Delete Brazil.mwm and Japan.mwm maps, because they was replaces with // smaler regions after osrm routing implementation. @@ -111,20 +130,27 @@ void CleanupMapsDirectory() platform.GetFilesByType(mapsDir, Platform::FILE_TYPE_DIRECTORY, subdirs); for (string const & subdir : subdirs) { - int64_t version; - if (ParseVersion(subdir, version)) + // No need to visit parent directory. + if (subdir == "..") + continue; + + int64_t version = 0; + if (subdir != "." && !ParseVersion(subdir, version)) + continue; + + string const subdirPath = my::JoinFoldersToPath(mapsDir, subdir); + + // It's OK to remove all temprorary files for maps older than app. + if (version != latestVersion) + DeleteDownloaderFilesForAllCountries(subdirPath); + + // Remove subdirectory if it does not contain any files except "." and "..". + if (subdir != "." && IsEmptyDirectory(subdirPath)) { - vector files; - string const subdirPath = my::JoinFoldersToPath(mapsDir, subdir); - platform.GetFilesByType(subdirPath, - Platform::FILE_TYPE_REGULAR | Platform::FILE_TYPE_DIRECTORY, files); - if (all_of(files.begin(), files.end(), &IsSpecialFile)) - { - Platform::EError const ret = Platform::RmDir(subdirPath); - ASSERT_EQUAL(Platform::ERR_OK, ret, - ("Can't remove empty directory:", subdirPath, "error:", ret)); - UNUSED_VALUE(ret); - } + Platform::EError const ret = Platform::RmDir(subdirPath); + ASSERT_EQUAL(Platform::ERR_OK, ret, + ("Can't remove empty directory:", subdirPath, "error:", ret)); + UNUSED_VALUE(ret); } } @@ -232,6 +258,15 @@ shared_ptr PreparePlaceForCountryFiles(CountryFile const & cou return make_shared(directory, countryFile, version); } +string GetFileDownloadPath(CountryFile const & countryFile, MapOptions file, int64_t version) +{ + Platform & platform = GetPlatform(); + string const readyFile = countryFile.GetNameWithExt(file) + READY_FILE_EXTENSION; + if (version == 0) + return my::JoinFoldersToPath(platform.WritableDir(), readyFile); + return my::JoinFoldersToPath({platform.WritableDir(), strings::to_string(version)}, readyFile); +} + ModelReader * GetCountryReader(platform::LocalCountryFile const & file, MapOptions options) { Platform & platform = GetPlatform(); diff --git a/platform/local_country_file_utils.hpp b/platform/local_country_file_utils.hpp index 739a9d35ba..a283b3e830 100644 --- a/platform/local_country_file_utils.hpp +++ b/platform/local_country_file_utils.hpp @@ -11,10 +11,14 @@ class ModelReader; namespace platform { +// Removes all files downloader creates during downloading of a country. +void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t version); + // Removes partially downloaded maps, empty directories and old // (format v1) maps. Also, removes old (splitted) Japan and Brazil -// maps. -void CleanupMapsDirectory(); +// maps. |version| must be set to the latest data version this app can +// work with. +void CleanupMapsDirectory(int64_t latestVersion); // Finds all local map files in a directory. Version of these files is // passed as an argument. @@ -49,6 +53,8 @@ bool ParseVersion(string const & s, int64_t & version); shared_ptr PreparePlaceForCountryFiles(CountryFile const & countryFile, int64_t version); +string GetFileDownloadPath(CountryFile const & countryFile, MapOptions file, int64_t version); + ModelReader * GetCountryReader(LocalCountryFile const & file, MapOptions options); // An API for managing country indexes. diff --git a/platform/platform_tests/local_country_file_tests.cpp b/platform/platform_tests/local_country_file_tests.cpp index 478bfd35dd..fa320355b8 100644 --- a/platform/platform_tests/local_country_file_tests.cpp +++ b/platform/platform_tests/local_country_file_tests.cpp @@ -159,7 +159,7 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) TEST(Contains(localFiles, brazilLocalFile), (brazilLocalFile, localFiles)); TEST(Contains(localFiles, irelandLocalFile), (irelandLocalFile, localFiles)); - CleanupMapsDirectory(); + CleanupMapsDirectory(0 /* latestVersion */); japanLocalFile.SyncWithDisk(); TEST_EQUAL(MapOptions::Nothing, japanLocalFile.GetFiles(), ()); @@ -183,23 +183,36 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) UNIT_TEST(LocalCountryFile_CleanupPartiallyDownloadedFiles) { - ScopedFile toBeDeleted[] = {{"Ireland.mwm.ready", "Ireland"}, - {"Netherlands.mwm.routing.downloading2", "Netherlands"}, - {"Germany.mwm.ready3", "Germany"}, - {"UK_England.mwm.resume4", "UK"}}; - ScopedFile toBeKept[] = { - {"Italy.mwm", "Italy"}, {"Spain.mwm", "Spain map"}, {"Spain.mwm.routing", "Spain routing"}}; + ScopedDir oldDir("101009"); + ScopedDir latestDir("101010"); - CleanupMapsDirectory(); + ScopedFile toBeDeleted[] = { + {"Ireland.mwm.ready", "Ireland"}, + {"Netherlands.mwm.routing.downloading2", "Netherlands"}, + {"Germany.mwm.ready3", "Germany"}, + {"UK_England.mwm.resume4", "UK"}, + {my::JoinFoldersToPath(oldDir.GetRelativePath(), "Russia_Central.mwm.downloading"), + "Central Russia map"}}; + ScopedFile toBeKept[] = { + {"Italy.mwm", "Italy"}, + {"Spain.mwm", "Spain map"}, + {"Spain.mwm.routing", "Spain routing"}, + {my::JoinFoldersToPath(latestDir.GetRelativePath(), "Russia_Southern.mwm.downloading"), + "Southern Russia map"}}; + + CleanupMapsDirectory(101010 /* latestVersion */); for (ScopedFile & file : toBeDeleted) { TEST(!file.Exists(), (file)); file.Reset(); } + TEST(!oldDir.Exists(), (oldDir)); + oldDir.Reset(); for (ScopedFile & file : toBeKept) TEST(file.Exists(), (file)); + TEST(latestDir.Exists(), (latestDir)); } // Creates test-dir and following files: diff --git a/storage/storage.cpp b/storage/storage.cpp index a62e1ab292..dee802f990 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -23,7 +23,6 @@ #include "3party/Alohalytics/src/alohalytics.h" - using namespace downloader; using namespace platform; @@ -392,6 +391,20 @@ void Storage::DownloadNextCountryFromQueue() return; QueuedCountry & queuedCountry = m_queue.front(); + TIndex const & index = queuedCountry.GetIndex(); + + // It's not even possible to prepare directory for files before + // downloading. Mark this country as failed and switch to next + // country. + if (!PreparePlaceForCountryFiles(GetCountryFile(index), GetCurrentDataVersion())) + { + OnMapDownloadFinished(index, false /* success */, queuedCountry.GetInitOptions()); + NotifyStatusChanged(index); + m_queue.pop_front(); + DownloadNextCountryFromQueue(); + return; + } + DownloadNextFile(queuedCountry); // New status for the country, "Downloading" @@ -400,7 +413,20 @@ void Storage::DownloadNextCountryFromQueue() void Storage::DownloadNextFile(QueuedCountry const & country) { - CountryFile const & countryFile = GetCountryFile(country.GetIndex()); + TIndex const & index = country.GetIndex(); + CountryFile const & countryFile = GetCountryFile(index); + + string const filePath = GetFileDownloadPath(index, country.GetCurrentFile()); + uint64_t size; + + // It may happen that the file already was downloaded, so there're + // no need to request servers list and download file. Let's + // switch to next file. + if (GetPlatform().GetFileSizeByFullPath(filePath, size)) + { + OnMapFileDownloadFinished(true /* success */, MapFilesDownloader::TProgress(size, size)); + return; + } // send Country name for statistics m_downloader->GetServersList(GetCurrentDataVersion(), countryFile.GetNameWithoutExt(), @@ -747,6 +773,11 @@ void Storage::SetDownloaderForTesting(unique_ptr && download m_downloader = move(downloader); } +void Storage::SetCurrentDataVersionForTesting(int64_t currentVersion) +{ + m_currentVersion = currentVersion; +} + Storage::TLocalFilePtr Storage::GetLocalFile(TIndex const & index, int64_t version) const { auto const it = m_localFiles.find(index); @@ -831,6 +862,9 @@ bool Storage::DeleteCountryFilesFromDownloader(TIndex const & index, MapOptions // Abrupt downloading of the current file if it should be removed. if (HasOptions(opt, queuedCountry->GetCurrentFile())) m_downloader->Reset(); + + // Remove all files downloader had been created for a country. + DeleteDownloaderFilesForCountry(GetCountryFile(index), GetCurrentDataVersion()); } queuedCountry->RemoveOptions(opt); @@ -858,8 +892,6 @@ uint64_t Storage::GetDownloadSize(QueuedCountry const & queuedCountry) const string Storage::GetFileDownloadPath(TIndex const & index, MapOptions file) const { - Platform & platform = GetPlatform(); - CountryFile const & countryFile = GetCountryFile(index); - return platform.WritablePathForFile(countryFile.GetNameWithExt(file) + READY_FILE_EXTENSION); + return platform::GetFileDownloadPath(GetCountryFile(index), file, GetCurrentDataVersion()); } } // namespace storage diff --git a/storage/storage.hpp b/storage/storage.hpp index 2d3b69eb32..a76f709e30 100644 --- a/storage/storage.hpp +++ b/storage/storage.hpp @@ -179,6 +179,7 @@ public: inline int64_t GetCurrentDataVersion() const { return m_currentVersion; } void SetDownloaderForTesting(unique_ptr && downloader); + void SetCurrentDataVersionForTesting(int64_t currentVersion); private: friend void UnitTest_StorageTest_DeleteCountry(); diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index 733aa7c452..6ee91c7017 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -4,6 +4,7 @@ #include "storage/storage_defines.hpp" #include "storage/storage_tests/fake_map_files_downloader.hpp" #include "storage/storage_tests/task_runner.hpp" +#include "storage/storage_tests/test_map_files_downloader.hpp" #include "indexer/indexer_tests/test_mwm_set.hpp" @@ -23,11 +24,14 @@ #include "base/string_utils.hpp" #include "std/bind.hpp" +#include "std/condition_variable.hpp" #include "std/map.hpp" +#include "std/mutex.hpp" #include "std/shared_ptr.hpp" #include "std/unique_ptr.hpp" #include "std/vector.hpp" +#include using namespace platform; @@ -198,26 +202,6 @@ unique_ptr CancelledCountryDownloaderChecker(Storage & vector{TStatus::ENotDownloaded, TStatus::EDownloading, TStatus::ENotDownloaded}); } -void OnCountryDownloaded(LocalCountryFile const & localFile) -{ - LOG(LINFO, ("OnCountryDownloaded:", localFile)); -} - -TLocalFilePtr CreateDummyMapFile(CountryFile const & countryFile, int64_t version, size_t size) -{ - TLocalFilePtr localFile = PreparePlaceForCountryFiles(countryFile, version); - TEST(localFile.get(), ("Can't prepare place for", countryFile, "(version ", version, ")")); - { - string const zeroes(size, '\0'); - FileWriter writer(localFile->GetPath(MapOptions::Map)); - writer.Write(zeroes.data(), zeroes.size()); - } - localFile->SyncWithDisk(); - TEST_EQUAL(MapOptions::Map, localFile->GetFiles(), ()); - TEST_EQUAL(size, localFile->GetSize(MapOptions::Map), ()); - return localFile; -} - class CountryStatusChecker { public: @@ -259,6 +243,77 @@ private: int m_slot; }; +class FailedDownloadingWaiter +{ +public: + FailedDownloadingWaiter(Storage & storage, TIndex const & index) + : m_storage(storage), m_index(index), m_finished(false) + { + m_slot = m_storage.Subscribe(bind(&FailedDownloadingWaiter::OnStatusChanged, this, _1), + bind(&FailedDownloadingWaiter::OnProgress, this, _1, _2)); + } + + ~FailedDownloadingWaiter() + { + Wait(); + m_storage.Unsubscribe(m_slot); + } + + void Wait() + { + unique_lock lock(m_mu); + m_cv.wait(lock, [this]() + { + return m_finished; + }); + } + + void OnStatusChanged(TIndex const & index) + { + if (index != m_index) + return; + TStatus const status = m_storage.CountryStatusEx(index); + if (status != TStatus::EDownloadFailed) + return; + lock_guard lock(m_mu); + m_finished = true; + m_cv.notify_one(); + + QCoreApplication::exit(); + } + + void OnProgress(TIndex const & /* index */, LocalAndRemoteSizeT const & /* progress */) {} + +private: + Storage & m_storage; + TIndex const m_index; + int m_slot; + + mutex m_mu; + condition_variable m_cv; + bool m_finished; +}; + +void OnCountryDownloaded(LocalCountryFile const & localFile) +{ + LOG(LINFO, ("OnCountryDownloaded:", localFile)); +} + +TLocalFilePtr CreateDummyMapFile(CountryFile const & countryFile, int64_t version, size_t size) +{ + TLocalFilePtr localFile = PreparePlaceForCountryFiles(countryFile, version); + TEST(localFile.get(), ("Can't prepare place for", countryFile, "(version", version, ")")); + { + string const zeroes(size, '\0'); + FileWriter writer(localFile->GetPath(MapOptions::Map)); + writer.Write(zeroes.data(), zeroes.size()); + } + localFile->SyncWithDisk(); + TEST_EQUAL(MapOptions::Map, localFile->GetFiles(), ()); + TEST_EQUAL(size, localFile->GetSize(MapOptions::Map), ()); + return localFile; +} + void InitStorage(Storage & storage, TaskRunner & runner, Storage::TUpdate const & update = &OnCountryDownloaded) { @@ -619,4 +674,36 @@ UNIT_TEST(StorageTest_DeleteCountry) map.Reset(); routing.Reset(); } + +UNIT_TEST(StorageTest_FailedDownloading) +{ + Storage storage; + storage.Init(&OnCountryDownloaded); + storage.SetDownloaderForTesting(make_unique()); + storage.SetCurrentDataVersionForTesting(1234); + + TIndex const index = storage.FindIndexByFile("Uruguay"); + CountryFile const countryFile = storage.GetCountryFile(index); + + // To prevent interference from other tests and on other tests it's + // better to remove temprorary downloader files. + DeleteDownloaderFilesForCountry(countryFile, storage.GetCurrentDataVersion()); + MY_SCOPE_GUARD(cleanup, [&]() + { + DeleteDownloaderFilesForCountry(countryFile, storage.GetCurrentDataVersion()); + }); + + { + FailedDownloadingWaiter waiter(storage, index); + storage.DownloadCountry(index, MapOptions::Map); + QCoreApplication::exec(); + } + + // File wasn't downloaded, but temprorary downloader files must exist. + string const downloadPath = + GetFileDownloadPath(countryFile, MapOptions::Map, storage.GetCurrentDataVersion()); + TEST(!Platform::IsFileExistsByFullPath(downloadPath), ()); + TEST(Platform::IsFileExistsByFullPath(downloadPath + DOWNLOADING_FILE_EXTENSION), ()); + TEST(Platform::IsFileExistsByFullPath(downloadPath + RESUME_FILE_EXTENSION), ()); +} } // namespace storage diff --git a/storage/storage_tests/storage_tests.pro b/storage/storage_tests/storage_tests.pro index 50cb71e9b6..b2fe16d81b 100644 --- a/storage/storage_tests/storage_tests.pro +++ b/storage/storage_tests/storage_tests.pro @@ -10,14 +10,22 @@ DEPENDENCIES = storage indexer platform_tests_support platform geometry coding b include($$ROOT_DIR/common.pri) -macx-*: LIBS *= "-framework IOKit" -linux*|win32-msvc*: QT *= network +DEFINES *= OMIM_UNIT_TEST_WITH_QT_EVENT_LOOP QT *= core +macx-* { + QT *= gui widgets # needed for QApplication with event loop, to test async events (downloader, etc.) + LIBS *= "-framework IOKit" "-framework QuartzCore" +} +win32*|linux* { + QT *= network +} + HEADERS += \ fake_map_files_downloader.hpp \ task_runner.hpp \ + test_map_files_downloader.hpp \ SOURCES += \ ../../testing/testingmain.cpp \ @@ -27,3 +35,4 @@ SOURCES += \ simple_tree_test.cpp \ storage_tests.cpp \ task_runner.cpp \ + test_map_files_downloader.cpp \ diff --git a/tools/ResponseProvider.py b/tools/ResponseProvider.py index a7fb3b95c3..2a3b26e43e 100644 --- a/tools/ResponseProvider.py +++ b/tools/ResponseProvider.py @@ -127,6 +127,9 @@ class ResponseProvider: "/unit_tests/notexisting_unittest": self.test_404, "/unit_tests/permanent" : self.test_301, "/unit_tests/47kb.file" : self.test_47_kb, + # Following two URIs are used to test downloading failures on different platforms. + "/unit_tests/mac/1234/Uruguay.mwm" : self.test_404, + "/unit_tests/linux/1234/Uruguay.mwm" : self.test_404, "/ping" : self.pong, "/kill" : self.kill, "/id" :self.my_id, diff --git a/tools/run_desktop_tests.py b/tools/run_desktop_tests.py index b5ae60a90a..f81ffc1836 100755 --- a/tools/run_desktop_tests.py +++ b/tools/run_desktop_tests.py @@ -38,6 +38,7 @@ PASSED = "passed" PORT = 34568 +TESTS_REQUIRING_SERVER = ["platform_tests", "storage_tests"] class TestRunner: @@ -126,7 +127,7 @@ class TestRunner: self.log_exec_file(test_file) - if test_file == "platform_tests": + if test_file in TESTS_REQUIRING_SERVER: self.start_server() test_file_with_keys = "{test_file}{data}{resources}".format(test_file=test_file, data=self.data_path, resources=self.user_resource_path) @@ -139,7 +140,7 @@ class TestRunner: process.wait() - if test_file == "platform_tests": + if test_file in TESTS_REQUIRING_SERVER: self.stop_server() if process.returncode > 0: