Answer PR comments

Author: mked-luxoft

src/components/application_manager/include/application_manager/message_helper.h

@@ -93,11 +93,14 @@ class MessageHelper {
  * OnSystemRequestNotification
  * @param policy_data pt snapshot data
  * @param connection_key connection key of application
+ * @param request_type OnSystemRequest request type
  * @return OnSystemRequest notification smart object
  */
   static smart_objects::SmartObjectSPtr
   CreateOnSystemRequestNotificationToMobile(
-      const std::vector<uint8_t>& policy_data, const uint32_t connection_key);
+      const std::vector<uint8_t>& policy_data,
+      const uint32_t connection_key,
+      const mobile_apis::RequestType::eType request_type);
 
   /**
     * @brief ServiceStatusUpdateNotificationBuilder small utility class used for

src/components/application_manager/src/message_helper/message_helper.cc

@@ -2332,20 +2332,16 @@ bool MessageHelper::SendUnsubscribedWayPoints(ApplicationManager& app_mngr) {
 
 smart_objects::SmartObjectSPtr
 MessageHelper::CreateOnSystemRequestNotificationToMobile(
-    const std::vector<uint8_t>& policy_data, const uint32_t app_id) {
+    const std::vector<uint8_t>& policy_data,
+    const uint32_t app_id,
+    const mobile_apis::RequestType::eType request_type) {
   auto notification =
       CreateNotification(mobile_apis::FunctionID::OnSystemRequestID, app_id);
 
   (*notification)[strings::params][strings::binary_data] =
       smart_objects::SmartObject(policy_data);
 
-#if defined(PROPRIETARY_MODE) || defined(EXTERNAL_PROPRIETARY_MODE)
-  (*notification)[strings::msg_params][strings::request_type] =
-      mobile_apis::RequestType::PROPRIETARY;
-#else
-  (*notification)[strings::msg_params][strings::request_type] =
-      mobile_apis::RequestType::HTTP;
-#endif  // PROPRIETARY || EXTERNAL_PROPRIETARY_MODE
+  (*notification)[strings::msg_params][strings::request_type] = request_type;
 
   return notification;
 }
@@ -2355,8 +2351,14 @@ void MessageHelper::SendPolicySnapshotNotification(
     const std::vector<uint8_t>& policy_data,
     const std::string& url,
     ApplicationManager& app_mngr) {
-  auto notification =
-      CreateOnSystemRequestNotificationToMobile(policy_data, connection_key);
+  const auto request_type =
+#if defined(PROPRIETARY_MODE) || defined(EXTERNAL_PROPRIETARY_MODE)
+      mobile_apis::RequestType::PROPRIETARY;
+#else
+      mobile_apis::RequestType::HTTP;
+#endif  // PROPRIETARY || EXTERNAL_PROPRIETARY_MODE
+  auto notification = CreateOnSystemRequestNotificationToMobile(
+      policy_data, connection_key, request_type);
 
   if (!url.empty()) {
     (*notification)[strings::msg_params][strings::url] =
@@ -2367,13 +2369,6 @@ void MessageHelper::SendPolicySnapshotNotification(
 
   (*notification)[strings::params][strings::binary_data] =
       smart_objects::SmartObject(policy_data);
-#if defined(PROPRIETARY_MODE) || defined(EXTERNAL_PROPRIETARY_MODE)
-  (*notification)[strings::msg_params][strings::request_type] =
-      mobile_apis::RequestType::PROPRIETARY;
-#else
-  (*notification)[strings::msg_params][strings::request_type] =
-      mobile_apis::RequestType::HTTP;
-#endif  // PROPRIETARY || EXTERNAL_PROPRIETARY_MODE
 
   PrintSmartObject(*notification);
   app_mngr.GetRPCService().ManageMobileCommand(notification,

src/components/application_manager/src/policies/policy_handler.cc

@@ -1490,7 +1490,9 @@ void PolicyHandler::OnSnapshotCreated(const BinaryMessage& pt_string,
   if (PTUIterationType::RetryIteration == iteration_type) {
     auto on_system_request_notification =
         MessageHelper::CreateOnSystemRequestNotificationToMobile(
-            pt_string, GetAppIdForSending());
+            pt_string,
+            GetAppIdForSending(),
+            mobile_apis::RequestType::PROPRIETARY);
     application_manager_.GetRPCService().ManageMobileCommand(
         on_system_request_notification, commands::Command::SOURCE_SDL);
   } else {

src/components/application_manager/test/include/application_manager/mock_message_helper.h

@@ -139,10 +139,11 @@ class MockMessageHelper {
   MOCK_METHOD2(SendDecryptCertificateToHMI,
                void(const std::string& file_name,
                     ApplicationManager& app_mngr));
-  MOCK_METHOD2(
-      CreateOnSystemRequestNotificationToMobile,
-      smart_objects::SmartObjectSPtr(const std::vector<uint8_t>& policy_data,
-                                     const uint32_t connection_key));
+  MOCK_METHOD3(CreateOnSystemRequestNotificationToMobile,
+               smart_objects::SmartObjectSPtr(
+                   const std::vector<uint8_t>& policy_data,
+                   const uint32_t connection_key,
+                   const mobile_apis::RequestType::eType request_type));
 #ifdef EXTERNAL_PROPRIETARY_MODE
   MOCK_METHOD4(
       SendGetListOfPermissionsResponse,

src/components/application_manager/test/mock_message_helper.cc

@@ -614,9 +614,12 @@ MessageHelper::ServiceStatusUpdateNotificationBuilder::notification() const {
 
 smart_objects::SmartObjectSPtr
 MessageHelper::CreateOnSystemRequestNotificationToMobile(
-    const std::vector<uint8_t>& policy_data, const uint32_t connection_key) {
+    const std::vector<uint8_t>& policy_data,
+    const uint32_t connection_key,
+    const mobile_apis::RequestType::eType request_type) {
   return MockMessageHelper::message_helper_mock()
-      ->CreateOnSystemRequestNotificationToMobile(policy_data, connection_key);
+      ->CreateOnSystemRequestNotificationToMobile(
+          policy_data, connection_key, request_type);
 }
 
 smart_objects::SmartObject MessageHelper::CreateAppServiceCapabilities(

src/components/policy/policy_external/include/policy/policy_manager_impl.h

@@ -838,7 +838,10 @@ class PolicyManagerImpl : public PolicyManager {
   void RetrySequenceFailed() OVERRIDE;
 
   /**
-   * @brief Begins new retry sequence
+   * @brief In EXTERNAL_PROPRIETARY_MODE PTU sequence is driven by HMI and
+   * begins with OnSystemRequest from HMI. Following function is called when
+   * this notification is received to track number of PTU retries and react
+   * accordingly once allowed retry count is exceeded
    */
   void OnSystemRequestReceived() OVERRIDE;
 
@@ -1197,7 +1200,7 @@ class PolicyManagerImpl : public PolicyManager {
   /**
    * @brief Lock for guarding retry sequence
    */
-  sync_primitives::Lock retry_sequence_lock_;
+  mutable sync_primitives::Lock retry_sequence_lock_;
 
   /**
    * @brief Device id, which is used during PTU handling for specific

src/components/policy/policy_external/src/policy_manager_impl.cc

@@ -1268,11 +1268,21 @@ void PolicyManagerImpl::SetUserConsentForApp(
 
 bool PolicyManagerImpl::IsAllowedRetryCountExceeded() const {
   LOG4CXX_AUTO_TRACE(logger_);
+  sync_primitives::AutoLock auto_lock(retry_sequence_lock_);
+
   return retry_sequence_index_ > retry_sequence_seconds_.size();
 }
 
 void PolicyManagerImpl::IncrementRetryIndex() {
   LOG4CXX_AUTO_TRACE(logger_);
+  sync_primitives::AutoLock auto_lock(retry_sequence_lock_);
+
+  if (!is_ptu_in_progress_) {
+    LOG4CXX_TRACE(logger_,
+                  "First PTU iteration, skipping incrementing retry index");
+    is_ptu_in_progress_ = true;
+    return;
+  }
 
   ++retry_sequence_index_;
   LOG4CXX_DEBUG(logger_,
@@ -1289,11 +1299,7 @@ void PolicyManagerImpl::RetrySequenceFailed() {
 void PolicyManagerImpl::OnSystemRequestReceived() {
   LOG4CXX_AUTO_TRACE(logger_);
 
-  if (is_ptu_in_progress_) {
-    IncrementRetryIndex();
-  } else {
-    is_ptu_in_progress_ = true;
-  }
+  IncrementRetryIndex();
 }
 
 bool PolicyManagerImpl::GetDefaultHmi(const std::string& policy_app_id,

src/components/policy/policy_regular/include/policy/policy_manager_impl.h

@@ -922,7 +922,7 @@ class PolicyManagerImpl : public PolicyManager {
   /**
    * @brief Starts new retry sequence
    */
-  void StartRetrySequence();
+  void OnPTUIterationTimeout();
 
  private:
   /**

src/components/policy/policy_regular/src/policy_manager_impl.cc

@@ -77,9 +77,10 @@ PolicyManagerImpl::PolicyManagerImpl()
           new AccessRemoteImpl(std::static_pointer_cast<CacheManager>(cache_)))
     , retry_sequence_timeout_(kDefaultRetryTimeoutInMSec)
     , retry_sequence_index_(0)
-    , timer_retry_sequence_("Retry sequence timer",
-                            new timer::TimerTaskImpl<PolicyManagerImpl>(
-                                this, &PolicyManagerImpl::StartRetrySequence))
+    , timer_retry_sequence_(
+          "Retry sequence timer",
+          new timer::TimerTaskImpl<PolicyManagerImpl>(
+              this, &PolicyManagerImpl::OnPTUIterationTimeout))
     , ignition_check(true)
     , retry_sequence_url_(0, 0, "")
     , wrong_ptu_update_received_(false)
@@ -1410,7 +1411,7 @@ void PolicyManagerImpl::set_cache_manager(
   cache_ = std::shared_ptr<CacheManagerInterface>(cache_manager);
 }
 
-void PolicyManagerImpl::StartRetrySequence() {
+void PolicyManagerImpl::OnPTUIterationTimeout() {
   LOG4CXX_DEBUG(logger_, "Start new retry sequence");
 
   const bool is_exceeded_retries_count =