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

4 files changed, 80 insertions, 51 deletions

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