From 44007f451d947d97870624f4990c1033d3808276 Mon Sep 17 00:00:00 2001 From: Conlain Kelly Date: Thu, 31 May 2018 14:54:52 -0400 Subject: Begin changing locks over, very segfault-y right now This required modification of the condition variable class as well as every instance of a recursive lock --- .../app_launch/app_launch_data_json.h | 2 +- .../application_manager/application_data_impl.h | 4 +- .../application_manager/application_manager_impl.h | 4 +- .../mobile/create_interaction_choice_set_request.h | 2 +- .../event_engine/event_dispatcher_impl.h | 2 +- .../application_manager/policies/policy_handler.h | 2 +- .../resumption/resumption_data.h | 2 +- .../src/app_launch/app_launch_data_json.cc | 1 - .../src/application_data_impl.cc | 2 - .../src/application_manager_impl.cc | 2 - .../create_interaction_choice_set_request.cc | 3 +- .../src/event_engine/event_dispatcher_impl.cc | 2 +- .../src/policies/policy_handler.cc | 1 - .../src/resumption/resume_ctrl_impl.cc | 1 - .../src/resumption/resumption_data.cc | 2 +- .../test/message_helper/message_helper_test.cc | 14 +- .../include/connection_handler/connection.h | 2 +- .../include/connection_handler/heartbeat_monitor.h | 2 +- .../connection_handler/src/connection.cc | 2 +- .../connection_handler/src/heartbeat_monitor.cc | 1 - .../include/utils/conditional_variable.h | 19 +-- src/components/include/utils/data_accessor.h | 6 +- src/components/include/utils/lock.h | 142 +++++++++------- src/components/include/utils/lock_posix.h | 184 +++++++++++++++++++++ .../policy_regular/include/policy/cache_manager.h | 2 +- .../policy/policy_regular/src/cache_manager.cc | 18 +- src/components/utils/CMakeLists.txt | 5 +- .../utils/src/conditional_variable_posix.cc | 138 +++++++--------- src/components/utils/src/lock_boost.cc | 143 ++++++++++++++++ src/components/utils/src/lock_posix.cc | 4 +- src/components/utils/test/lock_boost_test.cc | 103 ++++++++++++ src/components/utils/test/lock_posix_test.cc | 4 +- 32 files changed, 622 insertions(+), 199 deletions(-) create mode 100644 src/components/include/utils/lock_posix.h create mode 100644 src/components/utils/src/lock_boost.cc create mode 100644 src/components/utils/test/lock_boost_test.cc (limited to 'src/components') diff --git a/src/components/application_manager/include/application_manager/app_launch/app_launch_data_json.h b/src/components/application_manager/include/application_manager/app_launch/app_launch_data_json.h index 61117e552b..44b70ee305 100644 --- a/src/components/application_manager/include/application_manager/app_launch/app_launch_data_json.h +++ b/src/components/application_manager/include/application_manager/app_launch/app_launch_data_json.h @@ -141,7 +141,7 @@ class AppLaunchDataJson : public AppLaunchDataImpl { /** * @brief lock to protected common data */ - mutable sync_primitives::Lock app_launch_json_lock_; + mutable sync_primitives::RecursiveLock app_launch_json_lock_; /** * @brief ponter to Last State object diff --git a/src/components/application_manager/include/application_manager/application_data_impl.h b/src/components/application_manager/include/application_manager/application_data_impl.h index dc9be4e1d8..3690508f74 100644 --- a/src/components/application_manager/include/application_manager/application_data_impl.h +++ b/src/components/application_manager/include/application_manager/application_data_impl.h @@ -275,13 +275,13 @@ class DynamicApplicationDataImpl : public virtual Application { std::string display_layout_; CommandsMap commands_; - mutable sync_primitives::Lock commands_lock_; + mutable sync_primitives::RecursiveLock commands_lock_; SubMenuMap sub_menu_; mutable sync_primitives::Lock sub_menu_lock_; ChoiceSetMap choice_set_map_; mutable sync_primitives::Lock choice_set_map_lock_; PerformChoiceSetMap performinteraction_choice_set_map_; - mutable sync_primitives::Lock performinteraction_choice_set_lock_; + mutable sync_primitives::RecursiveLock performinteraction_choice_set_lock_; uint32_t is_perform_interaction_active_; bool is_reset_global_properties_active_; int32_t perform_interaction_mode_; diff --git a/src/components/application_manager/include/application_manager/application_manager_impl.h b/src/components/application_manager/include/application_manager/application_manager_impl.h index 286ad87018..985932a172 100644 --- a/src/components/application_manager/include/application_manager/application_manager_impl.h +++ b/src/components/application_manager/include/application_manager/application_manager_impl.h @@ -1698,7 +1698,7 @@ class ApplicationManagerImpl ForbiddenApps forbidden_applications; // Lock for applications list - mutable sync_primitives::Lock applications_list_lock_; + mutable sync_primitives::RecursiveLock applications_list_lock_; mutable sync_primitives::Lock apps_to_register_list_lock_; mutable sync_primitives::Lock subscribed_way_points_apps_lock_; @@ -1799,7 +1799,7 @@ class ApplicationManagerImpl std::vector timer_pool_; sync_primitives::Lock timer_pool_lock_; - sync_primitives::Lock stopping_application_mng_lock_; + sync_primitives::RecursiveLock stopping_application_mng_lock_; StateControllerImpl state_ctrl_; std::auto_ptr app_launch_dto_; std::auto_ptr app_launch_ctrl_; diff --git a/src/components/application_manager/include/application_manager/commands/mobile/create_interaction_choice_set_request.h b/src/components/application_manager/include/application_manager/commands/mobile/create_interaction_choice_set_request.h index c00d310e7d..f79a2e7225 100644 --- a/src/components/application_manager/include/application_manager/commands/mobile/create_interaction_choice_set_request.h +++ b/src/components/application_manager/include/application_manager/commands/mobile/create_interaction_choice_set_request.h @@ -136,7 +136,7 @@ class CreateInteractionChoiceSetRequest : public CommandRequestImpl { volatile bool is_timed_out_; sync_primitives::Lock is_timed_out_lock_; - sync_primitives::Lock vr_commands_lock_; + sync_primitives::RecursiveLock vr_commands_lock_; /* * @brief Sends VR AddCommand request to HMI * diff --git a/src/components/application_manager/include/application_manager/event_engine/event_dispatcher_impl.h b/src/components/application_manager/include/application_manager/event_engine/event_dispatcher_impl.h index 4cdbb902de..ed923369ac 100644 --- a/src/components/application_manager/include/application_manager/event_engine/event_dispatcher_impl.h +++ b/src/components/application_manager/include/application_manager/event_engine/event_dispatcher_impl.h @@ -118,7 +118,7 @@ class EventDispatcherImpl : public EventDispatcher { private: // Members section sync_primitives::Lock state_lock_; - sync_primitives::Lock observer_lock_; + sync_primitives::RecursiveLock observer_lock_; EventObserverMap observers_event_; ObserverVector observers_; }; diff --git a/src/components/application_manager/include/application_manager/policies/policy_handler.h b/src/components/application_manager/include/application_manager/policies/policy_handler.h index b4653c6cb2..4be01214ab 100644 --- a/src/components/application_manager/include/application_manager/policies/policy_handler.h +++ b/src/components/application_manager/include/application_manager/policies/policy_handler.h @@ -726,7 +726,7 @@ class PolicyHandler : public PolicyHandlerInterface, std::map app_to_device_link_; // Lock for app to device list - sync_primitives::Lock app_to_device_link_lock_; + sync_primitives::RecursiveLock app_to_device_link_lock_; utils::SharedPtr statistic_manager_impl_; const PolicySettings& settings_; diff --git a/src/components/application_manager/include/application_manager/resumption/resumption_data.h b/src/components/application_manager/include/application_manager/resumption/resumption_data.h index bee2bce570..5ecfaf3c11 100644 --- a/src/components/application_manager/include/application_manager/resumption/resumption_data.h +++ b/src/components/application_manager/include/application_manager/resumption/resumption_data.h @@ -276,7 +276,7 @@ class ResumptionData { ++first; } } - mutable sync_primitives::Lock resumption_lock_; + mutable sync_primitives::RecursiveLock resumption_lock_; const application_manager::ApplicationManager& application_manager_; }; } // namespace resumption diff --git a/src/components/application_manager/src/app_launch/app_launch_data_json.cc b/src/components/application_manager/src/app_launch/app_launch_data_json.cc index 7599dcccb3..99a6416a69 100644 --- a/src/components/application_manager/src/app_launch/app_launch_data_json.cc +++ b/src/components/application_manager/src/app_launch/app_launch_data_json.cc @@ -44,7 +44,6 @@ CREATE_LOGGERPTR_GLOBAL(logger_, "AppLaunch") AppLaunchDataJson::AppLaunchDataJson(const AppLaunchSettings& settings, resumption::LastState& last_state) : AppLaunchDataImpl(settings) - , app_launch_json_lock_(true) , last_state_(last_state) {} AppLaunchDataJson::~AppLaunchDataJson() {} diff --git a/src/components/application_manager/src/application_data_impl.cc b/src/components/application_manager/src/application_data_impl.cc index 226c83dbf6..931b3571fb 100644 --- a/src/components/application_manager/src/application_data_impl.cc +++ b/src/components/application_manager/src/application_data_impl.cc @@ -177,11 +177,9 @@ DynamicApplicationDataImpl::DynamicApplicationDataImpl() , night_color_scheme_(NULL) , display_layout_("") , commands_() - , commands_lock_(true) , sub_menu_() , choice_set_map_() , performinteraction_choice_set_map_() - , performinteraction_choice_set_lock_(true) , is_perform_interaction_active_(false) , is_reset_global_properties_active_(false) , perform_interaction_mode_(-1) {} diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index 3ead8fe65e..7d1b1aaeda 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -141,7 +141,6 @@ ApplicationManagerImpl::ApplicationManagerImpl( const ApplicationManagerSettings& am_settings, const policy::PolicySettings& policy_settings) : settings_(am_settings) - , applications_list_lock_(true) , audio_pass_thru_active_(false) , audio_pass_thru_app_id_(0) , driver_distraction_state_( @@ -168,7 +167,6 @@ ApplicationManagerImpl::ApplicationManagerImpl( , resume_ctrl_(new resumption::ResumeCtrlImpl(*this)) , navi_close_app_timeout_(am_settings.stop_streaming_timeout()) , navi_end_stream_timeout_(am_settings.stop_streaming_timeout()) - , stopping_application_mng_lock_(true) , state_ctrl_(*this) #ifdef TELEMETRY_MONITOR , metric_observer_(NULL) diff --git a/src/components/application_manager/src/commands/mobile/create_interaction_choice_set_request.cc b/src/components/application_manager/src/commands/mobile/create_interaction_choice_set_request.cc index 20387ef230..2ee3fd8bb8 100644 --- a/src/components/application_manager/src/commands/mobile/create_interaction_choice_set_request.cc +++ b/src/components/application_manager/src/commands/mobile/create_interaction_choice_set_request.cc @@ -53,8 +53,7 @@ CreateInteractionChoiceSetRequest::CreateInteractionChoiceSetRequest( , expected_chs_count_(0) , received_chs_count_(0) , error_from_hmi_(false) - , is_timed_out_(false) - , vr_commands_lock_(true) {} + , is_timed_out_(false) {} CreateInteractionChoiceSetRequest::~CreateInteractionChoiceSetRequest() { LOG4CXX_AUTO_TRACE(logger_); diff --git a/src/components/application_manager/src/event_engine/event_dispatcher_impl.cc b/src/components/application_manager/src/event_engine/event_dispatcher_impl.cc index b19a6f9194..505465d001 100644 --- a/src/components/application_manager/src/event_engine/event_dispatcher_impl.cc +++ b/src/components/application_manager/src/event_engine/event_dispatcher_impl.cc @@ -40,7 +40,7 @@ namespace event_engine { using namespace sync_primitives; EventDispatcherImpl::EventDispatcherImpl() - : state_lock_(false), observer_lock_(true), observers_event_() {} + : observers_event_() {} EventDispatcherImpl::~EventDispatcherImpl() {} diff --git a/src/components/application_manager/src/policies/policy_handler.cc b/src/components/application_manager/src/policies/policy_handler.cc index 559b9c0035..4c4b5d47da 100644 --- a/src/components/application_manager/src/policies/policy_handler.cc +++ b/src/components/application_manager/src/policies/policy_handler.cc @@ -325,7 +325,6 @@ PolicyHandler::PolicyHandler(const PolicySettings& settings, : AsyncRunner("PolicyHandler async runner thread") , dl_handle_(0) , last_activated_app_id_(0) - , app_to_device_link_lock_(true) , statistic_manager_impl_(utils::MakeShared(this)) , settings_(settings) , application_manager_(application_manager) {} diff --git a/src/components/application_manager/src/resumption/resume_ctrl_impl.cc b/src/components/application_manager/src/resumption/resume_ctrl_impl.cc index e3fd423970..e370d345b8 100644 --- a/src/components/application_manager/src/resumption/resume_ctrl_impl.cc +++ b/src/components/application_manager/src/resumption/resume_ctrl_impl.cc @@ -57,7 +57,6 @@ CREATE_LOGGERPTR_GLOBAL(logger_, "Resumption") ResumeCtrlImpl::ResumeCtrlImpl(ApplicationManager& application_manager) : event_engine::EventObserver(application_manager.event_dispatcher()) - , queue_lock_(false) , restore_hmi_level_timer_( "RsmCtrlRstore", new timer::TimerTaskImpl( diff --git a/src/components/application_manager/src/resumption/resumption_data.cc b/src/components/application_manager/src/resumption/resumption_data.cc index bd5bdbddab..ca049917c4 100644 --- a/src/components/application_manager/src/resumption/resumption_data.cc +++ b/src/components/application_manager/src/resumption/resumption_data.cc @@ -41,7 +41,7 @@ CREATE_LOGGERPTR_GLOBAL(logger_, "Resumption") ResumptionData::ResumptionData( const application_manager::ApplicationManager& application_manager) - : resumption_lock_(true), application_manager_(application_manager) {} + : application_manager_(application_manager) {} smart_objects::SmartObject ResumptionData::GetApplicationCommands( app_mngr::ApplicationConstSharedPtr application) const { diff --git a/src/components/application_manager/test/message_helper/message_helper_test.cc b/src/components/application_manager/test/message_helper/message_helper_test.cc index 289e4f5d05..dc4b3f48bf 100644 --- a/src/components/application_manager/test/message_helper/message_helper_test.cc +++ b/src/components/application_manager/test/message_helper/message_helper_test.cc @@ -236,7 +236,7 @@ TEST(MessageHelperTestCreate, CreateAddCommandRequestToHMI_SendSmartObject_Empty) { MockApplicationSharedPtr appSharedMock = utils::MakeShared(); ::application_manager::CommandsMap vis; - DataAccessor data_accessor(vis, true); + DataAccessor data_accessor(vis, sync_primitives::RecursiveLock()); EXPECT_CALL(*appSharedMock, commands_map()).WillOnce(Return(data_accessor)); application_manager_test::MockApplicationManager mock_application_manager; @@ -251,7 +251,7 @@ TEST(MessageHelperTestCreate, CreateAddCommandRequestToHMI_SendSmartObject_Equal) { MockApplicationSharedPtr appSharedMock = utils::MakeShared(); CommandsMap vis; - DataAccessor data_accessor(vis, true); + DataAccessor data_accessor(vis, sync_primitives::RecursiveLock()); smart_objects::SmartObjectSPtr smartObjectPtr = utils::MakeShared(); @@ -292,7 +292,7 @@ TEST(MessageHelperTestCreate, CreateAddVRCommandRequestFromChoiceToHMI_SendEmptyData_EmptyList) { MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::ChoiceSetMap vis; - DataAccessor< ::application_manager::ChoiceSetMap> data_accessor(vis, true); + DataAccessor< ::application_manager::ChoiceSetMap> data_accessor(vis, sync_primitives::RecursiveLock()); EXPECT_CALL(*appSharedMock, choice_set_map()).WillOnce(Return(data_accessor)); application_manager_test::MockApplicationManager mock_application_manager; @@ -307,7 +307,7 @@ TEST(MessageHelperTestCreate, CreateAddVRCommandRequestFromChoiceToHMI_SendObject_EqualList) { MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::ChoiceSetMap vis; - DataAccessor< ::application_manager::ChoiceSetMap> data_accessor(vis, true); + DataAccessor< ::application_manager::ChoiceSetMap> data_accessor(vis, sync_primitives::RecursiveLock()); smart_objects::SmartObjectSPtr smartObjectPtr = utils::MakeShared(); @@ -353,7 +353,7 @@ TEST(MessageHelperTestCreate, TEST(MessageHelperTestCreate, CreateAddSubMenuRequestToHMI_SendObject_Equal) { MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::SubMenuMap vis; - DataAccessor< ::application_manager::SubMenuMap> data_accessor(vis, true); + DataAccessor< ::application_manager::SubMenuMap> data_accessor(vis, sync_primitives::RecursiveLock()); smart_objects::SmartObjectSPtr smartObjectPtr = utils::MakeShared(); @@ -392,7 +392,7 @@ TEST(MessageHelperTestCreate, CreateAddSubMenuRequestToHMI_SendEmptyMap_EmptySmartObjectList) { MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::SubMenuMap vis; - DataAccessor< ::application_manager::SubMenuMap> data_accessor(vis, true); + DataAccessor< ::application_manager::SubMenuMap> data_accessor(vis, sync_primitives::RecursiveLock()); EXPECT_CALL(*appSharedMock, sub_menu_map()).WillOnce(Return(data_accessor)); @@ -713,7 +713,7 @@ TEST_F(MessageHelperTest, // Creating data acessor application_manager::VehicleInfoSubscriptions vis; DataAccessor data_accessor( - vis, true); + vis, sync_primitives::RecursiveLock()); // Calls for ApplicationManager EXPECT_CALL(*appSharedMock, app_id()).WillOnce(Return(1u)); EXPECT_CALL(*appSharedMock, SubscribedIVI()).WillOnce(Return(data_accessor)); diff --git a/src/components/connection_handler/include/connection_handler/connection.h b/src/components/connection_handler/include/connection_handler/connection.h index 9b72d60776..d46f574707 100644 --- a/src/components/connection_handler/include/connection_handler/connection.h +++ b/src/components/connection_handler/include/connection_handler/connection.h @@ -304,7 +304,7 @@ class Connection { */ SessionMap session_map_; - mutable sync_primitives::Lock session_map_lock_; + mutable sync_primitives::RecursiveLock session_map_lock_; /** * @brief monitor that closes connection if there is no traffic over it diff --git a/src/components/connection_handler/include/connection_handler/heartbeat_monitor.h b/src/components/connection_handler/include/connection_handler/heartbeat_monitor.h index 4d1d07112c..1a0bfce264 100644 --- a/src/components/connection_handler/include/connection_handler/heartbeat_monitor.h +++ b/src/components/connection_handler/include/connection_handler/heartbeat_monitor.h @@ -111,7 +111,7 @@ class HeartBeatMonitor : public threads::ThreadDelegate { typedef std::map SessionMap; SessionMap sessions_; - sync_primitives::Lock sessions_list_lock_; // recurcive + sync_primitives::RecursiveLock sessions_list_lock_; // recurcive sync_primitives::Lock main_thread_lock_; mutable sync_primitives::Lock heartbeat_timeout_seconds_lock_; sync_primitives::ConditionalVariable heartbeat_monitor_; diff --git a/src/components/connection_handler/src/connection.cc b/src/components/connection_handler/src/connection.cc index 16b88c4164..ae7eb1be18 100644 --- a/src/components/connection_handler/src/connection.cc +++ b/src/components/connection_handler/src/connection.cc @@ -81,7 +81,7 @@ Connection::Connection(ConnectionHandle connection_handle, : connection_handler_(connection_handler) , connection_handle_(connection_handle) , connection_device_handle_(connection_device_handle) - , session_map_lock_(true) + , session_map_lock_() , heartbeat_timeout_(heartbeat_timeout) { LOG4CXX_AUTO_TRACE(logger_); DCHECK(connection_handler_); diff --git a/src/components/connection_handler/src/heartbeat_monitor.cc b/src/components/connection_handler/src/heartbeat_monitor.cc index f3a2322810..50af5a042a 100644 --- a/src/components/connection_handler/src/heartbeat_monitor.cc +++ b/src/components/connection_handler/src/heartbeat_monitor.cc @@ -47,7 +47,6 @@ HeartBeatMonitor::HeartBeatMonitor(uint32_t heartbeat_timeout_mseconds, Connection* connection) : default_heartbeat_timeout_(heartbeat_timeout_mseconds) , connection_(connection) - , sessions_list_lock_(true) , run_(true) {} void HeartBeatMonitor::Process() { diff --git a/src/components/include/utils/conditional_variable.h b/src/components/include/utils/conditional_variable.h index f54a22e993..a29f255dbf 100644 --- a/src/components/include/utils/conditional_variable.h +++ b/src/components/include/utils/conditional_variable.h @@ -32,24 +32,13 @@ #ifndef SRC_COMPONENTS_INCLUDE_UTILS_CONDITIONAL_VARIABLE_H_ #define SRC_COMPONENTS_INCLUDE_UTILS_CONDITIONAL_VARIABLE_H_ -#if defined(OS_POSIX) -#include -#else -#error Please implement conditional variable for your OS -#endif #include +#include +#include "utils/lock.h" #include "utils/macro.h" namespace sync_primitives { -class AutoLock; -class Lock; - -namespace impl { -#if defined(OS_POSIX) -typedef pthread_cond_t PlatformConditionalVariable; -#endif -} // namespace impl /* * Conditional variable wrapper @@ -82,11 +71,11 @@ class ConditionalVariable { // Wait forever or up to milliseconds time limit bool Wait(AutoLock& auto_lock); - bool Wait(Lock& lock); + bool Wait(BaseLock& lock); WaitStatus WaitFor(AutoLock& auto_lock, uint32_t milliseconds); private: - impl::PlatformConditionalVariable cond_var_; + boost::condition_variable_any cond_var_; private: DISALLOW_COPY_AND_ASSIGN(ConditionalVariable); diff --git a/src/components/include/utils/data_accessor.h b/src/components/include/utils/data_accessor.h index 9be28a638b..6239812678 100644 --- a/src/components/include/utils/data_accessor.h +++ b/src/components/include/utils/data_accessor.h @@ -39,9 +39,9 @@ template class DataAccessor { public: - DataAccessor(const T& data, const sync_primitives::Lock& lock) + DataAccessor(const T& data, const sync_primitives::BaseLock& lock) : data_(data) - , lock_(const_cast(lock)) + , lock_(const_cast(lock)) , counter_(new uint32_t(0)) { lock_.Acquire(); } @@ -65,7 +65,7 @@ class DataAccessor { private: void* operator new(size_t size); const T& data_; - sync_primitives::Lock& lock_; + sync_primitives::BaseLock& lock_; utils::SharedPtr counter_; }; diff --git a/src/components/include/utils/lock.h b/src/components/include/utils/lock.h index e615a58f9d..4ace34d71e 100644 --- a/src/components/include/utils/lock.h +++ b/src/components/include/utils/lock.h @@ -32,24 +32,17 @@ #ifndef SRC_COMPONENTS_INCLUDE_UTILS_LOCK_H_ #define SRC_COMPONENTS_INCLUDE_UTILS_LOCK_H_ -#if defined(OS_POSIX) -#include -#include -#else -#error Please implement lock for your OS -#endif #include -#include "utils/macro.h" +#include +#include #include "utils/atomic.h" +#include "utils/macro.h" #include "utils/memory_barrier.h" +#include -namespace sync_primitives { +using boost::system::error_code; -namespace impl { -#if defined(OS_POSIX) -typedef pthread_mutex_t PlatformMutex; -#endif -} // namespace impl +namespace sync_primitives { class SpinMutex { public: @@ -78,71 +71,89 @@ class SpinMutex { volatile unsigned int state_; }; -/* Platform-indepenednt NON-RECURSIVE lock (mutex) wrapper - Please use AutoLock to ackquire and (automatically) release it - It eases balancing of multple lock taking/releasing and makes it - Impossible to forget to release the lock: - ... - ConcurentlyAccessedData data_; - sync_primitives::Lock data_lock_; - ... - { - sync_primitives::AutoLock auto_lock(data_lock_); - data_.ReadOrWriteData(); - } // lock is automatically released here -*/ -class Lock { +/* Abstract base class that allows AutoLock to handle both recursive and + * non-recursive locks + */ +class BaseLock { public: - Lock(); - Lock(bool is_recursive); - ~Lock(); - + BaseLock() : lock_taken_(0) {} // Ackquire the lock. Must be called only once on a thread. // Please consider using AutoLock to capture it. - void Acquire(); + virtual void Acquire() = 0; // Release the lock. Must be called only once on a thread after lock. // was acquired. Please consider using AutoLock to automatically release // the lock - void Release(); + virtual void Release() = 0; // Try if lock can be captured and lock it if it was possible. // If it captured, lock must be manually released calling to Release // when protected resource access was finished. // @returns wether lock was captured. - bool Try(); - - private: - impl::PlatformMutex mutex_; + virtual bool Try() = 0; -#ifndef NDEBUG + protected: /** - * @brief Basic debugging aid, a flag that signals wether this lock is - * currently taken - * Allows detection of abandoned and recursively captured mutexes - */ + * @brief Basic debugging aid, a flag that signals wether this lock is + * currently taken + * Allows detection of abandoned and recursively captured mutexes + */ uint32_t lock_taken_; - /** - * @brief Describe if mutex is recurcive or not - */ - bool is_mutex_recursive_; + // Ensures safety in locking + virtual void AssertTakenAndMarkFree() = 0; + virtual void AssertFreeAndMarkTaken() = 0; - void AssertFreeAndMarkTaken(); - void AssertTakenAndMarkFree(); -#else - void AssertFreeAndMarkTaken() {} - void AssertTakenAndMarkFree() {} -#endif +friend class ConditionalVariable; +}; - void Init(bool is_recursive); +/* + * Platform-indepenednt NON-RECURSIVE lock (mutex) wrapper + */ +class Lock : public BaseLock { + public: + Lock(); + ~Lock(); - friend class ConditionalVariable; + virtual void Acquire(); + + virtual void Release(); + + virtual bool Try(); + + private: + virtual void AssertTakenAndMarkFree(); + virtual void AssertFreeAndMarkTaken(); + boost::mutex mutex_; DISALLOW_COPY_AND_ASSIGN(Lock); + friend class ConditionalVariable; +}; + +/* + * Platform-indepenednt RECURSIVE lock (mutex) wrapper + */ +class RecursiveLock : public BaseLock { + public: + RecursiveLock(); + ~RecursiveLock(); + + virtual void Acquire(); + + virtual void Release(); + + virtual bool Try(); + + private: + virtual void AssertTakenAndMarkFree(); + virtual void AssertFreeAndMarkTaken(); + boost::recursive_mutex mutex_; + DISALLOW_COPY_AND_ASSIGN(RecursiveLock); + friend class ConditionalVariable; }; // This class is used to automatically acquire and release the a lock class AutoLock { public: - explicit AutoLock(Lock& lock) : lock_(lock) { + explicit AutoLock(BaseLock& lock) : lock_(lock) { + std::cerr << "lock is at " << &lock << std::endl; lock_.Acquire(); } ~AutoLock() { @@ -150,21 +161,32 @@ class AutoLock { } private: - Lock& GetLock() { + BaseLock& GetLock() { return lock_; } - Lock& lock_; + BaseLock& lock_; private: friend class AutoUnlock; friend class ConditionalVariable; DISALLOW_COPY_AND_ASSIGN(AutoLock); }; - +/* Please use AutoLock to ackquire and (automatically) release it + * It eases balancing of multple lock taking/releasing and makes it + * Impossible to forget to release the lock: + * ... + * ConcurentlyAccessedData data_; + * sync_primitives::Lock data_lock_; + * ... + * { + * sync_primitives::AutoLock auto_lock(data_lock_); + * data_.ReadOrWriteData(); + * } // lock is automatically released here + */ // This class is used to temporarly unlock autolocked lock class AutoUnlock { public: - explicit AutoUnlock(Lock& lock) : lock_(lock) { + explicit AutoUnlock(BaseLock& lock) : lock_(lock) { lock_.Release(); } explicit AutoUnlock(AutoLock& lock) : lock_(lock.GetLock()) { @@ -175,7 +197,7 @@ class AutoUnlock { } private: - Lock& lock_; + BaseLock& lock_; private: DISALLOW_COPY_AND_ASSIGN(AutoUnlock); diff --git a/src/components/include/utils/lock_posix.h b/src/components/include/utils/lock_posix.h new file mode 100644 index 0000000000..ae8b64959f --- /dev/null +++ b/src/components/include/utils/lock_posix.h @@ -0,0 +1,184 @@ +/* + * Copyright (c) 2013, Ford Motor Company + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following + * disclaimer in the documentation and/or other materials provided with the + * distribution. + * + * Neither the name of the Ford Motor Company nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ +#ifndef SRC_COMPONENTS_INCLUDE_UTILS_LOCK_H_ +#define SRC_COMPONENTS_INCLUDE_UTILS_LOCK_H_ + +#if defined(OS_POSIX) +#include +#include +#else +#error Please implement lock for your OS +#endif +#include +#include "utils/macro.h" +#include "utils/atomic.h" +#include "utils/memory_barrier.h" + +namespace sync_primitives_posix { + +namespace impl { +#if defined(OS_POSIX) +typedef pthread_mutex_t PlatformMutex; +#endif +} // namespace impl + +class SpinMutex { + public: + SpinMutex() : state_(0) {} + void Lock() { + // Comment below add exception for lint error + // Reason: FlexeLint doesn't know about compiler's built-in instructions + /*lint -e1055*/ + if (atomic_post_set(&state_) == 0) { + return; + } + for (;;) { + sched_yield(); + /*lint -e1055*/ + if (state_ == 0 && atomic_post_set(&state_) == 0) { + return; + } + } + } + void Unlock() { + state_ = 0; + } + ~SpinMutex() {} + + private: + volatile unsigned int state_; +}; + +/* Platform-indepenednt NON-RECURSIVE lock (mutex) wrapper + Please use AutoLock to ackquire and (automatically) release it + It eases balancing of multple lock taking/releasing and makes it + Impossible to forget to release the lock: + ... + ConcurentlyAccessedData data_; + sync_primitives::Lock data_lock_; + ... + { + sync_primitives::AutoLock auto_lock(data_lock_); + data_.ReadOrWriteData(); + } // lock is automatically released here +*/ +class Lock { + public: + Lock(); + Lock(bool is_recursive); + ~Lock(); + + // Ackquire the lock. Must be called only once on a thread. + // Please consider using AutoLock to capture it. + void Acquire(); + // Release the lock. Must be called only once on a thread after lock. + // was acquired. Please consider using AutoLock to automatically release + // the lock + void Release(); + // Try if lock can be captured and lock it if it was possible. + // If it captured, lock must be manually released calling to Release + // when protected resource access was finished. + // @returns wether lock was captured. + bool Try(); + + private: + impl::PlatformMutex mutex_; + +#ifndef NDEBUG + /** + * @brief Basic debugging aid, a flag that signals wether this lock is + * currently taken + * Allows detection of abandoned and recursively captured mutexes + */ + uint32_t lock_taken_; + + /** + * @brief Describe if mutex is recurcive or not + */ + bool is_mutex_recursive_; + + void AssertFreeAndMarkTaken(); + void AssertTakenAndMarkFree(); +#else + void AssertFreeAndMarkTaken() {} + void AssertTakenAndMarkFree() {} +#endif + + void Init(bool is_recursive); + + friend class ConditionalVariable; + DISALLOW_COPY_AND_ASSIGN(Lock); +}; + +// This class is used to automatically acquire and release the a lock +class AutoLock { + public: + explicit AutoLock(Lock& lock) : lock_(lock) { + lock_.Acquire(); + } + ~AutoLock() { + lock_.Release(); + } + + private: + Lock& GetLock() { + return lock_; + } + Lock& lock_; + + private: + friend class AutoUnlock; + friend class ConditionalVariable; + DISALLOW_COPY_AND_ASSIGN(AutoLock); +}; + +// This class is used to temporarly unlock autolocked lock +class AutoUnlock { + public: + explicit AutoUnlock(Lock& lock) : lock_(lock) { + lock_.Release(); + } + explicit AutoUnlock(AutoLock& lock) : lock_(lock.GetLock()) { + lock_.Release(); + } + ~AutoUnlock() { + lock_.Acquire(); + } + + private: + Lock& lock_; + + private: + DISALLOW_COPY_AND_ASSIGN(AutoUnlock); +}; +} // namespace sync_primitives +#endif // SRC_COMPONENTS_INCLUDE_UTILS_LOCK_H_ diff --git a/src/components/policy/policy_regular/include/policy/cache_manager.h b/src/components/policy/policy_regular/include/policy/cache_manager.h index 8c0acd44d2..464aa711bd 100644 --- a/src/components/policy/policy_regular/include/policy/cache_manager.h +++ b/src/components/policy/policy_regular/include/policy/cache_manager.h @@ -756,7 +756,7 @@ class CacheManager : public CacheManagerInterface { typedef std::set UnpairedDevices; UnpairedDevices is_unpaired_; - mutable sync_primitives::Lock cache_lock_; + mutable sync_primitives::RecursiveLock cache_lock_; sync_primitives::Lock unpaired_lock_; typedef std::map AppCalculatedPermissions; diff --git a/src/components/policy/policy_regular/src/cache_manager.cc b/src/components/policy/policy_regular/src/cache_manager.cc index 6a142374d5..b10cd51e8b 100644 --- a/src/components/policy/policy_regular/src/cache_manager.cc +++ b/src/components/policy/policy_regular/src/cache_manager.cc @@ -33,18 +33,18 @@ #include "policy/cache_manager.h" #include -#include -#include #include +#include +#include #include -#include "utils/file_system.h" -#include "json/reader.h" #include "json/features.h" +#include "json/reader.h" #include "json/writer.h" -#include "utils/logger.h" #include "utils/date_time.h" +#include "utils/file_system.h" #include "utils/gen_hash.h" +#include "utils/logger.h" #include "utils/macro.h" #include "utils/threads/thread.h" #include "utils/threads/thread_delegate.h" @@ -102,8 +102,7 @@ CacheManager::CacheManager() : CacheManagerInterface() , pt_(new policy_table::Table) , backup_(new SQLPTRepresentation()) - , update_required(false) - , cache_lock_(true) { + , update_required(false) { LOG4CXX_AUTO_TRACE(logger_); backuper_ = new BackgroundBackuper(this); backup_thread_ = threads::CreateThread("Backup thread", backuper_); @@ -1635,8 +1634,9 @@ void CacheManager::MergeFG(const policy_table::PolicyTable& new_pt, void CacheManager::MergeAP(const policy_table::PolicyTable& new_pt, policy_table::PolicyTable& pt) { LOG4CXX_AUTO_TRACE(logger_); - pt.app_policies_section.device = const_cast( - new_pt).app_policies_section.device; + pt.app_policies_section.device = + const_cast(new_pt) + .app_policies_section.device; pt.app_policies_section.apps[kDefaultId] = const_cast(new_pt) diff --git a/src/components/utils/CMakeLists.txt b/src/components/utils/CMakeLists.txt index 51835c125a..46acf76e89 100644 --- a/src/components/utils/CMakeLists.txt +++ b/src/components/utils/CMakeLists.txt @@ -101,7 +101,7 @@ set(PATHS collect_sources(SOURCES "${PATHS}" "${EXCLUDE_PATHS}") if (CMAKE_SYSTEM_NAME STREQUAL "QNX") - # --- QDB Wrapper + # --- QDB Wrapper add_subdirectory(./src/qdb_wrapper) else () # --- SQLite Wrapper @@ -109,10 +109,11 @@ else () endif () if(${CMAKE_SYSTEM_NAME} MATCHES "Linux") - list(APPEND LIBRARIES dl pthread ${RTLIB}) + list(APPEND LIBRARIES dl pthread ${RTLIB}) endif() add_library("Utils" ${SOURCES}) +list(APPEND LIBRARIES -lboost_system -lboost_filesystem -lboost_thread) target_link_libraries("Utils" ${LIBRARIES}) if(ENABLE_LOG) diff --git a/src/components/utils/src/conditional_variable_posix.cc b/src/components/utils/src/conditional_variable_posix.cc index 50ebc74556..6b2115ad80 100644 --- a/src/components/utils/src/conditional_variable_posix.cc +++ b/src/components/utils/src/conditional_variable_posix.cc @@ -41,108 +41,98 @@ namespace { const long kNanosecondsPerSecond = 1000000000; const long kMillisecondsPerSecond = 1000; const long kNanosecondsPerMillisecond = 1000000; -} +} // namespace namespace sync_primitives { CREATE_LOGGERPTR_GLOBAL(logger_, "Utils") -ConditionalVariable::ConditionalVariable() { - pthread_condattr_t attrs; - int initialized = pthread_condattr_init(&attrs); - if (initialized != 0) - LOG4CXX_ERROR(logger_, - "Failed to initialize " - "conditional variable attributes"); - pthread_condattr_setclock(&attrs, CLOCK_MONOTONIC); - initialized = pthread_cond_init(&cond_var_, &attrs); - if (initialized != 0) - LOG4CXX_ERROR(logger_, - "Failed to initialize " - "conditional variable"); - int rv = pthread_condattr_destroy(&attrs); - if (rv != 0) - LOG4CXX_ERROR(logger_, - "Failed to destroy " - "conditional variable attributes"); -} +ConditionalVariable::ConditionalVariable() {} -ConditionalVariable::~ConditionalVariable() { - pthread_cond_destroy(&cond_var_); -} +ConditionalVariable::~ConditionalVariable() {} void ConditionalVariable::NotifyOne() { - int signaled = pthread_cond_signal(&cond_var_); - if (signaled != 0) - LOG4CXX_ERROR(logger_, "Failed to signal conditional variable"); + cond_var_.notify_one(); } void ConditionalVariable::Broadcast() { - int signaled = pthread_cond_broadcast(&cond_var_); - if (signaled != 0) - LOG4CXX_ERROR(logger_, "Failed to broadcast conditional variable"); + cond_var_.notify_all(); } -bool ConditionalVariable::Wait(Lock& lock) { - lock.AssertTakenAndMarkFree(); - int wait_status = pthread_cond_wait(&cond_var_, &lock.mutex_); - lock.AssertFreeAndMarkTaken(); - if (wait_status != 0) { - LOG4CXX_ERROR(logger_, "Failed to wait for conditional variable"); +bool ConditionalVariable::Wait(BaseLock& lock) { + // NOTE this grossness is due to boost mutex and recursive mutex not sharing a + // superclass + + try { + lock.AssertTakenAndMarkFree(); + // What kind of lock are we ? + if (Lock* test_lock = dynamic_cast(&lock)) { + // Regular lock + cond_var_.wait(test_lock->mutex_); + } else if (RecursiveLock* test_rec_lock = + dynamic_cast(&lock)) { + // Recursive lock + cond_var_.wait(test_rec_lock->mutex_); + } else { + // unknown + LOG4CXX_ERROR(logger_, "Unknown lock type!"); + return false; + } + lock.AssertFreeAndMarkTaken(); + } catch (std::exception err) { + LOG4CXX_ERROR( + logger_, + "Failed to wait for conditional variable, exception:" << err.what()); return false; } + return true; } bool ConditionalVariable::Wait(AutoLock& auto_lock) { - Lock& lock = auto_lock.GetLock(); - lock.AssertTakenAndMarkFree(); - int wait_status = pthread_cond_wait(&cond_var_, &lock.mutex_); - lock.AssertFreeAndMarkTaken(); - if (wait_status != 0) { - LOG4CXX_ERROR(logger_, "Failed to wait for conditional variable"); - return false; - } - return true; + BaseLock& lock = auto_lock.GetLock(); + return Wait(lock); } ConditionalVariable::WaitStatus ConditionalVariable::WaitFor( AutoLock& auto_lock, uint32_t milliseconds) { - struct timespec now; - clock_gettime(CLOCK_MONOTONIC, &now); - timespec wait_interval; - wait_interval.tv_sec = now.tv_sec + (milliseconds / kMillisecondsPerSecond); - wait_interval.tv_nsec = - now.tv_nsec + - (milliseconds % kMillisecondsPerSecond) * kNanosecondsPerMillisecond; - wait_interval.tv_sec += wait_interval.tv_nsec / kNanosecondsPerSecond; - wait_interval.tv_nsec %= kNanosecondsPerSecond; - Lock& lock = auto_lock.GetLock(); - lock.AssertTakenAndMarkFree(); - int timedwait_status = - pthread_cond_timedwait(&cond_var_, &lock.mutex_, &wait_interval); - lock.AssertFreeAndMarkTaken(); + BaseLock& lock = auto_lock.GetLock(); + WaitStatus wait_status = kNoTimeout; - switch (timedwait_status) { - case 0: { - wait_status = kNoTimeout; - break; - } - case EINTR: { - wait_status = kNoTimeout; - break; + lock.AssertTakenAndMarkFree(); + try { + bool timeout = true; + + // What kind of lock are we ? + if (Lock* test_lock = dynamic_cast(&lock)) { + // Regular lock + // cond_var_.wait(test_lock->mutex_); + timeout = cond_var_.timed_wait( + test_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); + } else if (RecursiveLock* test_rec_lock = + dynamic_cast(&lock)) { + // Recursive lock + // cond_var_.wait(test_rec_lock->mutex_); + timeout = cond_var_.timed_wait( + test_rec_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); + } else { + // unknown + LOG4CXX_ERROR(logger_, "Unknown lock type!"); } - case ETIMEDOUT: { + + if (!timeout) { wait_status = kTimeout; - break; - } - default: { - LOG4CXX_ERROR( - logger_, - "Failed to timewait for conditional variable timedwait_status: " - << timedwait_status); } + } catch (boost::thread_interrupted inter) { + wait_status = kNoTimeout; + + } catch (std::exception err) { + LOG4CXX_ERROR( + logger_, + "Failed to timewait for conditional variable timedwait_status: " + << err.what()); } + return wait_status; } diff --git a/src/components/utils/src/lock_boost.cc b/src/components/utils/src/lock_boost.cc new file mode 100644 index 0000000000..67f8b5d2db --- /dev/null +++ b/src/components/utils/src/lock_boost.cc @@ -0,0 +1,143 @@ +/* + * Copyright (c) 2013, Ford Motor Company + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following + * disclaimer in the documentation and/or other materials provided with the + * distribution. + * + * Neither the name of the Ford Motor Company nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ + +#include +#include +#include +#include +#include +#include "utils/lock.h" +#include "utils/logger.h" + +namespace sync_primitives { + +CREATE_LOGGERPTR_GLOBAL(logger_, "Utils") + +Lock::Lock() {} + +Lock::~Lock() { + if (lock_taken_ > 0) { + LOG4CXX_ERROR(logger_, "Destroying non-released regular mutex " << &mutex_); + } +} + +void Lock::Acquire() { + try { + mutex_.lock(); + } catch (std::exception err) { + LOG4CXX_FATAL(logger_, + "Failed to acquire mutex " << &mutex_ << ": " << err.what()); + NOTREACHED(); + } + AssertFreeAndMarkTaken(); +} + +void Lock::Release() { + AssertTakenAndMarkFree(); + + mutex_.unlock(); +} + +bool Lock::Try() { + bool status = mutex_.try_lock(); + if (status) { + lock_taken_++; + } + return status; +} + +void Lock::AssertFreeAndMarkTaken() { + if (lock_taken_ != 0) { + LOG4CXX_ERROR(logger_, "Locking already taken not recursive mutex"); + NOTREACHED(); + } + + lock_taken_++; +} + +void Lock::AssertTakenAndMarkFree() { + if (lock_taken_ == 0) { + LOG4CXX_ERROR(logger_, "Unlocking a mutex that is not taken"); + NOTREACHED(); + } + lock_taken_--; +} + +// Recursive lock looks the same on the surface, some code duplication is +// necessary since they don't have a shared parent superclass +RecursiveLock::RecursiveLock() {} + +RecursiveLock::~RecursiveLock() { + if (lock_taken_ > 0) { + LOG4CXX_ERROR(logger_, + "Destroying non-released recursive mutex " << &mutex_); + } +} + +void RecursiveLock::Acquire() { + try { + mutex_.lock(); + } catch (std::exception err) { + LOG4CXX_FATAL(logger_, + "Failed to acquire mutex " << &mutex_ << ": " << err.what()); + NOTREACHED(); + } + + AssertFreeAndMarkTaken(); +} // namespace sync_primitives_boost + +void RecursiveLock::Release() { + AssertTakenAndMarkFree(); + mutex_.unlock(); +} + +bool RecursiveLock::Try() { + bool status = mutex_.try_lock(); + if (status) { + lock_taken_++; + } + return status; +} + +void RecursiveLock::AssertFreeAndMarkTaken() { + lock_taken_++; +} + +void RecursiveLock::AssertTakenAndMarkFree() { + if (lock_taken_ == 0) { + LOG4CXX_ERROR(logger_, "Unlocking a recursive mutex that is not taken"); + NOTREACHED(); + } + lock_taken_--; +} + +} // namespace sync_primitives diff --git a/src/components/utils/src/lock_posix.cc b/src/components/utils/src/lock_posix.cc index 9b90ad20b9..0f795bb669 100644 --- a/src/components/utils/src/lock_posix.cc +++ b/src/components/utils/src/lock_posix.cc @@ -30,7 +30,7 @@ * POSSIBILITY OF SUCH DAMAGE. */ -#include "utils/lock.h" +#include "utils/lock_posix.h" #include #include #include @@ -38,7 +38,7 @@ #include #include "utils/logger.h" -namespace sync_primitives { +namespace sync_primitives_posix { CREATE_LOGGERPTR_GLOBAL(logger_, "Utils") diff --git a/src/components/utils/test/lock_boost_test.cc b/src/components/utils/test/lock_boost_test.cc new file mode 100644 index 0000000000..0da735a990 --- /dev/null +++ b/src/components/utils/test/lock_boost_test.cc @@ -0,0 +1,103 @@ +/* + * Copyright (c) 2015, Ford Motor Company + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following + * disclaimer in the documentation and/or other materials provided with the + * distribution. + * + * Neither the name of the Ford Motor Company nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ + +#include "gtest/gtest.h" +#include "utils/lock.h" + +namespace test { +namespace components { +namespace utils_test { + +using sync_primitives::Lock; +using sync_primitives::RecursiveLock; + +TEST(LockBoostTest, TestNonRecursive) { + // Create Lock object + Lock test_mutex; + // Lock mutex + test_mutex.Acquire(); + // Check if created mutex is non-recursive + EXPECT_FALSE(test_mutex.Try()); + // Release mutex before destroy + test_mutex.Release(); +} + +TEST(LockBoostTest, TestRecursive) { + // Create Lock object + RecursiveLock test_mutex; + // Lock mutex + test_mutex.Acquire(); + // Check if created mutex is recursive + EXPECT_TRUE(test_mutex.Try()); + // Release mutex before destroy + test_mutex.Release(); + test_mutex.Release(); +} + +TEST(LockBoostTest, ReleaseMutex_ExpectMutexReleased) { + // Create Lock object (non-recursive mutex) + Lock test_mutex; + // Lock mutex + test_mutex.Acquire(); + // Release mutex + test_mutex.Release(); + // Try to lock it again. If released expect true + EXPECT_TRUE(test_mutex.Try()); + test_mutex.Release(); +} + +TEST(LockBoostTest, TryLockNonRecursiveMutex_ExpectMutexNotLockedTwice) { + // Create Lock object (non-recursive mutex) + Lock test_mutex; + // Lock mutex + test_mutex.Try(); + // Try to lock it again. If locked expect false + EXPECT_FALSE(test_mutex.Try()); + test_mutex.Release(); +} + +TEST(LockBoostTest, TryLockRecursiveMutex_ExpectMutexLockedTwice) { + // Create Lock object (recursive mutex) + RecursiveLock test_mutex; + // Lock mutex + test_mutex.Try(); + // Try to lock it again. Expect true and internal counter increase + EXPECT_TRUE(test_mutex.Try()); + // Release mutex twice as was locked twice. + // Every Release() will decrement internal counter + test_mutex.Release(); + test_mutex.Release(); +} + +} // namespace utils_test +} // namespace components +} // namespace test diff --git a/src/components/utils/test/lock_posix_test.cc b/src/components/utils/test/lock_posix_test.cc index a78659ab31..04f2f5fa34 100644 --- a/src/components/utils/test/lock_posix_test.cc +++ b/src/components/utils/test/lock_posix_test.cc @@ -31,13 +31,13 @@ */ #include "gtest/gtest.h" -#include "utils/lock.h" +#include "utils/lock_posix.h" namespace test { namespace components { namespace utils_test { -using sync_primitives::Lock; +using sync_primitives_posix::Lock; TEST(LockPosixTest, DefaultCtorTest_ExpectNonRecursiveMutexCreated) { // Create Lock object -- cgit v1.2.1 From 09c941dd2ffd4f98ba706895a67133318c3c5007 Mon Sep 17 00:00:00 2001 From: Conlain Kelly Date: Tue, 5 Jun 2018 11:07:54 -0400 Subject: dump current implementation, currently stuck on bigger SDL lock scoping bug --- .../test/hmi_language_handler_test.cc | 2 +- .../test/message_helper/message_helper_test.cc | 60 +++++++++++++------- src/components/include/utils/data_accessor.h | 2 + src/components/include/utils/lock.h | 28 +++++---- .../utils/src/conditional_variable_posix.cc | 55 ++++++++++-------- src/components/utils/src/lock_boost.cc | 66 ++++++++++++++-------- .../utils/test/conditional_variable_test.cc | 7 ++- src/components/utils/test/message_queue_test.cc | 3 + 8 files changed, 140 insertions(+), 83 deletions(-) (limited to 'src/components') diff --git a/src/components/application_manager/test/hmi_language_handler_test.cc b/src/components/application_manager/test/hmi_language_handler_test.cc index 2020da5052..c0138710d4 100644 --- a/src/components/application_manager/test/hmi_language_handler_test.cc +++ b/src/components/application_manager/test/hmi_language_handler_test.cc @@ -129,11 +129,11 @@ class HmiLanguageHandlerTest : public ::testing::Test { } } + ::sync_primitives::Lock app_set_lock_; MockApplicationManager app_manager_; MockHMICapabilities hmi_capabilities_; MockEventDispatcher event_dispatcher_; SharedPtr hmi_language_handler_; - ::sync_primitives::Lock app_set_lock_; resumption_test::MockLastState last_state_; }; diff --git a/src/components/application_manager/test/message_helper/message_helper_test.cc b/src/components/application_manager/test/message_helper/message_helper_test.cc index dc4b3f48bf..aef80866b6 100644 --- a/src/components/application_manager/test/message_helper/message_helper_test.cc +++ b/src/components/application_manager/test/message_helper/message_helper_test.cc @@ -33,18 +33,17 @@ #include #include -#include "gmock/gmock.h" -#include "utils/macro.h" -#include "utils/make_shared.h" -#include "application_manager/policies/policy_handler.h" +#include "application_manager/event_engine/event_dispatcher.h" #include "application_manager/mock_application.h" -#include "utils/custom_string.h" -#include "policy/mock_policy_settings.h" -#include "application_manager/policies/policy_handler.h" #include "application_manager/mock_application_manager.h" -#include "application_manager/event_engine/event_dispatcher.h" -#include "application_manager/state_controller.h" +#include "application_manager/policies/policy_handler.h" #include "application_manager/resumption/resume_ctrl.h" +#include "application_manager/state_controller.h" +#include "gmock/gmock.h" +#include "policy/mock_policy_settings.h" +#include "utils/custom_string.h" +#include "utils/macro.h" +#include "utils/make_shared.h" #ifdef EXTERNAL_PROPRIETARY_MODE #include "policy/policy_external/include/policy/policy_types.h" @@ -64,12 +63,12 @@ typedef utils::SharedPtr MockApplicationSharedPtr; typedef std::vector StringArray; typedef utils::SharedPtr ApplicationSharedPtr; +using testing::_; using testing::AtLeast; -using testing::ReturnRefOfCopy; -using testing::ReturnRef; using testing::Return; +using testing::ReturnRef; +using testing::ReturnRefOfCopy; using testing::SaveArg; -using testing::_; TEST(MessageHelperTestCreate, CreateBlockedByPoliciesResponse_SmartObject_Equal) { @@ -234,9 +233,13 @@ TEST(MessageHelperTestCreate, CreateShowRequestToHMI_SendSmartObject_Equal) { TEST(MessageHelperTestCreate, CreateAddCommandRequestToHMI_SendSmartObject_Empty) { + sync_primitives::RecursiveLock access_lock; + MockApplicationSharedPtr appSharedMock = utils::MakeShared(); ::application_manager::CommandsMap vis; - DataAccessor data_accessor(vis, sync_primitives::RecursiveLock()); + + DataAccessor data_accessor(vis, + access_lock); EXPECT_CALL(*appSharedMock, commands_map()).WillOnce(Return(data_accessor)); application_manager_test::MockApplicationManager mock_application_manager; @@ -249,9 +252,11 @@ TEST(MessageHelperTestCreate, TEST(MessageHelperTestCreate, CreateAddCommandRequestToHMI_SendSmartObject_Equal) { + sync_primitives::RecursiveLock access_lock; + MockApplicationSharedPtr appSharedMock = utils::MakeShared(); CommandsMap vis; - DataAccessor data_accessor(vis, sync_primitives::RecursiveLock()); + DataAccessor data_accessor(vis, access_lock); smart_objects::SmartObjectSPtr smartObjectPtr = utils::MakeShared(); @@ -290,9 +295,12 @@ TEST(MessageHelperTestCreate, TEST(MessageHelperTestCreate, CreateAddVRCommandRequestFromChoiceToHMI_SendEmptyData_EmptyList) { + sync_primitives::RecursiveLock access_lock; MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::ChoiceSetMap vis; - DataAccessor< ::application_manager::ChoiceSetMap> data_accessor(vis, sync_primitives::RecursiveLock()); + + DataAccessor< ::application_manager::ChoiceSetMap> data_accessor(vis, + access_lock); EXPECT_CALL(*appSharedMock, choice_set_map()).WillOnce(Return(data_accessor)); application_manager_test::MockApplicationManager mock_application_manager; @@ -305,9 +313,12 @@ TEST(MessageHelperTestCreate, TEST(MessageHelperTestCreate, CreateAddVRCommandRequestFromChoiceToHMI_SendObject_EqualList) { + sync_primitives::RecursiveLock access_lock; MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::ChoiceSetMap vis; - DataAccessor< ::application_manager::ChoiceSetMap> data_accessor(vis, sync_primitives::RecursiveLock()); + + DataAccessor< ::application_manager::ChoiceSetMap> data_accessor(vis, + access_lock); smart_objects::SmartObjectSPtr smartObjectPtr = utils::MakeShared(); @@ -351,9 +362,12 @@ TEST(MessageHelperTestCreate, } TEST(MessageHelperTestCreate, CreateAddSubMenuRequestToHMI_SendObject_Equal) { + sync_primitives::RecursiveLock access_lock; MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::SubMenuMap vis; - DataAccessor< ::application_manager::SubMenuMap> data_accessor(vis, sync_primitives::RecursiveLock()); + + DataAccessor< ::application_manager::SubMenuMap> data_accessor(vis, + access_lock); smart_objects::SmartObjectSPtr smartObjectPtr = utils::MakeShared(); @@ -390,9 +404,12 @@ TEST(MessageHelperTestCreate, CreateAddSubMenuRequestToHMI_SendObject_Equal) { TEST(MessageHelperTestCreate, CreateAddSubMenuRequestToHMI_SendEmptyMap_EmptySmartObjectList) { + sync_primitives::RecursiveLock access_lock; MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::SubMenuMap vis; - DataAccessor< ::application_manager::SubMenuMap> data_accessor(vis, sync_primitives::RecursiveLock()); + + DataAccessor< ::application_manager::SubMenuMap> data_accessor(vis, + access_lock); EXPECT_CALL(*appSharedMock, sub_menu_map()).WillOnce(Return(data_accessor)); @@ -708,12 +725,14 @@ TEST_F(MessageHelperTest, VerifySoftButtonString_CorrectStrings_True) { TEST_F(MessageHelperTest, GetIVISubscriptionRequests_ValidApplication_HmiRequestNotEmpty) { + sync_primitives::RecursiveLock access_lock; // Creating sharedPtr to MockApplication MockApplicationSharedPtr appSharedMock = utils::MakeShared(); // Creating data acessor application_manager::VehicleInfoSubscriptions vis; + DataAccessor data_accessor( - vis, sync_primitives::RecursiveLock()); + vis, access_lock); // Calls for ApplicationManager EXPECT_CALL(*appSharedMock, app_id()).WillOnce(Return(1u)); EXPECT_CALL(*appSharedMock, SubscribedIVI()).WillOnce(Return(data_accessor)); @@ -937,7 +956,8 @@ TEST_F(MessageHelperTest, SubscribeApplicationToSoftButton_CallFromApp) { size_t function_id = 1; // EXPECT_CALL(*appSharedPtr, - SubscribeToSoftButtons(function_id, SoftButtonID())).Times(1); + SubscribeToSoftButtons(function_id, SoftButtonID())) + .Times(1); MessageHelper::SubscribeApplicationToSoftButton( message_params, appSharedPtr, function_id); } diff --git a/src/components/include/utils/data_accessor.h b/src/components/include/utils/data_accessor.h index 6239812678..7ad2252a65 100644 --- a/src/components/include/utils/data_accessor.h +++ b/src/components/include/utils/data_accessor.h @@ -52,6 +52,8 @@ class DataAccessor { } ~DataAccessor() { + // std::cerr << "destructing accessor, counter is " << *counter_ << "for lock " + // << &lock_ << "\n"; if (0 == *counter_) { lock_.Release(); } else { diff --git a/src/components/include/utils/lock.h b/src/components/include/utils/lock.h index 4ace34d71e..d167f5bb54 100644 --- a/src/components/include/utils/lock.h +++ b/src/components/include/utils/lock.h @@ -35,10 +35,10 @@ #include #include #include +#include #include "utils/atomic.h" #include "utils/macro.h" #include "utils/memory_barrier.h" -#include using boost::system::error_code; @@ -76,7 +76,8 @@ class SpinMutex { */ class BaseLock { public: - BaseLock() : lock_taken_(0) {} + BaseLock() {} + virtual ~BaseLock() {} // Ackquire the lock. Must be called only once on a thread. // Please consider using AutoLock to capture it. virtual void Acquire() = 0; @@ -91,18 +92,11 @@ class BaseLock { virtual bool Try() = 0; protected: - /** - * @brief Basic debugging aid, a flag that signals wether this lock is - * currently taken - * Allows detection of abandoned and recursively captured mutexes - */ - uint32_t lock_taken_; - // Ensures safety in locking virtual void AssertTakenAndMarkFree() = 0; virtual void AssertFreeAndMarkTaken() = 0; -friend class ConditionalVariable; + friend class ConditionalVariable; }; /* @@ -120,6 +114,12 @@ class Lock : public BaseLock { virtual bool Try(); private: + /** + * @brief Basic debugging aid, a flag that signals wether this lock is + * currently taken + * Allows detection of abandoned and recursively captured mutexes + */ + uint32_t lock_taken_; virtual void AssertTakenAndMarkFree(); virtual void AssertFreeAndMarkTaken(); boost::mutex mutex_; @@ -142,6 +142,12 @@ class RecursiveLock : public BaseLock { virtual bool Try(); private: + /** + * @brief Basic debugging aid, a flag that signals wether this lock is + * currently taken + * Allows detection of abandoned and recursively captured mutexes + */ + uint32_t lock_taken_; virtual void AssertTakenAndMarkFree(); virtual void AssertFreeAndMarkTaken(); boost::recursive_mutex mutex_; @@ -153,7 +159,7 @@ class RecursiveLock : public BaseLock { class AutoLock { public: explicit AutoLock(BaseLock& lock) : lock_(lock) { - std::cerr << "lock is at " << &lock << std::endl; + // std::cerr << "lock is at " << &lock << std::endl; lock_.Acquire(); } ~AutoLock() { diff --git a/src/components/utils/src/conditional_variable_posix.cc b/src/components/utils/src/conditional_variable_posix.cc index 6b2115ad80..109e2bac9b 100644 --- a/src/components/utils/src/conditional_variable_posix.cc +++ b/src/components/utils/src/conditional_variable_posix.cc @@ -63,28 +63,32 @@ bool ConditionalVariable::Wait(BaseLock& lock) { // NOTE this grossness is due to boost mutex and recursive mutex not sharing a // superclass - try { - lock.AssertTakenAndMarkFree(); - // What kind of lock are we ? - if (Lock* test_lock = dynamic_cast(&lock)) { - // Regular lock - cond_var_.wait(test_lock->mutex_); - } else if (RecursiveLock* test_rec_lock = - dynamic_cast(&lock)) { - // Recursive lock - cond_var_.wait(test_rec_lock->mutex_); - } else { - // unknown - LOG4CXX_ERROR(logger_, "Unknown lock type!"); - return false; - } + lock.AssertTakenAndMarkFree(); + // try { + // What kind of lock are we ? + if (Lock* test_lock = dynamic_cast(&lock)) { + // Regular lock + cond_var_.wait(test_lock->mutex_); + } else if (RecursiveLock* test_rec_lock = + dynamic_cast(&lock)) { + // Recursive lock + cond_var_.wait(test_rec_lock->mutex_); + } else { + // unknown lock.AssertFreeAndMarkTaken(); - } catch (std::exception err) { - LOG4CXX_ERROR( - logger_, - "Failed to wait for conditional variable, exception:" << err.what()); + + LOG4CXX_ERROR(logger_, "Unknown lock type!"); return false; } + // } catch (std::exception err) { + // lock.AssertFreeAndMarkTaken(); + // + // LOG4CXX_ERROR( + // logger_, + // "Failed to wait for conditional variable, exception:" << err.what()); + // return false; + // } + lock.AssertFreeAndMarkTaken(); return true; } @@ -125,13 +129,14 @@ ConditionalVariable::WaitStatus ConditionalVariable::WaitFor( } } catch (boost::thread_interrupted inter) { wait_status = kNoTimeout; - - } catch (std::exception err) { - LOG4CXX_ERROR( - logger_, - "Failed to timewait for conditional variable timedwait_status: " - << err.what()); } + // catch (std::exception err) { + // LOG4CXX_ERROR( + // logger_, + // "Failed to timewait for conditional variable timedwait_status: " + // << err.what()); + // } + lock.AssertFreeAndMarkTaken(); return wait_status; } diff --git a/src/components/utils/src/lock_boost.cc b/src/components/utils/src/lock_boost.cc index 67f8b5d2db..e3c4a8cb4f 100644 --- a/src/components/utils/src/lock_boost.cc +++ b/src/components/utils/src/lock_boost.cc @@ -42,26 +42,33 @@ namespace sync_primitives { CREATE_LOGGERPTR_GLOBAL(logger_, "Utils") -Lock::Lock() {} +Lock::Lock() : lock_taken_(0) {} Lock::~Lock() { + // std::cerr << "destructing lock " << &mutex_ << " with lock count " + // << lock_taken_ << '\n'; if (lock_taken_ > 0) { - LOG4CXX_ERROR(logger_, "Destroying non-released regular mutex " << &mutex_); + LOG4CXX_FATAL(logger_, "Destroying non-released regular mutex " << &mutex_); } } void Lock::Acquire() { - try { - mutex_.lock(); - } catch (std::exception err) { - LOG4CXX_FATAL(logger_, - "Failed to acquire mutex " << &mutex_ << ": " << err.what()); - NOTREACHED(); - } + // try { + mutex_.lock(); + // std::cerr << "acquiring lock " << &mutex_ << " with lock count " + // << lock_taken_ << '\n'; + // } catch (std::exception err) { + // LOG4CXX_FATAL(logger_, + // "Failed to acquire mutex " << &mutex_ << ": " << + // err.what()); + // NOTREACHED(); + // } AssertFreeAndMarkTaken(); } void Lock::Release() { + // std::cerr << "releasing lock " << &mutex_ << " with lock count " + // << lock_taken_ << '\n'; AssertTakenAndMarkFree(); mutex_.unlock(); @@ -69,15 +76,17 @@ void Lock::Release() { bool Lock::Try() { bool status = mutex_.try_lock(); + // std::cerr << "trying lock " << &mutex_ << " with lock count " << lock_taken_ + // << " status was " << status << '\n'; if (status) { - lock_taken_++; + AssertFreeAndMarkTaken(); } return status; } void Lock::AssertFreeAndMarkTaken() { if (lock_taken_ != 0) { - LOG4CXX_ERROR(logger_, "Locking already taken not recursive mutex"); + LOG4CXX_FATAL(logger_, "Locking already taken not recursive mutex"); NOTREACHED(); } @@ -86,7 +95,7 @@ void Lock::AssertFreeAndMarkTaken() { void Lock::AssertTakenAndMarkFree() { if (lock_taken_ == 0) { - LOG4CXX_ERROR(logger_, "Unlocking a mutex that is not taken"); + LOG4CXX_FATAL(logger_, "Unlocking a mutex that is not taken"); NOTREACHED(); } lock_taken_--; @@ -94,36 +103,45 @@ void Lock::AssertTakenAndMarkFree() { // Recursive lock looks the same on the surface, some code duplication is // necessary since they don't have a shared parent superclass -RecursiveLock::RecursiveLock() {} +RecursiveLock::RecursiveLock() : lock_taken_(0) {} RecursiveLock::~RecursiveLock() { + // std::cerr << "destructing recursive lock " << &mutex_ << " with lock count " + // << lock_taken_ << '\n'; if (lock_taken_ > 0) { - LOG4CXX_ERROR(logger_, + LOG4CXX_FATAL(logger_, "Destroying non-released recursive mutex " << &mutex_); } } void RecursiveLock::Acquire() { - try { - mutex_.lock(); - } catch (std::exception err) { - LOG4CXX_FATAL(logger_, - "Failed to acquire mutex " << &mutex_ << ": " << err.what()); - NOTREACHED(); - } + // try { + mutex_.lock(); + // std::cerr << "acquiring recursive lock " << &mutex_ << " with lock count " + // << lock_taken_ << '\n'; + // } catch (std::exception err) { + // LOG4CXX_FATAL(logger_, + // "Failed to acquire mutex " << &mutex_ << ": " << + // err.what()); + // NOTREACHED(); + // } AssertFreeAndMarkTaken(); -} // namespace sync_primitives_boost +} void RecursiveLock::Release() { + std::cerr << "releasing recursive lock " << &mutex_ << " with lock count " + << lock_taken_ << '\n'; AssertTakenAndMarkFree(); mutex_.unlock(); } bool RecursiveLock::Try() { bool status = mutex_.try_lock(); + // std::cerr << "trying recursive lock " << &mutex_ << " with lock count " + // << lock_taken_ << " status was " << status << '\n'; if (status) { - lock_taken_++; + AssertFreeAndMarkTaken(); } return status; } @@ -134,7 +152,7 @@ void RecursiveLock::AssertFreeAndMarkTaken() { void RecursiveLock::AssertTakenAndMarkFree() { if (lock_taken_ == 0) { - LOG4CXX_ERROR(logger_, "Unlocking a recursive mutex that is not taken"); + LOG4CXX_FATAL(logger_, "Unlocking a recursive mutex that is not taken"); NOTREACHED(); } lock_taken_--; diff --git a/src/components/utils/test/conditional_variable_test.cc b/src/components/utils/test/conditional_variable_test.cc index 524d53cafa..290ff3434f 100644 --- a/src/components/utils/test/conditional_variable_test.cc +++ b/src/components/utils/test/conditional_variable_test.cc @@ -30,14 +30,14 @@ * POSSIBILITY OF SUCH DAMAGE. */ -#include #include +#include #include "gtest/gtest.h" +#include "utils/conditional_variable.h" #include "utils/lock.h" #include "utils/macro.h" -#include "utils/conditional_variable.h" namespace test { namespace components { @@ -102,6 +102,7 @@ TEST_F(ConditionalVariableTest, cond_var_.WaitFor(test_lock, 2000); std::string last_value("changed again by thread 1"); EXPECT_EQ(last_value, test_value_); + pthread_join(thread1, NULL); } TEST_F(ConditionalVariableTest, @@ -116,6 +117,8 @@ TEST_F(ConditionalVariableTest, ASSERT_FALSE(thread_created) << "thread2 is not created!"; check_counter(); EXPECT_EQ(2u, counter_); + pthread_join(thread1, NULL); + pthread_join(thread2, NULL); } TEST_F( diff --git a/src/components/utils/test/message_queue_test.cc b/src/components/utils/test/message_queue_test.cc index ddc1aa1d81..9aa70933d6 100644 --- a/src/components/utils/test/message_queue_test.cc +++ b/src/components/utils/test/message_queue_test.cc @@ -158,6 +158,8 @@ TEST_F(MessageQueueTest, ASSERT_EQ(test_val_1, test_line); // Check the size of queue after 1 element was removed ASSERT_EQ(0u, test_queue.size()); + pthread_join(thread1, NULL); + pthread_join(thread2, NULL); } TEST_F(MessageQueueTest, @@ -169,6 +171,7 @@ TEST_F(MessageQueueTest, test_queue.wait(); check_value = true; ASSERT_TRUE(check_value); + pthread_join(thread1, NULL); } } // namespace utils_test -- cgit v1.2.1 From 24d8f16ce9c4ba5f2c09f68fff66e0f46383b6c1 Mon Sep 17 00:00:00 2001 From: Conlain Kelly Date: Wed, 18 Jul 2018 13:44:53 -0400 Subject: Cleanup, add some cmake support --- .../connection_handler/connection_handler_impl.h | 2 +- .../connection_handler/src/connection.cc | 2 +- .../src/connection_handler_impl.cc | 2 +- src/components/include/utils/lock.h | 3 + src/components/include/utils/lock_posix.h | 184 --------------------- src/components/utils/CMakeLists.txt | 4 +- src/components/utils/src/lock_posix.cc | 149 ----------------- src/components/utils/test/lock_posix_test.cc | 123 -------------- 8 files changed, 9 insertions(+), 460 deletions(-) delete mode 100644 src/components/include/utils/lock_posix.h delete mode 100644 src/components/utils/src/lock_posix.cc delete mode 100644 src/components/utils/test/lock_posix_test.cc (limited to 'src/components') diff --git a/src/components/connection_handler/include/connection_handler/connection_handler_impl.h b/src/components/connection_handler/include/connection_handler/connection_handler_impl.h index 121d5ed08c..730d7ebe32 100644 --- a/src/components/connection_handler/include/connection_handler/connection_handler_impl.h +++ b/src/components/connection_handler/include/connection_handler/connection_handler_impl.h @@ -640,7 +640,7 @@ class ConnectionHandlerImpl * @brief session/connection map */ SessionConnectionMap session_connection_map_; - mutable std::shared_ptr + mutable std::shared_ptr session_connection_map_lock_ptr_; /** diff --git a/src/components/connection_handler/src/connection.cc b/src/components/connection_handler/src/connection.cc index b8399b2c9b..08b37b9026 100644 --- a/src/components/connection_handler/src/connection.cc +++ b/src/components/connection_handler/src/connection.cc @@ -81,7 +81,7 @@ Connection::Connection(ConnectionHandle connection_handle, : connection_handler_(connection_handler) , connection_handle_(connection_handle) , connection_device_handle_(connection_device_handle) - , session_map_lock_(true) + , session_map_lock_() , primary_connection_handle_(0) , heartbeat_timeout_(heartbeat_timeout) { LOG4CXX_AUTO_TRACE(logger_); diff --git a/src/components/connection_handler/src/connection_handler_impl.cc b/src/components/connection_handler/src/connection_handler_impl.cc index 270b38d724..9aa84989d9 100644 --- a/src/components/connection_handler/src/connection_handler_impl.cc +++ b/src/components/connection_handler/src/connection_handler_impl.cc @@ -69,7 +69,7 @@ ConnectionHandlerImpl::ConnectionHandlerImpl( , transport_manager_(tm) , protocol_handler_(NULL) , session_connection_map_lock_ptr_( - std::make_shared(true)) + std::make_shared()) , connection_list_lock_() , connection_handler_observer_lock_() , connection_list_deleter_(&connection_list_) diff --git a/src/components/include/utils/lock.h b/src/components/include/utils/lock.h index 1b27a4e915..bfa1ef1770 100644 --- a/src/components/include/utils/lock.h +++ b/src/components/include/utils/lock.h @@ -159,6 +159,9 @@ class RecursiveLock : public BaseLock { // This class is used to automatically acquire and release the a lock class AutoLock { public: + explicit AutoLock(const std::shared_ptr& lock) : lock_(*lock) { + lock_.Acquire(); + } explicit AutoLock(BaseLock& lock) : lock_(lock) { // std::cerr << "lock is at " << &lock << std::endl; lock_.Acquire(); diff --git a/src/components/include/utils/lock_posix.h b/src/components/include/utils/lock_posix.h deleted file mode 100644 index ae8b64959f..0000000000 --- a/src/components/include/utils/lock_posix.h +++ /dev/null @@ -1,184 +0,0 @@ -/* - * Copyright (c) 2013, Ford Motor Company - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following - * disclaimer in the documentation and/or other materials provided with the - * distribution. - * - * Neither the name of the Ford Motor Company nor the names of its contributors - * may be used to endorse or promote products derived from this software - * without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" - * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE - * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR - * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF - * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS - * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN - * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) - * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE - * POSSIBILITY OF SUCH DAMAGE. - */ -#ifndef SRC_COMPONENTS_INCLUDE_UTILS_LOCK_H_ -#define SRC_COMPONENTS_INCLUDE_UTILS_LOCK_H_ - -#if defined(OS_POSIX) -#include -#include -#else -#error Please implement lock for your OS -#endif -#include -#include "utils/macro.h" -#include "utils/atomic.h" -#include "utils/memory_barrier.h" - -namespace sync_primitives_posix { - -namespace impl { -#if defined(OS_POSIX) -typedef pthread_mutex_t PlatformMutex; -#endif -} // namespace impl - -class SpinMutex { - public: - SpinMutex() : state_(0) {} - void Lock() { - // Comment below add exception for lint error - // Reason: FlexeLint doesn't know about compiler's built-in instructions - /*lint -e1055*/ - if (atomic_post_set(&state_) == 0) { - return; - } - for (;;) { - sched_yield(); - /*lint -e1055*/ - if (state_ == 0 && atomic_post_set(&state_) == 0) { - return; - } - } - } - void Unlock() { - state_ = 0; - } - ~SpinMutex() {} - - private: - volatile unsigned int state_; -}; - -/* Platform-indepenednt NON-RECURSIVE lock (mutex) wrapper - Please use AutoLock to ackquire and (automatically) release it - It eases balancing of multple lock taking/releasing and makes it - Impossible to forget to release the lock: - ... - ConcurentlyAccessedData data_; - sync_primitives::Lock data_lock_; - ... - { - sync_primitives::AutoLock auto_lock(data_lock_); - data_.ReadOrWriteData(); - } // lock is automatically released here -*/ -class Lock { - public: - Lock(); - Lock(bool is_recursive); - ~Lock(); - - // Ackquire the lock. Must be called only once on a thread. - // Please consider using AutoLock to capture it. - void Acquire(); - // Release the lock. Must be called only once on a thread after lock. - // was acquired. Please consider using AutoLock to automatically release - // the lock - void Release(); - // Try if lock can be captured and lock it if it was possible. - // If it captured, lock must be manually released calling to Release - // when protected resource access was finished. - // @returns wether lock was captured. - bool Try(); - - private: - impl::PlatformMutex mutex_; - -#ifndef NDEBUG - /** - * @brief Basic debugging aid, a flag that signals wether this lock is - * currently taken - * Allows detection of abandoned and recursively captured mutexes - */ - uint32_t lock_taken_; - - /** - * @brief Describe if mutex is recurcive or not - */ - bool is_mutex_recursive_; - - void AssertFreeAndMarkTaken(); - void AssertTakenAndMarkFree(); -#else - void AssertFreeAndMarkTaken() {} - void AssertTakenAndMarkFree() {} -#endif - - void Init(bool is_recursive); - - friend class ConditionalVariable; - DISALLOW_COPY_AND_ASSIGN(Lock); -}; - -// This class is used to automatically acquire and release the a lock -class AutoLock { - public: - explicit AutoLock(Lock& lock) : lock_(lock) { - lock_.Acquire(); - } - ~AutoLock() { - lock_.Release(); - } - - private: - Lock& GetLock() { - return lock_; - } - Lock& lock_; - - private: - friend class AutoUnlock; - friend class ConditionalVariable; - DISALLOW_COPY_AND_ASSIGN(AutoLock); -}; - -// This class is used to temporarly unlock autolocked lock -class AutoUnlock { - public: - explicit AutoUnlock(Lock& lock) : lock_(lock) { - lock_.Release(); - } - explicit AutoUnlock(AutoLock& lock) : lock_(lock.GetLock()) { - lock_.Release(); - } - ~AutoUnlock() { - lock_.Acquire(); - } - - private: - Lock& lock_; - - private: - DISALLOW_COPY_AND_ASSIGN(AutoUnlock); -}; -} // namespace sync_primitives -#endif // SRC_COMPONENTS_INCLUDE_UTILS_LOCK_H_ diff --git a/src/components/utils/CMakeLists.txt b/src/components/utils/CMakeLists.txt index 6449f123a4..0eb64ae8c2 100644 --- a/src/components/utils/CMakeLists.txt +++ b/src/components/utils/CMakeLists.txt @@ -114,8 +114,10 @@ if(${CMAKE_SYSTEM_NAME} MATCHES "Linux") endif() add_library("Utils" ${SOURCES}) -list(APPEND LIBRARIES -lboost_system -lboost_filesystem -lboost_thread) +list(APPEND LIBRARIES -lboost_system -lboost_thread) target_link_libraries("Utils" ${LIBRARIES}) +add_dependencies("Utils" Boost) + if(ENABLE_LOG) add_dependencies("Utils" install-3rd_party_logger) diff --git a/src/components/utils/src/lock_posix.cc b/src/components/utils/src/lock_posix.cc deleted file mode 100644 index 2807de1d0a..0000000000 --- a/src/components/utils/src/lock_posix.cc +++ /dev/null @@ -1,149 +0,0 @@ -/* - * Copyright (c) 2013, Ford Motor Company - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following - * disclaimer in the documentation and/or other materials provided with the - * distribution. - * - * Neither the name of the Ford Motor Company nor the names of its contributors - * may be used to endorse or promote products derived from this software - * without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" - * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE - * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR - * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF - * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS - * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN - * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) - * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE - * POSSIBILITY OF SUCH DAMAGE. - */ - -#include "utils/lock_posix.h" -#include -#include -#include -#include -#include -#include "utils/logger.h" - -namespace sync_primitives_posix { - -CREATE_LOGGERPTR_GLOBAL(logger_, "Utils") - -Lock::Lock() -#ifndef NDEBUG - : lock_taken_(0) - , is_mutex_recursive_(false) -#endif // NDEBUG -{ - Init(false); -} - -Lock::Lock(bool is_recursive) -#ifndef NDEBUG - : lock_taken_(0) - , is_mutex_recursive_(is_recursive) -#endif // NDEBUG -{ - Init(is_recursive); -} - -Lock::~Lock() { -#ifndef NDEBUG - if (lock_taken_ > 0) { - LOG4CXX_FATAL(logger_, "Destroying non-released mutex " << &mutex_); - NOTREACHED(); - } -#endif - int32_t status = pthread_mutex_destroy(&mutex_); - if (status != 0) { - LOG4CXX_FATAL(logger_, - "Failed to destroy mutex " << &mutex_ << ": " - << strerror(status)); - NOTREACHED(); - } -} - -void Lock::Acquire() { - const int32_t status = pthread_mutex_lock(&mutex_); - if (status != 0) { - LOG4CXX_FATAL(logger_, - "Failed to acquire mutex " << &mutex_ << ": " - << strerror(status)); - NOTREACHED(); - } else { - AssertFreeAndMarkTaken(); - } -} - -void Lock::Release() { - AssertTakenAndMarkFree(); - const int32_t status = pthread_mutex_unlock(&mutex_); - if (status != 0) { - LOG4CXX_FATAL(logger_, - "Failed to unlock mutex" << &mutex_ << ": " - << strerror(status)); - NOTREACHED(); - } -} - -bool Lock::Try() { - const int32_t status = pthread_mutex_trylock(&mutex_); - if (status == 0) { -#ifndef NDEBUG - lock_taken_++; -#endif - return true; - } - return false; -} - -#ifndef NDEBUG -void Lock::AssertFreeAndMarkTaken() { - if ((lock_taken_ > 0) && !is_mutex_recursive_) { - LOG4CXX_FATAL(logger_, "Locking already taken not recursive mutex"); - NOTREACHED(); - } - lock_taken_++; -} -void Lock::AssertTakenAndMarkFree() { - if (lock_taken_ == 0) { - LOG4CXX_FATAL(logger_, "Unlocking a mutex that is not taken"); - NOTREACHED(); - } - lock_taken_--; -} -#endif - -void Lock::Init(bool is_recursive) { - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - - const int32_t mutex_type = - is_recursive ? PTHREAD_MUTEX_RECURSIVE : PTHREAD_MUTEX_ERRORCHECK; - - pthread_mutexattr_settype(&attr, mutex_type); - const int32_t status = pthread_mutex_init(&mutex_, &attr); - - pthread_mutexattr_destroy(&attr); - - if (status != 0) { - LOG4CXX_FATAL(logger_, - "Failed to initialize mutex. " << std::strerror(status)); - DCHECK(status != 0); - } -} - -} // namespace sync_primitives diff --git a/src/components/utils/test/lock_posix_test.cc b/src/components/utils/test/lock_posix_test.cc deleted file mode 100644 index 04f2f5fa34..0000000000 --- a/src/components/utils/test/lock_posix_test.cc +++ /dev/null @@ -1,123 +0,0 @@ -/* - * Copyright (c) 2015, Ford Motor Company - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following - * disclaimer in the documentation and/or other materials provided with the - * distribution. - * - * Neither the name of the Ford Motor Company nor the names of its contributors - * may be used to endorse or promote products derived from this software - * without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" - * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE - * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR - * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF - * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS - * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN - * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) - * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE - * POSSIBILITY OF SUCH DAMAGE. - */ - -#include "gtest/gtest.h" -#include "utils/lock_posix.h" - -namespace test { -namespace components { -namespace utils_test { - -using sync_primitives_posix::Lock; - -TEST(LockPosixTest, DefaultCtorTest_ExpectNonRecursiveMutexCreated) { - // Create Lock object - Lock test_mutex; - // Lock mutex - test_mutex.Acquire(); - // Check if created mutex is non-recursive - EXPECT_FALSE(test_mutex.Try()); - // Release mutex before destroy - test_mutex.Release(); -} - -TEST(LockPosixTest, CtorTestWithFalseArgument_ExpectNonRecursiveMutexCreated) { - // Create Lock object - Lock test_mutex(false); - // Lock mutex - test_mutex.Acquire(); - // Check if created mutex is non-recursive - EXPECT_FALSE(test_mutex.Try()); - // Release mutex before destroy - test_mutex.Release(); -} - -TEST(LockPosixTest, CtorTestWithTrueArgument_ExpectRecursiveMutexCreated) { - // Create Lock object - Lock test_mutex(true); - // Lock mutex - test_mutex.Acquire(); - // Check if created mutex is recursive - EXPECT_TRUE(test_mutex.Try()); - // Release mutex before destroy - test_mutex.Release(); - test_mutex.Release(); -} - -TEST(LockPosixTest, AcquireMutex_ExpectMutexLocked) { - // Create Lock object (non-recursive mutex) - Lock test_mutex; - // Lock mutex - test_mutex.Acquire(); - // Try to lock it again. If locked expect false - EXPECT_FALSE(test_mutex.Try()); - test_mutex.Release(); -} - -TEST(LockPosixTest, ReleaseMutex_ExpectMutexReleased) { - // Create Lock object (non-recursive mutex) - Lock test_mutex; - // Lock mutex - test_mutex.Acquire(); - // Release mutex - test_mutex.Release(); - // Try to lock it again. If released expect true - EXPECT_TRUE(test_mutex.Try()); - test_mutex.Release(); -} - -TEST(LockPosixTest, TryLockNonRecursiveMutex_ExpectMutexNotLockedTwice) { - // Create Lock object (non-recursive mutex) - Lock test_mutex; - // Lock mutex - test_mutex.Try(); - // Try to lock it again. If locked expect false - EXPECT_FALSE(test_mutex.Try()); - test_mutex.Release(); -} - -TEST(LockPosixTest, TryLockRecursiveMutex_ExpectMutexLockedTwice) { - // Create Lock object (recursive mutex) - Lock test_mutex(true); - // Lock mutex - test_mutex.Try(); - // Try to lock it again. Expect true and internal counter increase - EXPECT_TRUE(test_mutex.Try()); - // Release mutex twice as was locked twice. - // Every Release() will decrement internal counter - test_mutex.Release(); - test_mutex.Release(); -} - -} // namespace utils_test -} // namespace components -} // namespace test -- cgit v1.2.1 From f77b9f7348fb823253912e07d04bca0bba936e05 Mon Sep 17 00:00:00 2001 From: Conlain Kelly Date: Wed, 18 Jul 2018 14:09:53 -0400 Subject: ayyy it builds --- .../include/application_manager/application_data_impl.h | 4 ++-- .../application_manager/src/application_manager_impl.cc | 2 +- .../test/message_helper/message_helper_test.cc | 8 ++++---- src/components/policy/policy_regular/src/cache_manager.cc | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) (limited to 'src/components') diff --git a/src/components/application_manager/include/application_manager/application_data_impl.h b/src/components/application_manager/include/application_manager/application_data_impl.h index fbf6d87d0e..3f60040db9 100644 --- a/src/components/application_manager/include/application_manager/application_data_impl.h +++ b/src/components/application_manager/include/application_manager/application_data_impl.h @@ -279,9 +279,9 @@ class DynamicApplicationDataImpl : public virtual Application { SubMenuMap sub_menu_; mutable std::shared_ptr sub_menu_lock_ptr_; ChoiceSetMap choice_set_map_; - mutable std::shared_ptr choice_set_map_lock_ptr_; + mutable std::shared_ptr choice_set_map_lock_ptr_; PerformChoiceSetMap performinteraction_choice_set_map_; - mutable std::shared_ptr + mutable std::shared_ptr performinteraction_choice_set_lock_ptr_; uint32_t is_perform_interaction_active_; bool is_reset_global_properties_active_; diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index b91072598b..2c9f5d2f6f 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -165,7 +165,7 @@ ApplicationManagerImpl::ApplicationManagerImpl( , resume_ctrl_(new resumption::ResumeCtrlImpl(*this)) , navi_close_app_timeout_(am_settings.stop_streaming_timeout()) , navi_end_stream_timeout_(am_settings.stop_streaming_timeout()) - , stopping_application_mng_lock_(true) + , stopping_application_mng_lock_() , state_ctrl_(*this) , application_list_update_timer_( "AM ListUpdater", diff --git a/src/components/application_manager/test/message_helper/message_helper_test.cc b/src/components/application_manager/test/message_helper/message_helper_test.cc index b1871a6107..e7b7b7337f 100644 --- a/src/components/application_manager/test/message_helper/message_helper_test.cc +++ b/src/components/application_manager/test/message_helper/message_helper_test.cc @@ -312,7 +312,7 @@ TEST(MessageHelperTestCreate, MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::ChoiceSetMap vis; DataAccessor< ::application_manager::ChoiceSetMap> data_accessor( - vis, std::make_shared(true)); + vis, std::make_shared()); EXPECT_CALL(*appSharedMock, choice_set_map()).WillOnce(Return(data_accessor)); application_manager_test::MockApplicationManager mock_application_manager; @@ -328,7 +328,7 @@ TEST(MessageHelperTestCreate, MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::ChoiceSetMap vis; DataAccessor< ::application_manager::ChoiceSetMap> data_accessor( - vis, std::make_shared(true)); + vis, std::make_shared()); smart_objects::SmartObjectSPtr smartObjectPtr = utils::MakeShared(); @@ -375,7 +375,7 @@ TEST(MessageHelperTestCreate, CreateAddSubMenuRequestToHMI_SendObject_Equal) { MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::SubMenuMap vis; DataAccessor< ::application_manager::SubMenuMap> data_accessor( - vis, std::make_shared(true)); + vis, std::make_shared()); smart_objects::SmartObjectSPtr smartObjectPtr = utils::MakeShared(); @@ -415,7 +415,7 @@ TEST(MessageHelperTestCreate, MockApplicationSharedPtr appSharedMock = utils::MakeShared(); application_manager::SubMenuMap vis; DataAccessor< ::application_manager::SubMenuMap> data_accessor( - vis, std::make_shared(true)); + vis, std::make_shared()); EXPECT_CALL(*appSharedMock, sub_menu_map()).WillOnce(Return(data_accessor)); diff --git a/src/components/policy/policy_regular/src/cache_manager.cc b/src/components/policy/policy_regular/src/cache_manager.cc index aee5c02d4c..bfca801f74 100644 --- a/src/components/policy/policy_regular/src/cache_manager.cc +++ b/src/components/policy/policy_regular/src/cache_manager.cc @@ -103,7 +103,7 @@ CacheManager::CacheManager() : CacheManagerInterface() , pt_(new policy_table::Table) , backup_(new SQLPTRepresentation()) - , update_required(false) + , update_required(false){ LOG4CXX_AUTO_TRACE(logger_); backuper_ = new BackgroundBackuper(this); backup_thread_ = threads::CreateThread("Backup thread", backuper_); -- cgit v1.2.1 From baccac8a8b4ad7f9eca9130cf24b0ca35d6cfd4d Mon Sep 17 00:00:00 2001 From: Conlain Kelly Date: Wed, 18 Jul 2018 14:59:02 -0400 Subject: Cleanup, add more error checking --- src/components/utils/src/lock_boost.cc | 49 +++++++++++----------------------- 1 file changed, 15 insertions(+), 34 deletions(-) (limited to 'src/components') diff --git a/src/components/utils/src/lock_boost.cc b/src/components/utils/src/lock_boost.cc index e3c4a8cb4f..a76bd02a78 100644 --- a/src/components/utils/src/lock_boost.cc +++ b/src/components/utils/src/lock_boost.cc @@ -32,7 +32,6 @@ #include #include -#include #include #include #include "utils/lock.h" @@ -45,39 +44,30 @@ CREATE_LOGGERPTR_GLOBAL(logger_, "Utils") Lock::Lock() : lock_taken_(0) {} Lock::~Lock() { - // std::cerr << "destructing lock " << &mutex_ << " with lock count " - // << lock_taken_ << '\n'; if (lock_taken_ > 0) { LOG4CXX_FATAL(logger_, "Destroying non-released regular mutex " << &mutex_); } } void Lock::Acquire() { - // try { + try { mutex_.lock(); - // std::cerr << "acquiring lock " << &mutex_ << " with lock count " - // << lock_taken_ << '\n'; - // } catch (std::exception err) { - // LOG4CXX_FATAL(logger_, - // "Failed to acquire mutex " << &mutex_ << ": " << - // err.what()); - // NOTREACHED(); - // } + } catch (std::exception err) { + LOG4CXX_FATAL(logger_, + "Failed to acquire mutex " << &mutex_ << ": " << + err.what()); + NOTREACHED(); + } AssertFreeAndMarkTaken(); } void Lock::Release() { - // std::cerr << "releasing lock " << &mutex_ << " with lock count " - // << lock_taken_ << '\n'; AssertTakenAndMarkFree(); - mutex_.unlock(); } bool Lock::Try() { bool status = mutex_.try_lock(); - // std::cerr << "trying lock " << &mutex_ << " with lock count " << lock_taken_ - // << " status was " << status << '\n'; if (status) { AssertFreeAndMarkTaken(); } @@ -89,7 +79,6 @@ void Lock::AssertFreeAndMarkTaken() { LOG4CXX_FATAL(logger_, "Locking already taken not recursive mutex"); NOTREACHED(); } - lock_taken_++; } @@ -106,8 +95,6 @@ void Lock::AssertTakenAndMarkFree() { RecursiveLock::RecursiveLock() : lock_taken_(0) {} RecursiveLock::~RecursiveLock() { - // std::cerr << "destructing recursive lock " << &mutex_ << " with lock count " - // << lock_taken_ << '\n'; if (lock_taken_ > 0) { LOG4CXX_FATAL(logger_, "Destroying non-released recursive mutex " << &mutex_); @@ -115,31 +102,25 @@ RecursiveLock::~RecursiveLock() { } void RecursiveLock::Acquire() { - // try { + try { mutex_.lock(); - // std::cerr << "acquiring recursive lock " << &mutex_ << " with lock count " - // << lock_taken_ << '\n'; - // } catch (std::exception err) { - // LOG4CXX_FATAL(logger_, - // "Failed to acquire mutex " << &mutex_ << ": " << - // err.what()); - // NOTREACHED(); - // } - + } catch (std::exception err) { + LOG4CXX_FATAL(logger_, + "Failed to acquire mutex " << &mutex_ << ": " << + err.what()); + NOTREACHED(); + } AssertFreeAndMarkTaken(); } void RecursiveLock::Release() { - std::cerr << "releasing recursive lock " << &mutex_ << " with lock count " - << lock_taken_ << '\n'; + AssertTakenAndMarkFree(); mutex_.unlock(); } bool RecursiveLock::Try() { bool status = mutex_.try_lock(); - // std::cerr << "trying recursive lock " << &mutex_ << " with lock count " - // << lock_taken_ << " status was " << status << '\n'; if (status) { AssertFreeAndMarkTaken(); } -- cgit v1.2.1 From 1257372af4cc6d6b350f4a91ec823f9c00161858 Mon Sep 17 00:00:00 2001 From: Conlain Kelly Date: Wed, 18 Jul 2018 15:01:15 -0400 Subject: style fix --- .../include/application_manager/application_manager_impl.h | 3 ++- .../src/app_launch/app_launch_data_json.cc | 3 +-- .../application_manager/src/application_data_impl.cc | 3 ++- .../application_manager/src/application_manager_impl.cc | 3 ++- .../src/event_engine/event_dispatcher_impl.cc | 3 +-- src/components/policy/policy_regular/src/cache_manager.cc | 2 +- src/components/utils/src/lock_boost.cc | 11 ++++------- 7 files changed, 13 insertions(+), 15 deletions(-) (limited to 'src/components') diff --git a/src/components/application_manager/include/application_manager/application_manager_impl.h b/src/components/application_manager/include/application_manager/application_manager_impl.h index 79d5dda867..4fee30eb8a 100644 --- a/src/components/application_manager/include/application_manager/application_manager_impl.h +++ b/src/components/application_manager/include/application_manager/application_manager_impl.h @@ -1450,7 +1450,8 @@ class ApplicationManagerImpl ForbiddenApps forbidden_applications; // Lock for applications list - mutable std::shared_ptr applications_list_lock_ptr_; + mutable std::shared_ptr + applications_list_lock_ptr_; mutable std::shared_ptr apps_to_register_list_lock_ptr_; mutable sync_primitives::Lock subscribed_way_points_apps_lock_; diff --git a/src/components/application_manager/src/app_launch/app_launch_data_json.cc b/src/components/application_manager/src/app_launch/app_launch_data_json.cc index 99a6416a69..f7d24c26b1 100644 --- a/src/components/application_manager/src/app_launch/app_launch_data_json.cc +++ b/src/components/application_manager/src/app_launch/app_launch_data_json.cc @@ -43,8 +43,7 @@ CREATE_LOGGERPTR_GLOBAL(logger_, "AppLaunch") AppLaunchDataJson::AppLaunchDataJson(const AppLaunchSettings& settings, resumption::LastState& last_state) - : AppLaunchDataImpl(settings) - , last_state_(last_state) {} + : AppLaunchDataImpl(settings), last_state_(last_state) {} AppLaunchDataJson::~AppLaunchDataJson() {} diff --git a/src/components/application_manager/src/application_data_impl.cc b/src/components/application_manager/src/application_data_impl.cc index 356c75dc77..3cfbeb2602 100644 --- a/src/components/application_manager/src/application_data_impl.cc +++ b/src/components/application_manager/src/application_data_impl.cc @@ -184,7 +184,8 @@ DynamicApplicationDataImpl::DynamicApplicationDataImpl() , choice_set_map_() , choice_set_map_lock_ptr_(std::make_shared()) , performinteraction_choice_set_map_() - , performinteraction_choice_set_lock_ptr_(std::make_shared()) + , performinteraction_choice_set_lock_ptr_( + std::make_shared()) , is_perform_interaction_active_(false) , is_reset_global_properties_active_(false) , perform_interaction_mode_(-1) {} diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index 2c9f5d2f6f..3174e27607 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -143,7 +143,8 @@ ApplicationManagerImpl::ApplicationManagerImpl( const ApplicationManagerSettings& am_settings, const policy::PolicySettings& policy_settings) : settings_(am_settings) - , applications_list_lock_ptr_(std::make_shared()) + , applications_list_lock_ptr_( + std::make_shared()) , apps_to_register_list_lock_ptr_(std::make_shared()) , audio_pass_thru_active_(false) , audio_pass_thru_app_id_(0) diff --git a/src/components/application_manager/src/event_engine/event_dispatcher_impl.cc b/src/components/application_manager/src/event_engine/event_dispatcher_impl.cc index 505465d001..f1e5bf9735 100644 --- a/src/components/application_manager/src/event_engine/event_dispatcher_impl.cc +++ b/src/components/application_manager/src/event_engine/event_dispatcher_impl.cc @@ -39,8 +39,7 @@ namespace application_manager { namespace event_engine { using namespace sync_primitives; -EventDispatcherImpl::EventDispatcherImpl() - : observers_event_() {} +EventDispatcherImpl::EventDispatcherImpl() : observers_event_() {} EventDispatcherImpl::~EventDispatcherImpl() {} diff --git a/src/components/policy/policy_regular/src/cache_manager.cc b/src/components/policy/policy_regular/src/cache_manager.cc index bfca801f74..64a16e5e6e 100644 --- a/src/components/policy/policy_regular/src/cache_manager.cc +++ b/src/components/policy/policy_regular/src/cache_manager.cc @@ -103,7 +103,7 @@ CacheManager::CacheManager() : CacheManagerInterface() , pt_(new policy_table::Table) , backup_(new SQLPTRepresentation()) - , update_required(false){ + , update_required(false) { LOG4CXX_AUTO_TRACE(logger_); backuper_ = new BackgroundBackuper(this); backup_thread_ = threads::CreateThread("Backup thread", backuper_); diff --git a/src/components/utils/src/lock_boost.cc b/src/components/utils/src/lock_boost.cc index a76bd02a78..c22ac467b5 100644 --- a/src/components/utils/src/lock_boost.cc +++ b/src/components/utils/src/lock_boost.cc @@ -51,11 +51,10 @@ Lock::~Lock() { void Lock::Acquire() { try { - mutex_.lock(); + mutex_.lock(); } catch (std::exception err) { LOG4CXX_FATAL(logger_, - "Failed to acquire mutex " << &mutex_ << ": " << - err.what()); + "Failed to acquire mutex " << &mutex_ << ": " << err.what()); NOTREACHED(); } AssertFreeAndMarkTaken(); @@ -103,18 +102,16 @@ RecursiveLock::~RecursiveLock() { void RecursiveLock::Acquire() { try { - mutex_.lock(); + mutex_.lock(); } catch (std::exception err) { LOG4CXX_FATAL(logger_, - "Failed to acquire mutex " << &mutex_ << ": " << - err.what()); + "Failed to acquire mutex " << &mutex_ << ": " << err.what()); NOTREACHED(); } AssertFreeAndMarkTaken(); } void RecursiveLock::Release() { - AssertTakenAndMarkFree(); mutex_.unlock(); } -- cgit v1.2.1 From adba01da38c865370d378eb2f0d237543a5b12d4 Mon Sep 17 00:00:00 2001 From: Conlain Kelly Date: Wed, 18 Jul 2018 15:08:12 -0400 Subject: rename cond var implementation file --- .../utils/src/conditional_variable_boost.cc | 144 +++++++++++++++++++++ .../utils/src/conditional_variable_posix.cc | 144 --------------------- 2 files changed, 144 insertions(+), 144 deletions(-) create mode 100644 src/components/utils/src/conditional_variable_boost.cc delete mode 100644 src/components/utils/src/conditional_variable_posix.cc (limited to 'src/components') diff --git a/src/components/utils/src/conditional_variable_boost.cc b/src/components/utils/src/conditional_variable_boost.cc new file mode 100644 index 0000000000..109e2bac9b --- /dev/null +++ b/src/components/utils/src/conditional_variable_boost.cc @@ -0,0 +1,144 @@ +/* + * Copyright (c) 2013, Ford Motor Company + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following + * disclaimer in the documentation and/or other materials provided with the + * distribution. + * + * Neither the name of the Ford Motor Company nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ +#include "utils/conditional_variable.h" + +#include +#include + +#include "utils/lock.h" +#include "utils/logger.h" + +namespace { +const long kNanosecondsPerSecond = 1000000000; +const long kMillisecondsPerSecond = 1000; +const long kNanosecondsPerMillisecond = 1000000; +} // namespace + +namespace sync_primitives { + +CREATE_LOGGERPTR_GLOBAL(logger_, "Utils") + +ConditionalVariable::ConditionalVariable() {} + +ConditionalVariable::~ConditionalVariable() {} + +void ConditionalVariable::NotifyOne() { + cond_var_.notify_one(); +} + +void ConditionalVariable::Broadcast() { + cond_var_.notify_all(); +} + +bool ConditionalVariable::Wait(BaseLock& lock) { + // NOTE this grossness is due to boost mutex and recursive mutex not sharing a + // superclass + + lock.AssertTakenAndMarkFree(); + // try { + // What kind of lock are we ? + if (Lock* test_lock = dynamic_cast(&lock)) { + // Regular lock + cond_var_.wait(test_lock->mutex_); + } else if (RecursiveLock* test_rec_lock = + dynamic_cast(&lock)) { + // Recursive lock + cond_var_.wait(test_rec_lock->mutex_); + } else { + // unknown + lock.AssertFreeAndMarkTaken(); + + LOG4CXX_ERROR(logger_, "Unknown lock type!"); + return false; + } + // } catch (std::exception err) { + // lock.AssertFreeAndMarkTaken(); + // + // LOG4CXX_ERROR( + // logger_, + // "Failed to wait for conditional variable, exception:" << err.what()); + // return false; + // } + lock.AssertFreeAndMarkTaken(); + + return true; +} + +bool ConditionalVariable::Wait(AutoLock& auto_lock) { + BaseLock& lock = auto_lock.GetLock(); + return Wait(lock); +} + +ConditionalVariable::WaitStatus ConditionalVariable::WaitFor( + AutoLock& auto_lock, uint32_t milliseconds) { + BaseLock& lock = auto_lock.GetLock(); + + WaitStatus wait_status = kNoTimeout; + lock.AssertTakenAndMarkFree(); + try { + bool timeout = true; + + // What kind of lock are we ? + if (Lock* test_lock = dynamic_cast(&lock)) { + // Regular lock + // cond_var_.wait(test_lock->mutex_); + timeout = cond_var_.timed_wait( + test_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); + } else if (RecursiveLock* test_rec_lock = + dynamic_cast(&lock)) { + // Recursive lock + // cond_var_.wait(test_rec_lock->mutex_); + timeout = cond_var_.timed_wait( + test_rec_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); + } else { + // unknown + LOG4CXX_ERROR(logger_, "Unknown lock type!"); + } + + if (!timeout) { + wait_status = kTimeout; + } + } catch (boost::thread_interrupted inter) { + wait_status = kNoTimeout; + } + // catch (std::exception err) { + // LOG4CXX_ERROR( + // logger_, + // "Failed to timewait for conditional variable timedwait_status: " + // << err.what()); + // } + lock.AssertFreeAndMarkTaken(); + + return wait_status; +} + +} // namespace sync_primitives diff --git a/src/components/utils/src/conditional_variable_posix.cc b/src/components/utils/src/conditional_variable_posix.cc deleted file mode 100644 index 109e2bac9b..0000000000 --- a/src/components/utils/src/conditional_variable_posix.cc +++ /dev/null @@ -1,144 +0,0 @@ -/* - * Copyright (c) 2013, Ford Motor Company - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following - * disclaimer in the documentation and/or other materials provided with the - * distribution. - * - * Neither the name of the Ford Motor Company nor the names of its contributors - * may be used to endorse or promote products derived from this software - * without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" - * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE - * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR - * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF - * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS - * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN - * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) - * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE - * POSSIBILITY OF SUCH DAMAGE. - */ -#include "utils/conditional_variable.h" - -#include -#include - -#include "utils/lock.h" -#include "utils/logger.h" - -namespace { -const long kNanosecondsPerSecond = 1000000000; -const long kMillisecondsPerSecond = 1000; -const long kNanosecondsPerMillisecond = 1000000; -} // namespace - -namespace sync_primitives { - -CREATE_LOGGERPTR_GLOBAL(logger_, "Utils") - -ConditionalVariable::ConditionalVariable() {} - -ConditionalVariable::~ConditionalVariable() {} - -void ConditionalVariable::NotifyOne() { - cond_var_.notify_one(); -} - -void ConditionalVariable::Broadcast() { - cond_var_.notify_all(); -} - -bool ConditionalVariable::Wait(BaseLock& lock) { - // NOTE this grossness is due to boost mutex and recursive mutex not sharing a - // superclass - - lock.AssertTakenAndMarkFree(); - // try { - // What kind of lock are we ? - if (Lock* test_lock = dynamic_cast(&lock)) { - // Regular lock - cond_var_.wait(test_lock->mutex_); - } else if (RecursiveLock* test_rec_lock = - dynamic_cast(&lock)) { - // Recursive lock - cond_var_.wait(test_rec_lock->mutex_); - } else { - // unknown - lock.AssertFreeAndMarkTaken(); - - LOG4CXX_ERROR(logger_, "Unknown lock type!"); - return false; - } - // } catch (std::exception err) { - // lock.AssertFreeAndMarkTaken(); - // - // LOG4CXX_ERROR( - // logger_, - // "Failed to wait for conditional variable, exception:" << err.what()); - // return false; - // } - lock.AssertFreeAndMarkTaken(); - - return true; -} - -bool ConditionalVariable::Wait(AutoLock& auto_lock) { - BaseLock& lock = auto_lock.GetLock(); - return Wait(lock); -} - -ConditionalVariable::WaitStatus ConditionalVariable::WaitFor( - AutoLock& auto_lock, uint32_t milliseconds) { - BaseLock& lock = auto_lock.GetLock(); - - WaitStatus wait_status = kNoTimeout; - lock.AssertTakenAndMarkFree(); - try { - bool timeout = true; - - // What kind of lock are we ? - if (Lock* test_lock = dynamic_cast(&lock)) { - // Regular lock - // cond_var_.wait(test_lock->mutex_); - timeout = cond_var_.timed_wait( - test_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); - } else if (RecursiveLock* test_rec_lock = - dynamic_cast(&lock)) { - // Recursive lock - // cond_var_.wait(test_rec_lock->mutex_); - timeout = cond_var_.timed_wait( - test_rec_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); - } else { - // unknown - LOG4CXX_ERROR(logger_, "Unknown lock type!"); - } - - if (!timeout) { - wait_status = kTimeout; - } - } catch (boost::thread_interrupted inter) { - wait_status = kNoTimeout; - } - // catch (std::exception err) { - // LOG4CXX_ERROR( - // logger_, - // "Failed to timewait for conditional variable timedwait_status: " - // << err.what()); - // } - lock.AssertFreeAndMarkTaken(); - - return wait_status; -} - -} // namespace sync_primitives -- cgit v1.2.1 From c96b488f0a5c6f114016ab9c013a347b5257d7d9 Mon Sep 17 00:00:00 2001 From: Conlain Kelly Date: Wed, 18 Jul 2018 15:14:34 -0400 Subject: maybe final cleanup --- .../utils/src/conditional_variable_boost.cc | 51 +++++++++++----------- 1 file changed, 25 insertions(+), 26 deletions(-) (limited to 'src/components') diff --git a/src/components/utils/src/conditional_variable_boost.cc b/src/components/utils/src/conditional_variable_boost.cc index 109e2bac9b..f1c23c13a4 100644 --- a/src/components/utils/src/conditional_variable_boost.cc +++ b/src/components/utils/src/conditional_variable_boost.cc @@ -64,30 +64,30 @@ bool ConditionalVariable::Wait(BaseLock& lock) { // superclass lock.AssertTakenAndMarkFree(); - // try { - // What kind of lock are we ? - if (Lock* test_lock = dynamic_cast(&lock)) { - // Regular lock - cond_var_.wait(test_lock->mutex_); - } else if (RecursiveLock* test_rec_lock = - dynamic_cast(&lock)) { - // Recursive lock - cond_var_.wait(test_rec_lock->mutex_); - } else { - // unknown + try { + // What kind of lock are we ? + if (Lock* test_lock = dynamic_cast(&lock)) { + // Regular lock + cond_var_.wait(test_lock->mutex_); + } else if (RecursiveLock* test_rec_lock = + dynamic_cast(&lock)) { + // Recursive lock + cond_var_.wait(test_rec_lock->mutex_); + } else { + // unknown + lock.AssertFreeAndMarkTaken(); + + LOG4CXX_ERROR(logger_, "Unknown lock type!"); + return false; + } + } catch (std::exception err) { lock.AssertFreeAndMarkTaken(); - LOG4CXX_ERROR(logger_, "Unknown lock type!"); + LOG4CXX_ERROR( + logger_, + "Failed to wait for conditional variable, exception:" << err.what()); return false; } - // } catch (std::exception err) { - // lock.AssertFreeAndMarkTaken(); - // - // LOG4CXX_ERROR( - // logger_, - // "Failed to wait for conditional variable, exception:" << err.what()); - // return false; - // } lock.AssertFreeAndMarkTaken(); return true; @@ -129,13 +129,12 @@ ConditionalVariable::WaitStatus ConditionalVariable::WaitFor( } } catch (boost::thread_interrupted inter) { wait_status = kNoTimeout; + } catch (std::exception err) { + LOG4CXX_ERROR( + logger_, + "Failed to timewait for conditional variable timedwait_status: " + << err.what()); } - // catch (std::exception err) { - // LOG4CXX_ERROR( - // logger_, - // "Failed to timewait for conditional variable timedwait_status: " - // << err.what()); - // } lock.AssertFreeAndMarkTaken(); return wait_status; -- cgit v1.2.1 From f8a549cdf0e1b52ef66f1ca703a1d07d4c99f645 Mon Sep 17 00:00:00 2001 From: Conlain Kelly Date: Tue, 24 Jul 2018 14:31:17 -0400 Subject: Remove exception-handling code, extra constructors Note that locks will now crash on error via an uncaught Boost exception This is because SDL prohibits exceptions, but Boost uses them internally --- .../src/application_manager_impl.cc | 1 - .../connection_handler/src/connection.cc | 1 - .../utils/src/conditional_variable_boost.cc | 87 +++++++++------------- src/components/utils/src/lock_boost.cc | 16 +--- 4 files changed, 36 insertions(+), 69 deletions(-) (limited to 'src/components') diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index a0be3d1939..9b936bdf41 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -166,7 +166,6 @@ ApplicationManagerImpl::ApplicationManagerImpl( , resume_ctrl_(new resumption::ResumeCtrlImpl(*this)) , navi_close_app_timeout_(am_settings.stop_streaming_timeout()) , navi_end_stream_timeout_(am_settings.stop_streaming_timeout()) - , stopping_application_mng_lock_() , state_ctrl_(*this) , application_list_update_timer_( "AM ListUpdater", diff --git a/src/components/connection_handler/src/connection.cc b/src/components/connection_handler/src/connection.cc index 08b37b9026..5a35919c6c 100644 --- a/src/components/connection_handler/src/connection.cc +++ b/src/components/connection_handler/src/connection.cc @@ -81,7 +81,6 @@ Connection::Connection(ConnectionHandle connection_handle, : connection_handler_(connection_handler) , connection_handle_(connection_handle) , connection_device_handle_(connection_device_handle) - , session_map_lock_() , primary_connection_handle_(0) , heartbeat_timeout_(heartbeat_timeout) { LOG4CXX_AUTO_TRACE(logger_); diff --git a/src/components/utils/src/conditional_variable_boost.cc b/src/components/utils/src/conditional_variable_boost.cc index f1c23c13a4..c07b8284ea 100644 --- a/src/components/utils/src/conditional_variable_boost.cc +++ b/src/components/utils/src/conditional_variable_boost.cc @@ -64,29 +64,18 @@ bool ConditionalVariable::Wait(BaseLock& lock) { // superclass lock.AssertTakenAndMarkFree(); - try { - // What kind of lock are we ? - if (Lock* test_lock = dynamic_cast(&lock)) { - // Regular lock - cond_var_.wait(test_lock->mutex_); - } else if (RecursiveLock* test_rec_lock = - dynamic_cast(&lock)) { - // Recursive lock - cond_var_.wait(test_rec_lock->mutex_); - } else { - // unknown - lock.AssertFreeAndMarkTaken(); - - LOG4CXX_ERROR(logger_, "Unknown lock type!"); - return false; - } - } catch (std::exception err) { - lock.AssertFreeAndMarkTaken(); - - LOG4CXX_ERROR( - logger_, - "Failed to wait for conditional variable, exception:" << err.what()); - return false; + // What kind of lock are we ? + if (Lock* test_lock = dynamic_cast(&lock)) { + // Regular lock + cond_var_.wait(test_lock->mutex_); + } else if (RecursiveLock* test_rec_lock = + dynamic_cast(&lock)) { + // Recursive lock + cond_var_.wait(test_rec_lock->mutex_); + } else { + // unknown, give up the lock + LOG4CXX_ERROR(logger_, "Unknown lock type!"); + NOTREACHED(); } lock.AssertFreeAndMarkTaken(); @@ -104,36 +93,28 @@ ConditionalVariable::WaitStatus ConditionalVariable::WaitFor( WaitStatus wait_status = kNoTimeout; lock.AssertTakenAndMarkFree(); - try { - bool timeout = true; - - // What kind of lock are we ? - if (Lock* test_lock = dynamic_cast(&lock)) { - // Regular lock - // cond_var_.wait(test_lock->mutex_); - timeout = cond_var_.timed_wait( - test_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); - } else if (RecursiveLock* test_rec_lock = - dynamic_cast(&lock)) { - // Recursive lock - // cond_var_.wait(test_rec_lock->mutex_); - timeout = cond_var_.timed_wait( - test_rec_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); - } else { - // unknown - LOG4CXX_ERROR(logger_, "Unknown lock type!"); - } - - if (!timeout) { - wait_status = kTimeout; - } - } catch (boost::thread_interrupted inter) { - wait_status = kNoTimeout; - } catch (std::exception err) { - LOG4CXX_ERROR( - logger_, - "Failed to timewait for conditional variable timedwait_status: " - << err.what()); + bool timeout = true; + + // What kind of lock are we ? + if (Lock* test_lock = dynamic_cast(&lock)) { + // Regular lock + // cond_var_.wait(test_lock->mutex_); + timeout = cond_var_.timed_wait( + test_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); + } else if (RecursiveLock* test_rec_lock = + dynamic_cast(&lock)) { + // Recursive lock + // cond_var_.wait(test_rec_lock->mutex_); + timeout = cond_var_.timed_wait( + test_rec_lock->mutex_, boost::posix_time::milliseconds(milliseconds)); + } else { + // this is an unknown lock, we have an issue + LOG4CXX_ERROR(logger_, "Unknown lock type!"); + NOTREACHED(); + } + + if (!timeout) { + wait_status = kTimeout; } lock.AssertFreeAndMarkTaken(); diff --git a/src/components/utils/src/lock_boost.cc b/src/components/utils/src/lock_boost.cc index c22ac467b5..2299bcf77d 100644 --- a/src/components/utils/src/lock_boost.cc +++ b/src/components/utils/src/lock_boost.cc @@ -50,13 +50,7 @@ Lock::~Lock() { } void Lock::Acquire() { - try { - mutex_.lock(); - } catch (std::exception err) { - LOG4CXX_FATAL(logger_, - "Failed to acquire mutex " << &mutex_ << ": " << err.what()); - NOTREACHED(); - } + mutex_.lock(); AssertFreeAndMarkTaken(); } @@ -101,13 +95,7 @@ RecursiveLock::~RecursiveLock() { } void RecursiveLock::Acquire() { - try { - mutex_.lock(); - } catch (std::exception err) { - LOG4CXX_FATAL(logger_, - "Failed to acquire mutex " << &mutex_ << ": " << err.what()); - NOTREACHED(); - } + mutex_.lock(); AssertFreeAndMarkTaken(); } -- cgit v1.2.1