Fix vehicle data params after master reset #2614

SNiukalov · 2018-09-17T07:21:04Z

This PR is ready for review.

Risk

This PR makes no API changes.

Summary

There was a problem that in some cases parallel call of functions of policy_manager_ object could cause database inner structure corruption which causes undefined issues with policy permissions. In order to avoid such behavior, all calls of policy_manager_ was wrapped with universal function which provides thread safe access for this object.

CLA

I have signed the CLA

AByzhynar · 2018-09-19T11:50:52Z

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

  const std::vector<UserFriendlyMessage> result =
      policy_manager_->GetUserFriendlyMessages(message_codes, language);
+  policy_manager_lock_.Release();


@SNiukalov Why do you make Lock and Release twice?

@AByzhynar there is ifdef -> else -> endif sections.
But as for me you can use next way:

std::vector<UserFriendlyMessage> result; #ifdef EXTERNAL_PROPRIETARY_MODE const std::string active_hmi_language = application_manager::MessageHelper::CommonLanguageToString( application_manager_.hmi_capabilities().active_ui_language()); { sync_primitives::AutoLock lock(policy_manager_lock_); result = policy_manager_->GetUserFriendlyMessages(message_codes, language, active_hmi_language); } #else { sync_primitives::AutoLock lock(policy_manager_lock_); result = policy_manager_->GetUserFriendlyMessages(message_codes, language); } #endif // EXTERNAL_PROPRIETARY_MODE

This code block is in the definition for EXTERNAL_PROPRIETARY_MODE.
In the code for EXTERNAL_PROPRIETARY_MODE,
before calling the PolicyManager method,
The methods of the ApplicationManager and HMICapabilities classes are called.
We don't use this object synchronize for them.

@AByzhynar
@EKuliiev
Fixed in: 793f178

AKalinich-Luxoft · 2018-09-21T17:06:39Z

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

-    policy_manager_->SetUserConsentForDevice(device_mac, is_allowed);
+    {
+      sync_primitives::AutoLock lock(policy_manager_lock_);
+      policy_manager_->SetUserConsentForDevice(device_mac, is_allowed);


@SNiukalov I see a lot of calls like policy_manager-> something + acquire/release lock
As we have C++11 support in open source, I propose you to declare a template like:

template <typename Func, typename... Args> void CallPolicyManagerFunction(Func func, Args... args) { sync_primitives::AutoLock lock(policy_manager_lock_); ((*policy_manager_).*func)(args...); }

And simplify all these calls to a single line:

CallPolicyManagerFunction(&PolicyManager::SetUserConsentForDevice, device_mac, is_allowed);

Please analyze all such places and try this template instead

@AKalinich-Luxoft
Fixed in: 793f178

AKalinich-Luxoft · 2018-09-21T17:07:47Z

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

  return policy_manager_->GetLockScreenIconUrl();
 }

 uint32_t PolicyHandler::NextRetryTimeout() {
  POLICY_LIB_CHECK(0);
-  LOG4CXX_AUTO_TRACE(logger_);
+  sync_primitives::AutoLock lock(policy_manager_lock_);
  return policy_manager_->NextRetryTimeout();


@SNiukalov try to create similar template with the function return type

@SNiukalov that's crazy, but something like that:

template <typename Func, typename... Args> auto CallPolicyManagerFunction(Func func, Args... args) -> decltype((std::declval<PolicyManager>().*std::declval<Func>())(std::declval<Args>()...)) { sync_primitives::AutoLock lock(policy_manager_lock_); return ((*policy_manager_).*func)(args...); }

It should work for both cases - when return type required and when not

@SNiukalov or another way - try to create accessor to PolicyManager instance and make access to it only through accessor

@AKalinich-Luxoft
Fixed in: 793f178

AKalinich-Luxoft

Actually, you haven't analyzed 50+ places where this template also applicable. Try to minimize usage of policy_manager_lock_ as much as possible. I have to mark all the places you missed

AKalinich-Luxoft · 2018-09-26T22:17:18Z

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

@@ -2163,4 +2249,13 @@ void PolicyHandler::OnUpdateHMILevel(const std::string& device_id,
  }
  UpdateHMILevel(app, level);
 }
+
+template <typename Func, typename... Args>


@SNiukalov I think you could merge this template definition with declaration and keep whole in header file

AKalinich-Luxoft · 2018-09-26T22:20:00Z

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

  POLICY_LIB_CHECK(false);
  // Subscribing to notification for system readiness to be able to get system
  // info necessary for policy table
  event_observer_->subscribe_on_event(
      hmi_apis::FunctionID::BasicCommunication_OnReady);
  std::string preloaded_file = get_settings().preloaded_pt_file();
  if (file_system::FileExists(preloaded_file)) {
+    sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov CallPolicyManagerFunction?

AKalinich-Luxoft · 2018-09-26T22:20:09Z

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

  POLICY_LIB_CHECK(false);
  std::string preloaded_file = get_settings().preloaded_pt_file();
  if (file_system::FileExists(preloaded_file)) {
+    sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov CallPolicyManagerFunction?

AKalinich-Luxoft · 2018-09-26T22:20:16Z

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

  POLICY_LIB_CHECK(false);
+  sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov @SNiukalov CallPolicyManagerFunction?

AKalinich-Luxoft · 2018-09-26T22:20:26Z

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

  POLICY_LIB_CHECK_VOID();
+  sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov CallPolicyManagerFunction?

AKalinich-Luxoft · 2018-09-26T22:27:15Z

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

  policy_manager_->Set(app_id, type, value);
 }

 void PolicyHandler::Add(const std::string& app_id,
                        usage_statistics::AppStopwatchId type,
                        int32_t timespan_seconds) {
  POLICY_LIB_CHECK();
+  sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov CallPolicyManagerFunction?

AKalinich-Luxoft · 2018-09-26T22:27:20Z

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

@@ -2059,12 +2141,14 @@ void PolicyHandler::UpdateHMILevel(ApplicationSharedPtr app,
 bool PolicyHandler::CheckModule(const PTString& app_id,
                                const PTString& module) {
  POLICY_LIB_CHECK(false);
+  sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov CallPolicyManagerFunction?

AKalinich-Luxoft · 2018-09-26T22:27:26Z

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

  return policy_manager_->CheckModule(app_id, module);
 }

 void PolicyHandler::OnRemoteAppPermissionsChanged(
    const std::string& device_id, const std::string& application_id) {
  POLICY_LIB_CHECK_VOID();
+  sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov CallPolicyManagerFunction?

AKalinich-Luxoft · 2018-09-26T22:27:31Z

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

  POLICY_LIB_CHECK(false);
+  sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov CallPolicyManagerFunction?

AKalinich-Luxoft · 2018-09-26T22:27:39Z

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

@@ -2117,16 +2200,19 @@ void PolicyHandler::SetDefaultHmiTypes(
                   std::back_inserter(hmi_types),
                   SmartObjectToInt());
  }
+
+  sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov CallPolicyManagerFunction?

@AKalinich-Luxoft Sorry, there was a small misunderstanding.
Replaced everything, where there is no need for statics_cast for the overload functions
in: e60c21e

SNiukalov · 2018-09-27T14:38:50Z

Rebuild Required

AByzhynar · 2018-10-09T15:38:20Z

src/components/application_manager/include/application_manager/policies/policy_handler.h

+  /**
+   * @brief Call PolicyManager function with sync_primitives::AutoLock
+   * @param func function PolicyManager to call
+   * @param args argmunets for call function


@SNiukalov typo

@AByzhynar
Fixed in: 8dd6d43

AByzhynar · 2018-10-09T15:41:25Z

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

      , state_controller_(state_controller) {}

  void operator()(const ApplicationSharedPtr& app) {
    DCHECK_OR_RETURN_VOID(policy_manager_);
    if (device_id_ == app->device()) {
      std::string hmi_level = "NONE";
      mobile_apis::HMILevel::eType default_mobile_hmi;
-      policy_manager_->GetDefaultHmi(app->policy_app_id(), &hmi_level);
+      {
+        sync_primitives::AutoLock lock(policy_manager_lock_);


@SNiukalov Add check for shared ptr validity before dereferencing

@AByzhynar
Fixed in: 741b392

AByzhynar · 2018-10-09T15:53:40Z

@SNiukalov Please add appropriate description with detailed fix information

LuxoftAKutsan

@SNiukalov this fix may significantly reduce the performance of SDL (replacing RW lock with recursive) . Also it adds not obvious template function for calling Policy Manager, and there no way to prevent using of the policy_manager_ variable without lock.

This fix required additional description

I suppose that here could be better solution, but probably this fix is OK,.
Please describe it carefully. Add the root cause of the issue, and how your fix prevent it.

EKuliiev · 2018-11-07T18:44:20Z

@SNiukalov see comments

EKuliiev · 2018-11-07T18:44:54Z

@SNiukalov actualize branch

SNiukalov · 2018-11-09T10:33:08Z

@AByzhynar @LuxoftAKutsan
Added detailed fix information.

SNiukalov · 2018-11-09T10:38:52Z

@EKuliiev
Branch updated.

…ject.

AByzhynar · 2018-11-09T11:38:23Z

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

  POLICY_LIB_CHECK_VOID();
+  std::string policy_table_status =


@SNiukalov Const?

@AByzhynar
Fixed in: 98dc17d

AByzhynar · 2018-11-09T11:38:39Z

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

@@ -964,8 +981,8 @@ void PolicyHandler::OnPendingPermissionChange(
    return;
  }

-  AppPermissions permissions =
-      policy_manager_->GetAppPermissionsChanges(policy_app_id);
+  AppPermissions permissions = CallPolicyManagerFunction(


@SNiukalov Const?

@AByzhynar
Fixed in: 98dc17d

AByzhynar · 2018-11-09T11:38:49Z

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

@@ -1068,11 +1086,14 @@ bool PolicyHandler::ReceiveMessageFromSDK(const std::string& file,
                                          const BinaryMessage& pt_string) {
  POLICY_LIB_CHECK(false);

-  bool ret = policy_manager_->LoadPT(file, pt_string);
+  bool ret = CallPolicyManagerFunction(&PolicyManager::LoadPT, file, pt_string);


@SNiukalov Const?

@AByzhynar
Fixed in: 98dc17d

AByzhynar · 2018-11-09T11:39:46Z

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

+    DeviceConsent consent =
+        CallPolicyManagerFunction(&PolicyManager::GetUserConsentForDevice,
+                                  permissions.deviceInfo.device_mac_address);
+
    permissions.isSDLAllowed = kDeviceAllowed == consent;


@SNiukalov Add round brackets here, please

@AByzhynar
Fixed in: 98dc17d

AByzhynar · 2018-11-09T11:40:03Z

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

-  std::string update_status = policy_manager_->ForcePTExchangeAtUserRequest();
+  LOG4CXX_DEBUG(logger_, "PT exchange at user request");
+
+  std::string update_status =


@SNiukalov Const?

@AByzhynar
Fixed in: 98dc17d

SNiukalov requested review from AKalinich-Luxoft, AByzhynar, LitvinenkoIra, Ypostolov, BSolonenko, a user and EKuliiev September 17, 2018 07:21

BSolonenko approved these changes Sep 19, 2018

View reviewed changes

AByzhynar suggested changes Sep 19, 2018

View reviewed changes

AKalinich-Luxoft suggested changes Sep 21, 2018

View reviewed changes

AKalinich-Luxoft suggested changes Sep 26, 2018

View reviewed changes

AKalinich-Luxoft approved these changes Sep 27, 2018

View reviewed changes

AByzhynar suggested changes Oct 9, 2018

View reviewed changes

AByzhynar requested review from AByzhynar and LuxoftAKutsan October 9, 2018 15:50

LuxoftAKutsan reviewed Oct 9, 2018

View reviewed changes

KrupenichOleg mentioned this pull request Oct 24, 2018

ATF script for defect 2596 smartdevicelink/sdl_atf_test_scripts#2123

Closed

EKuliiev approved these changes Nov 7, 2018

View reviewed changes

SNiukalov force-pushed the fix/vehicle_data_params_after_master_reset branch from e60c21e to a4f0569 Compare November 9, 2018 10:26

SNiukalov added 3 commits November 9, 2018 13:34

Fixed vehicle data after master reset.

87f3800

Removed direct use of the "acquire/release" of the synchronization ob…

8dd6d43

…ject.

Minimized use of policy_manager_lock_.

741b392

SNiukalov force-pushed the fix/vehicle_data_params_after_master_reset branch from a4f0569 to 741b392 Compare November 9, 2018 11:37

AByzhynar suggested changes Nov 9, 2018

View reviewed changes

Apply const.

98dc17d

AByzhynar approved these changes Dec 5, 2018

View reviewed changes

Ypostolov approved these changes Dec 5, 2018

View reviewed changes

LitvinenkoIra approved these changes Dec 5, 2018

View reviewed changes

		POLICY_LIB_CHECK(false);
		sync_primitives::AutoLock lock(policy_manager_lock_);

		POLICY_LIB_CHECK_VOID();
		sync_primitives::AutoLock lock(policy_manager_lock_);

Fix vehicle data params after master reset #2614

Are you sure you want to change the base?

Fix vehicle data params after master reset #2614

Conversation

SNiukalov commented Sep 17, 2018 • edited by AKalinich-Luxoft

Risk

Summary

CLA

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SNiukalov Sep 26, 2018 • edited

Choose a reason for hiding this comment

AKalinich-Luxoft Sep 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AKalinich-Luxoft Sep 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AKalinich-Luxoft left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SNiukalov commented Sep 27, 2018

Choose a reason for hiding this comment

SNiukalov Nov 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SNiukalov Nov 9, 2018 • edited

Choose a reason for hiding this comment

AByzhynar commented Oct 9, 2018

LuxoftAKutsan left a comment • edited

Choose a reason for hiding this comment

EKuliiev commented Nov 7, 2018

EKuliiev commented Nov 7, 2018

SNiukalov commented Nov 9, 2018

SNiukalov commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SNiukalov commented Sep 17, 2018 •

edited by AKalinich-Luxoft

SNiukalov Sep 26, 2018 •

edited

AKalinich-Luxoft Sep 21, 2018 •

edited

AKalinich-Luxoft Sep 21, 2018 •

edited

AKalinich-Luxoft left a comment •

edited

SNiukalov Nov 9, 2018 •

edited

SNiukalov Nov 9, 2018 •

edited

LuxoftAKutsan left a comment •

edited