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 --- .../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 + 4 files changed, 80 insertions(+), 51 deletions(-) (limited to 'src/components/utils') 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