dump current implementation, currently stuck on bigger SDL lock scoping bug

8 files changed, 140 insertions, 83 deletions

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<am::HMILanguageHandler> 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 <string>
 #include <vector>
 
-#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<MockApplication> MockApplicationSharedPtr;
 typedef std::vector<std::string> StringArray;
 typedef utils::SharedPtr<application_manager::Application> 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<MockApplication>();
   ::application_manager::CommandsMap vis;
-  DataAccessor<application_manager::CommandsMap> data_accessor(vis, sync_primitives::RecursiveLock());
+
+  DataAccessor<application_manager::CommandsMap> 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<MockApplication>();
   CommandsMap vis;
-  DataAccessor<CommandsMap> data_accessor(vis, sync_primitives::RecursiveLock());
+  DataAccessor<CommandsMap> data_accessor(vis, access_lock);
   smart_objects::SmartObjectSPtr smartObjectPtr =
       utils::MakeShared<smart_objects::SmartObject>();
 
@@ -290,9 +295,12 @@ TEST(MessageHelperTestCreate,
 
 TEST(MessageHelperTestCreate,
      CreateAddVRCommandRequestFromChoiceToHMI_SendEmptyData_EmptyList) {
+  sync_primitives::RecursiveLock access_lock;
   MockApplicationSharedPtr appSharedMock = utils::MakeShared<MockApplication>();
   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<MockApplication>();
   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<smart_objects::SmartObject>();
 
@@ -351,9 +362,12 @@ TEST(MessageHelperTestCreate,
 }
 
 TEST(MessageHelperTestCreate, CreateAddSubMenuRequestToHMI_SendObject_Equal) {
+  sync_primitives::RecursiveLock access_lock;
   MockApplicationSharedPtr appSharedMock = utils::MakeShared<MockApplication>();
   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<smart_objects::SmartObject>();
 
@@ -390,9 +404,12 @@ TEST(MessageHelperTestCreate, CreateAddSubMenuRequestToHMI_SendObject_Equal) {
 
 TEST(MessageHelperTestCreate,
      CreateAddSubMenuRequestToHMI_SendEmptyMap_EmptySmartObjectList) {
+  sync_primitives::RecursiveLock access_lock;
   MockApplicationSharedPtr appSharedMock = utils::MakeShared<MockApplication>();
   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<MockApplication>();
   // Creating data acessor
   application_manager::VehicleInfoSubscriptions vis;
+
   DataAccessor<application_manager::VehicleInfoSubscriptions> 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 <stdint.h>
 #include <boost/thread/mutex.hpp>
 #include <boost/thread/recursive_mutex.hpp>
+#include <iostream>
 #include "utils/atomic.h"
 #include "utils/macro.h"
 #include "utils/memory_barrier.h"
-#include <iostream>
 
 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*>(&lock)) {
-      // Regular lock
-      cond_var_.wait<boost::mutex>(test_lock->mutex_);
-    } else if (RecursiveLock* test_rec_lock =
-                   dynamic_cast<RecursiveLock*>(&lock)) {
-      // Recursive lock
-      cond_var_.wait<boost::recursive_mutex>(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*>(&lock)) {
+    // Regular lock
+    cond_var_.wait<boost::mutex>(test_lock->mutex_);
+  } else if (RecursiveLock* test_rec_lock =
+                 dynamic_cast<RecursiveLock*>(&lock)) {
+    // Recursive lock
+    cond_var_.wait<boost::recursive_mutex>(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 <iostream>
 #include <pthread.h>
+#include <iostream>
 
 #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