Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vehicle data params after master reset #2614

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

SNiukalov
Copy link
Contributor

@SNiukalov SNiukalov commented Sep 17, 2018

Fixes #2596

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Why do you make Lock and Release twice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@SNiukalov SNiukalov Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@AKalinich-Luxoft AKalinich-Luxoft Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@AKalinich-Luxoft AKalinich-Luxoft Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@AKalinich-Luxoft AKalinich-Luxoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -2163,4 +2249,13 @@ void PolicyHandler::OnUpdateHMILevel(const std::string& device_id,
}
UpdateHMILevel(app, level);
}

template <typename Func, typename... Args>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov CallPolicyManagerFunction?

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_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov CallPolicyManagerFunction?

POLICY_LIB_CHECK(false);
sync_primitives::AutoLock lock(policy_manager_lock_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov @SNiukalov CallPolicyManagerFunction?

POLICY_LIB_CHECK_VOID();
sync_primitives::AutoLock lock(policy_manager_lock_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov CallPolicyManagerFunction?

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_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov CallPolicyManagerFunction?

@@ -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_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov CallPolicyManagerFunction?

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_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov CallPolicyManagerFunction?

POLICY_LIB_CHECK(false);
sync_primitives::AutoLock lock(policy_manager_lock_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov CallPolicyManagerFunction?

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

sync_primitives::AutoLock lock(policy_manager_lock_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov CallPolicyManagerFunction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

Rebuild Required

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov typo

Copy link
Contributor Author

@SNiukalov SNiukalov Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in: 8dd6d43

, 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_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Add check for shared ptr validity before dereferencing

Copy link
Contributor Author

@SNiukalov SNiukalov Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in: 741b392

@AByzhynar
Copy link
Contributor

@SNiukalov Please add appropriate description with detailed fix information

Copy link
Contributor

@LuxoftAKutsan LuxoftAKutsan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

EKuliiev commented Nov 7, 2018

@SNiukalov see comments

@EKuliiev
Copy link
Contributor

EKuliiev commented Nov 7, 2018

@SNiukalov actualize branch

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

@AByzhynar @LuxoftAKutsan
Added detailed fix information.

@SNiukalov
Copy link
Contributor Author

@EKuliiev
Branch updated.

@SNiukalov SNiukalov force-pushed the fix/vehicle_data_params_after_master_reset branch from a4f0569 to 741b392 Compare November 9, 2018 11:37
POLICY_LIB_CHECK_VOID();
std::string policy_table_status =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Const?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in: 98dc17d

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

AppPermissions permissions =
policy_manager_->GetAppPermissionsChanges(policy_app_id);
AppPermissions permissions = CallPolicyManagerFunction(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Const?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in: 98dc17d

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Const?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in: 98dc17d

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

permissions.isSDLAllowed = kDeviceAllowed == consent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Add round brackets here, please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in: 98dc17d

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

std::string update_status =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Const?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in: 98dc17d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants