[storage] Fixing the (logical) race on the diffs cancellation result.

3party/bsdiff-courgette/bsdiff/bsdiff.h

@@ -501,6 +501,9 @@ BSDiffStatus ApplyBinaryPatch(OldReader & old_reader, NewSink & new_sink,
     return UNEXPECTED_ERROR;
   }
 
+  if (cancellable.IsCancelled())
+    return CANCELLED;
+
   return OK;
 }
 }  // namespace bsdiff

storage/diff_scheme/diff_manager.cpp

@@ -102,10 +102,6 @@ void Manager::ApplyDiff(ApplyDiffParams && p, base::Cancellable const & cancella
     case DiffApplicationResult::Ok:
     {
       diffFile->DeleteFromDisk(MapOptions::Diff);
-      std::lock_guard<std::mutex> lock(m_mutex);
-      auto it = m_diffs.find(diffFile->GetCountryName());
-      CHECK(it != m_diffs.end(), ());
-      it->second.m_status = SingleDiffStatus::Applied;
       break;
     }
     case DiffApplicationResult::Cancelled:
@@ -163,25 +159,13 @@ bool Manager::HasDiffFor(storage::CountryId const & countryId) const
   return WithDiff(countryId, [](DiffInfo const &) {});
 }
 
-void Manager::RemoveAppliedDiffs()
-{
-  std::lock_guard<std::mutex> lock(m_mutex);
-  for (auto it = m_diffs.begin(); it != m_diffs.end();)
-  {
-    if (it->second.m_status == SingleDiffStatus::Applied)
-      it = m_diffs.erase(it);
-    else
-      ++it;
-  }
-
-  if (m_diffs.empty())
-    m_status = Status::NotAvailable;
-}
-
 void Manager::RemoveDiffForCountry(storage::CountryId const & countryId)
 {
   std::lock_guard<std::mutex> lock(m_mutex);
   m_diffs.erase(countryId);
+
+  if (m_diffs.empty())
+    m_status = Status::NotAvailable;
 }
 
 void Manager::AbortDiffScheme()

storage/diff_scheme/diff_manager.hpp

@@ -60,7 +60,6 @@ public:
   // has been downloaded.
   bool HasDiffFor(storage::CountryId const & countryId) const;
 
-  void RemoveAppliedDiffs();
   void RemoveDiffForCountry(storage::CountryId const & countryId);
   void AbortDiffScheme();
 

storage/storage.cpp

@@ -630,7 +630,6 @@ void Storage::DownloadNextCountryFromQueue()
 
   if (m_queue.empty())
   {
-    m_diffManager.RemoveAppliedDiffs();
     m_downloadingPolicy->ScheduleRetry(m_failedCountries, [this](CountriesSet const & needReload) {
       for (auto const & country : needReload)
       {
@@ -1299,7 +1298,10 @@ bool Storage::DeleteCountryFilesFromDownloader(CountryId const & countryId)
     return false;
 
   if (m_latestDiffRequest && m_latestDiffRequest == countryId)
+  {
     m_diffsCancellable.Cancel();
+    m_diffsBeingApplied.erase(countryId);
+  }
 
   MapOptions const opt = queuedCountry->GetInitOptions();
   if (IsCountryFirstInQueue(countryId))
@@ -1571,6 +1573,7 @@ void Storage::ApplyDiff(CountryId const & countryId, function<void(bool isSucces
 {
   m_diffsCancellable.Reset();
   m_latestDiffRequest = countryId;
+  m_diffsBeingApplied.insert(countryId);
   NotifyStatusChangedForHierarchy(countryId);
 
   diffs::Manager::ApplyDiffParams params;
@@ -1584,6 +1587,7 @@ void Storage::ApplyDiff(CountryId const & countryId, function<void(bool isSucces
     ASSERT(false, ("Invalid attempt to get version of diff with country id:", countryId));
     fn(false);
     m_latestDiffRequest = {};
+    m_diffsBeingApplied.erase(countryId);
     return;
   }
 
@@ -1603,13 +1607,20 @@ void Storage::ApplyDiff(CountryId const & countryId, function<void(bool isSucces
         }
 
         GetPlatform().RunTask(Platform::Thread::Gui, [this, fn, diffFile, countryId, result] {
+          auto realResult = result;
+          if (m_diffsBeingApplied.count(countryId) == 0 && realResult == DiffApplicationResult::Ok)
+          {
+            realResult = DiffApplicationResult::Cancelled;
+          }
           m_latestDiffRequest = {};
-          switch (result)
+          m_diffsBeingApplied.erase(countryId);
+          switch (realResult)
           {
           case DiffApplicationResult::Ok:
           {
             RegisterCountryFiles(diffFile);
             Platform::DisableBackupForFile(diffFile->GetPath(MapOptions::Map));
+            m_diffManager.RemoveDiffForCountry(countryId);
             fn(true);
             break;
           }
@@ -1621,6 +1632,7 @@ void Storage::ApplyDiff(CountryId const & countryId, function<void(bool isSucces
           }
           case DiffApplicationResult::Failed:
           {
+            m_diffManager.RemoveDiffForCountry(countryId);
             fn(false);
             break;
           }

storage/storage.hpp

@@ -203,6 +203,13 @@ private:
 
   boost::optional<CountryId> m_latestDiffRequest;
 
+  // Since the diff manager runs on a different thread, the result
+  // of diff application may return "Ok" when in fact the diff was
+  // cancelled. However, the storage thread knows for sure whether the
+  // latest request was to apply or to cancel the diff, and this knowledge
+  // is represented by |m_diffsBeingApplied|.
+  std::set<CountryId> m_diffsBeingApplied;
+
   DownloadingPolicy m_defaultDownloadingPolicy;
   DownloadingPolicy * m_downloadingPolicy = &m_defaultDownloadingPolicy;