Correct using m_routingSessionMutex for RoutingSession members and review fixes.

2 files changed, 111 insertions, 36 deletions

diff --git a/routing/routing_session.cpp b/routing/routing_session.cpp
index f5806c57e2..9d6b143bd2 100644
--- a/routing/routing_session.cpp
+++ b/routing/routing_session.cpp
@@ -73,7 +73,7 @@ RoutingSession::RoutingSession()
 {
 }
 
-void RoutingSession::Init(TRoutingStatisticsCallback const & routingStatisticsFn,
+void RoutingSession::Init(RoutingStatisticsCallback const & routingStatisticsFn,
                           RouterDelegate::TPointCheckCallback const & pointCheckCallback)
 {
   threads::MutexGuard guard(m_routingSessionMutex);
@@ -98,7 +98,7 @@ void RoutingSession::BuildRoute(Checkpoints const & checkpoints,
 }
 
 void RoutingSession::RebuildRoute(m2::PointD const & startPoint,
-                                  TReadyCallback const & readyCallback, uint32_t timeoutSec,
+                                  ReadyCallback const & readyCallback, uint32_t timeoutSec,
                                   State routeRebuildingState, bool adjustToPrevRoute)
 {
   {
@@ -196,6 +196,72 @@ void RoutingSession::RebuildRouteOnTrafficUpdate()
                routing::RoutingSession::State::RouteRebuilding, false /* adjustToPrevRoute */);
 }
 
+bool RoutingSession::IsActive() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return (m_state != RoutingNotActive);
+}
+
+bool RoutingSession::IsNavigable() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return IsNavigableImpl();
+}
+
+bool RoutingSession::IsBuilt() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return (IsNavigableImpl() || m_state == RouteNeedRebuild);
+}
+
+bool RoutingSession::IsBuilding() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return (m_state == RouteBuilding || m_state == RouteRebuilding);
+}
+
+bool RoutingSession::IsBuildingOnly() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return m_state == RouteBuilding;
+}
+
+bool RoutingSession::IsRebuildingOnly() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return m_state == RouteRebuilding;
+}
+
+bool RoutingSession::IsNotReady() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return m_state == RouteNotReady;
+}
+
+bool RoutingSession::IsFinished() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return m_state == RouteFinished;
+}
+
+bool RoutingSession::IsNoFollowing() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return m_state == RouteNoFollowing;
+}
+
+bool RoutingSession::IsOnRoute() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return (m_state == OnRoute);
+}
+
+bool RoutingSession::IsFollowing() const
+{
+  threads::MutexGuard guard(m_routingSessionMutex);
+  return m_isFollowing;
+}
+
 void RoutingSession::Reset()
 {
   threads::MutexGuard guard(m_routingSessionMutex);
@@ -220,6 +286,7 @@ void RoutingSession::ResetImpl()
 
 RoutingSession::State RoutingSession::OnLocationPositionChanged(GpsInfo const & info, Index const & index)
  {
+  threads::MutexGuard guard(m_routingSessionMutex);
   ASSERT(m_state != RoutingNotActive, ());
   ASSERT(m_router != nullptr, ());
 
@@ -228,7 +295,6 @@ RoutingSession::State RoutingSession::OnLocationPositionChanged(GpsInfo const &
       || m_state == RouteNotReady || m_state == RouteNoFollowing)
     return m_state;
 
-  threads::MutexGuard guard(m_routingSessionMutex);
   ASSERT(m_route, ());
   ASSERT(m_route->IsValid(), ());
 
@@ -326,7 +392,7 @@ void RoutingSession::GetRouteFollowingInfo(FollowingInfo & info) const
     return;
   }
 
-  if (!IsNavigable())
+  if (!IsNavigableImpl())
   {
     info = FollowingInfo();
     FormatDistance(m_route->GetTotalDistanceMeters(), info.m_distToTarget, info.m_targetUnitsSuffix);
@@ -431,7 +497,7 @@ void RoutingSession::GenerateTurnNotifications(vector<string> & turnNotification
   if (!m_routingSettings.m_soundDirection)
     return;
 
-  if (!m_route->IsValid() || !IsNavigable())
+  if (!m_route->IsValid() || !IsNavigableImpl())
     return;
 
   vector<turns::TurnItemDist> turns;
@@ -502,6 +568,7 @@ traffic::SpeedGroup RoutingSession::MatchTraffic(
 bool RoutingSession::DisableFollowMode()
 {
   LOG(LINFO, ("Routing disables a following mode. State: ", m_state.load()));
+  threads::MutexGuard guard(m_routingSessionMutex);
   if (m_state == RouteNotStarted || m_state == OnRoute)
   {
     SetState(RouteNoFollowing);
@@ -514,6 +581,7 @@ bool RoutingSession::DisableFollowMode()
 bool RoutingSession::EnableFollowMode()
 {
   LOG(LINFO, ("Routing enables a following mode. State: ", m_state.load()));
+  threads::MutexGuard guard(m_routingSessionMutex);
   if (m_state == RouteNotStarted || m_state == OnRoute)
   {
     SetState(OnRoute);
@@ -528,8 +596,8 @@ void RoutingSession::SetRoutingSettings(RoutingSettings const & routingSettings)
   m_routingSettings = routingSettings;
 }
 
-void RoutingSession::SetReadyCallbacks(TReadyCallback const & buildReadyCallback,
-                                       TReadyCallback const & rebuildReadyCallback)
+void RoutingSession::SetReadyCallbacks(ReadyCallback const & buildReadyCallback,
+                                       ReadyCallback const & rebuildReadyCallback)
 {
   m_buildReadyCallback = buildReadyCallback;
   // m_rebuildReadyCallback used from multiple threads but it's the only place we write m_rebuildReadyCallback
@@ -542,7 +610,7 @@ void RoutingSession::SetReadyCallbacks(TReadyCallback const & buildReadyCallback
   m_rebuildReadyCallback = rebuildReadyCallback;
 }
 
-void RoutingSession::SetProgressCallback(TProgressCallback const & progressCallback)
+void RoutingSession::SetProgressCallback(ProgressCallback const & progressCallback)
 {
   m_progressCallback = progressCallback;
 }
@@ -554,6 +622,7 @@ void RoutingSession::SetCheckpointCallback(CheckpointCallback const & checkpoint
 
 void RoutingSession::SetUserCurrentPosition(m2::PointD const & position)
 {
+  threads::MutexGuard guard(m_routingSessionMutex);
   // All methods which read/write m_userCurrentPosition*, m_userFormerPosition*  work in RoutingManager thread
   m_userFormerPosition = m_userCurrentPosition;
   m_userFormerPositionValid = m_userCurrentPositionValid;
diff --git a/routing/routing_session.hpp b/routing/routing_session.hpp
index 2c138633b4..cfa179e91a 100644
--- a/routing/routing_session.hpp
+++ b/routing/routing_session.hpp
@@ -21,6 +21,7 @@
 #include "base/mutex.hpp"
 
 #include "std/atomic.hpp"
+#include "std/functional.hpp"
 #include "std/limits.hpp"
 #include "std/map.hpp"
 #include "std/shared_ptr.hpp"
@@ -75,16 +76,15 @@ public:
    * RouteFinished -> RouteNotReady       // start new route
    */
 
-  typedef function<void(map<string, string> const &)> TRoutingStatisticsCallback;
-
-  typedef function<void(Route const &, IRouter::ResultCode)> TReadyCallback;
-  typedef function<void(float)> TProgressCallback;
-  typedef function<void(size_t passedCheckpointIdx)> CheckpointCallback;
+  using RoutingStatisticsCallback = function<void(map<string, string> const &)>;
+  using ReadyCallback = function<void(Route const &, IRouter::ResultCode)>;
+  using ProgressCallback = function<void(float)>;
+  using CheckpointCallback = function<void(size_t passedCheckpointIdx)>;
   using RouteCallback = function<void(Route const &)>;
 
   RoutingSession();
 
-  void Init(TRoutingStatisticsCallback const & routingStatisticsFn,
+  void Init(RoutingStatisticsCallback const & routingStatisticsFn,
             RouterDelegate::TPointCheckCallback const & pointCheckCallback);
 
   void SetRouter(unique_ptr<IRouter> && router, unique_ptr<OnlineAbsentCountriesFetcher> && fetcher);
@@ -93,24 +93,25 @@ public:
   /// @param[in] timeoutSec timeout in seconds, if zero then there is no timeout
   void BuildRoute(Checkpoints const & checkpoints,
                   uint32_t timeoutSec);
-  void RebuildRoute(m2::PointD const & startPoint, TReadyCallback const & readyCallback,
+  void RebuildRoute(m2::PointD const & startPoint, ReadyCallback const & readyCallback,
                     uint32_t timeoutSec, State routeRebuildingState, bool adjustToPrevRoute);
 
   m2::PointD GetStartPoint() const;
   m2::PointD GetEndPoint() const;
-  bool IsActive() const { return (m_state != RoutingNotActive); }
-  bool IsNavigable() const { return (m_state == RouteNotStarted || m_state == OnRoute || m_state == RouteFinished); }
-  bool IsBuilt() const { return (IsNavigable() || m_state == RouteNeedRebuild); }
+
+  bool IsActive() const;
+  bool IsNavigable() const;
+  bool IsBuilt() const;
   /// \returns true if a new route is in process of building rebuilding or
   /// if a route is being rebuilt in case the user left the route, and false otherwise.
-  bool IsBuilding() const { return (m_state == RouteBuilding || m_state == RouteRebuilding); }
-  bool IsBuildingOnly() const { return m_state == RouteBuilding; }
-  bool IsRebuildingOnly() const { return m_state == RouteRebuilding; }
-  bool IsNotReady() const { return m_state == RouteNotReady; }
-  bool IsFinished() const { return m_state == RouteFinished; }
-  bool IsNoFollowing() const { return m_state == RouteNoFollowing; }
-  bool IsOnRoute() const { return (m_state == OnRoute); }
-  bool IsFollowing() const { return m_isFollowing; }
+  bool IsBuilding() const;
+  bool IsBuildingOnly() const;
+  bool IsRebuildingOnly() const;
+  bool IsNotReady() const;
+  bool IsFinished() const;
+  bool IsNoFollowing() const;
+  bool IsOnRoute() const;
+  bool IsFollowing() const;
   void Reset();
 
   inline void SetState(State state) { m_state = state; }
@@ -148,9 +149,9 @@ public:
   bool EnableFollowMode();
 
   void SetRoutingSettings(RoutingSettings const & routingSettings);
-  void SetReadyCallbacks(TReadyCallback const & buildReadyCallback,
-                         TReadyCallback const & rebuildReadyCallback);
-  void SetProgressCallback(TProgressCallback const & progressCallback);
+  void SetReadyCallbacks(ReadyCallback const & buildReadyCallback,
+                         ReadyCallback const & rebuildReadyCallback);
+  void SetProgressCallback(ProgressCallback const & progressCallback);
   void SetCheckpointCallback(CheckpointCallback const & checkpointCallback);
 
   // Sound notifications for turn instructions.
@@ -178,10 +179,10 @@ private:
   struct DoReadyCallback
   {
     RoutingSession & m_rs;
-    TReadyCallback m_callback;
+    ReadyCallback m_callback;
     threads::Mutex & m_routeSessionMutexInner;
 
-    DoReadyCallback(RoutingSession & rs, TReadyCallback const & cb,
+    DoReadyCallback(RoutingSession & rs, ReadyCallback const & cb,
                     threads::Mutex & routeSessionMutex)
         : m_rs(rs), m_callback(cb), m_routeSessionMutexInner(routeSessionMutex)
     {
@@ -190,21 +191,26 @@ private:
     void operator()(Route & route, IRouter::ResultCode e);
   };
 
+  // Should be called with locked m_routingSessionMutex.
   void AssignRoute(Route & route, IRouter::ResultCode e);
 
   /// Returns a nearest speed camera record on your way and distance to it.
   /// Returns kInvalidSpeedCameraDistance if there is no cameras on your way.
+  // Should be called with locked m_routingSessionMutex.
   double GetDistanceToCurrentCamM(SpeedCameraRestriction & camera, Index const & index);
 
   /// RemoveRoute removes m_route and resets route attributes (m_state, m_lastDistance, m_moveAwayCounter).
   void RemoveRoute();
   void RebuildRouteOnTrafficUpdate();
 
-  // Must be called with locked m_routingSessionMutex
+  // Should be called with locked m_routingSessionMutex.
   void ResetImpl();
-
+  // Should be called with locked m_routingSessionMutex.
   double GetCompletionPercent() const;
+  // Should be called with locked m_routingSessionMutex.
   void PassCheckpoints();
+  // Should be called with locked m_routingSessionMutex.
+  bool IsNavigableImpl() const { return (m_state == RouteNotStarted || m_state == OnRoute || m_state == RouteFinished); }
 
 private:
   unique_ptr<AsyncRouter> m_router;
@@ -241,9 +247,9 @@ private:
 
   RoutingSettings m_routingSettings;
 
-  TReadyCallback m_buildReadyCallback;
-  TReadyCallback m_rebuildReadyCallback;
-  TProgressCallback m_progressCallback;
+  ReadyCallback m_buildReadyCallback;
+  ReadyCallback m_rebuildReadyCallback;
+  ProgressCallback m_progressCallback;
   CheckpointCallback m_checkpointCallback;
 
   // Statistics parameters