[editor] Fixes for the OSM uploading code

Signed-off-by: map-per <map-per@gmx.de>

libs/editor/editor_notes.cpp

@@ -142,7 +142,7 @@ void Notes::CreateNote(ms::LatLon const & latLon, std::string const & text)
     return;
   }
 
-  std::lock_guard<std::mutex> g(m_mu);
+  std::lock_guard<std::mutex> g(m_dataAccessMutex);
   auto const it = std::find_if(m_notes.begin(), m_notes.end(), [&latLon, &text](Note const & note)
   { return latLon.EqualDxDy(note.m_point, kTolerance) && text == note.m_note; });
   // No need to add the same note. It works in case when saved note are not uploaded yet.
@@ -155,61 +155,56 @@ void Notes::CreateNote(ms::LatLon const & latLon, std::string const & text)
 
 void Notes::Upload(osm::OsmOAuth const & auth)
 {
-  // Capture self to keep it from destruction until this thread is done.
-  auto const self = shared_from_this();
+  std::unique_lock<std::mutex> uploadingNotesLock(m_uploadingNotesMutex, std::defer_lock);
+  if (!uploadingNotesLock.try_lock()) {
+    // Do not run more than one uploading task at a time.
+    LOG(LDEBUG, ("OSM notes upload is already running"));
+    return;
+  }
+  std::unique_lock<std::mutex> dataAccessLock(m_dataAccessMutex);
 
-  auto const doUpload = [self, auth]()
-  {
-    std::unique_lock<std::mutex> ulock(self->m_mu);
   // Size of m_notes is decreased only in this method.
-    auto & notes = self->m_notes;
-    size_t size = notes.size();
+  size_t size = m_notes.size();
   osm::ServerApi06 api(auth);
 
   while (size > 0)
   {
     try
     {
-        ulock.unlock();
-        auto const id = api.CreateNote(notes.front().m_point, notes.front().m_note);
-        ulock.lock();
+      dataAccessLock.unlock();
+      auto const id = api.CreateNote(m_notes.front().m_point, m_notes.front().m_note);
+      dataAccessLock.lock();
       LOG(LINFO, ("A note uploaded with id", id));
     }
     catch (osm::ServerApi06::ServerApi06Exception const & e)
     {
       LOG(LERROR, ("Can't upload note.", e.Msg()));
-        // We believe that next iterations will suffer from the same error.
+      // Don't attempt upload for other notes as they will likely suffer from the same error.
       return;
     }
 
-      notes.pop_front();
+    m_notes.pop_front();
     --size;
-      ++self->m_uploadedNotesCount;
-      Save(self->m_fileName, self->m_notes, self->m_uploadedNotesCount);
+    ++m_uploadedNotesCount;
+    Save(m_fileName, m_notes, m_uploadedNotesCount);
   }
-  };
-
-  static auto future = std::async(std::launch::async, doUpload);
-  auto const status = future.wait_for(std::chrono::milliseconds(0));
-  if (status == std::future_status::ready)
-    future = std::async(std::launch::async, doUpload);
 }
 
 std::list<Note> Notes::GetNotes() const
 {
-  std::lock_guard<std::mutex> g(m_mu);
+  std::lock_guard<std::mutex> g(m_dataAccessMutex);
   return m_notes;
 }
 
 size_t Notes::NotUploadedNotesCount() const
 {
-  std::lock_guard<std::mutex> g(m_mu);
+  std::lock_guard<std::mutex> g(m_dataAccessMutex);
   return m_notes.size();
 }
 
 size_t Notes::UploadedNotesCount() const
 {
-  std::lock_guard<std::mutex> g(m_mu);
+  std::lock_guard<std::mutex> g(m_dataAccessMutex);
   return m_uploadedNotesCount;
 }
 }  // namespace editor

libs/editor/editor_notes.hpp

@@ -46,7 +46,8 @@ private:
   explicit Notes(std::string const & fileName);
 
   std::string const m_fileName;
-  mutable std::mutex m_mu;
+  mutable std::mutex m_dataAccessMutex;
+  mutable std::mutex m_uploadingNotesMutex;
 
   // m_notes keeps the notes that have not been uploaded yet.
   // Once a note has been uploaded, it is removed from m_notes.

libs/editor/osm_editor.cpp

@@ -126,7 +126,7 @@ bool IsObsolete(editor::XMLFeature const & xml, FeatureID const & fid)
 }
 }  // namespace
 
-Editor::Editor() : m_configLoader(m_config), m_notes(editor::Notes::MakeNotes()), m_isUploadingNow(false)
+Editor::Editor() : m_configLoader(m_config), m_notes(editor::Notes::MakeNotes())
 {
   SetDefaultStorage();
 }
@@ -576,18 +576,21 @@ void Editor::UploadChanges(string const & oauthToken, ChangesetTags tags, Finish
   if (m_notes->NotUploadedNotesCount())
     m_notes->Upload(OsmOAuth::ServerAuth(oauthToken));
 
-  auto const features = m_features.Get();
-
-  if (!HaveMapEditsToUpload(*features))
+  if (!HaveMapEditsToUpload(*m_features.Get()))
   {
     LOG(LDEBUG, ("There are no local edits to upload."));
     return;
   }
 
-  auto upload = [this](string secret, ChangesetTags tags, FinishUploadCallback callback)
-  {
+  std::unique_lock<std::mutex> uploadingEditsLock(m_uploadingEditsMutex, std::defer_lock);
+  if (!uploadingEditsLock.try_lock()) {
+    // Do not run more than one uploading task at a time.
+    LOG(LDEBUG, ("OSM edits upload is already running"));
+    return;
+  }
+
   int uploadedFeaturesCount = 0, errorsCount = 0;
-    ChangesetWrapper changeset(secret, std::move(tags));
+  ChangesetWrapper changeset(oauthToken, std::move(tags));
   auto const features = m_features.Get();
 
   for (auto const & id : *features)
@@ -867,18 +870,6 @@ void Editor::UploadChanges(string const & oauthToken, ChangesetTags tags, Finish
       result = UploadResult::Error;
     callback(result);
   }
-
-    m_isUploadingNow = false;
-  };
-
-  // Do not run more than one uploading task at a time.
-  if (!m_isUploadingNow)
-  {
-    m_isUploadingNow = true;
-    GetPlatform().RunTask(Platform::Thread::Network, [upload = std::move(upload), oauthToken, tags = std::move(tags),
-                                                      callback = std::move(callback)]()
-    { upload(std::move(oauthToken), std::move(tags), std::move(callback)); });
-  }
 }
 
 void Editor::SaveUploadedInformation(FeatureID const & fid, UploadInfo const & uploadInfo)

libs/editor/osm_editor.hpp

@@ -260,7 +260,7 @@ private:
 
   std::unique_ptr<editor::StorageBase> m_storage;
 
-  std::atomic<bool> m_isUploadingNow;
+  std::mutex m_uploadingEditsMutex;
 
   DECLARE_THREAD_CHECKER(MainThreadChecker);
 };  // class Editor