Merge pull request #1218 from gorshenin/cancel-downloading-bugfix

[storage, bugfix in release] Fixed cancel of downloading.

platform/country_defines.cpp

@@ -8,6 +8,11 @@ bool HasOptions(TMapOptions mask, TMapOptions options)
          static_cast<uint8_t>(options);
 }
 
+TMapOptions IntersectOptions(TMapOptions lhs, TMapOptions rhs)
+{
+  return static_cast<TMapOptions>(static_cast<uint8_t>(lhs) & static_cast<uint8_t>(rhs));
+}
+
 TMapOptions SetOptions(TMapOptions mask, TMapOptions options)
 {
   return static_cast<TMapOptions>(static_cast<uint8_t>(mask) | static_cast<uint8_t>(options));

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);

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<LocalCountryFile> const & rhs)
+  auto const equalsToLocalFile = [&localFile](shared_ptr<LocalCountryFile> 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<LocalCountryFile> 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<LocalCountryFile> 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<LocalCountryFile> 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<LocalCountryFile> localFile = GetLocalFile(index, GetCurrentDataVersion()))
+  {
+    localFile->DeleteFromDisk(opt);
+    localFile->SyncWithDisk();
+    if (localFile->GetFiles() == TMapOptions::ENothing)
+    {
+      auto equalsToLocalFile = [&localFile](shared_ptr<LocalCountryFile> 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

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<platform::LocalCountryFile> 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;

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<string> 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

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<string> m_servers;
   TProgress m_progress;
   bool m_idle;
 
+  unique_ptr<FileWriter> m_writer;
+  TFileDownloadedCallback m_onDownloaded;
+  TDownloadingProgressCallback m_onProgress;
+
+  uint64_t m_timestamp;
+
   TaskRunner & m_taskRunner;
   ThreadChecker m_checker;
 };

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<TStatus> m_transitionList;
 };
 
+class CancelDownloadingWhenAlmostDoneChecker : public CountryDownloaderChecker
+{
+public:
+  CancelDownloadingWhenAlmostDoneChecker(Storage & storage, TIndex const & index,
+                                         TaskRunner & runner)
+      : CountryDownloaderChecker(storage, index, TMapOptions::EMapWithCarRouting,
+                                 vector<TStatus>{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<CountryDownloaderChecker> AbsentCountryDownloaderChecker(Storage & storage,
@@ -484,7 +517,7 @@ UNIT_TEST(StorageTest_DownloadTwoCountriesAndDelete)
     unique_ptr<CountryDownloaderChecker> venezuelaChecker = make_unique<CountryDownloaderChecker>(
         storage, venezuelaIndex, TMapOptions::EMapWithCarRouting,
         vector<TStatus>{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<LocalCountryFile> file = storage.GetLatestLocalFile(index);
+  TEST(!file, (*file));
+}