From b9a7cead1cdf588b9b00ec46f38a727b14681c5b Mon Sep 17 00:00:00 2001 From: "zhanyong.wan" Date: Fri, 26 Mar 2010 20:23:06 +0000 Subject: Fixes a leak in ThreadLocal. --- include/gtest/internal/gtest-port.h | 24 +++++++++-- test/gtest-port_test.cc | 81 +++++++++++++++++++++++++++++-------- 2 files changed, 84 insertions(+), 21 deletions(-) diff --git a/include/gtest/internal/gtest-port.h b/include/gtest/internal/gtest-port.h index 66cfe46e..a2a62be9 100644 --- a/include/gtest/internal/gtest-port.h +++ b/include/gtest/internal/gtest-port.h @@ -1084,10 +1084,19 @@ extern "C" inline void DeleteThreadLocalValue(void* value_holder) { // // The template type argument T must have a public copy constructor. // In addition, the default ThreadLocal constructor requires T to have -// a public default constructor. An object managed by a ThreadLocal -// instance for a thread is guaranteed to exist at least until the -// earliest of the two events: (a) the thread terminates or (b) the -// ThreadLocal object is destroyed. +// a public default constructor. +// +// An object managed for a thread by a ThreadLocal instance is deleted +// when the thread exits. Or, if the ThreadLocal instance dies in +// that thread, when the ThreadLocal dies. It's the user's +// responsibility to ensure that all other threads using a ThreadLocal +// have exited when it dies, or the per-thread objects for those +// threads will not be deleted. +// +// Google Test only uses global ThreadLocal objects. That means they +// will die after main() has returned. Therefore, no per-thread +// object managed by Google Test will be leaked as long as all threads +// using Google Test have exited when main() returns. template class ThreadLocal { public: @@ -1097,6 +1106,11 @@ class ThreadLocal { default_(value) {} ~ThreadLocal() { + // Destroys the managed object for the current thread, if any. + DeleteThreadLocalValue(pthread_getspecific(key_)); + + // Releases resources associated with the key. This will *not* + // delete managed objects for other threads. GTEST_CHECK_POSIX_SUCCESS_(pthread_key_delete(key_)); } @@ -1120,6 +1134,8 @@ class ThreadLocal { static pthread_key_t CreateKey() { pthread_key_t key; + // When a thread exits, DeleteThreadLocalValue() will be called on + // the object managed for that thread. GTEST_CHECK_POSIX_SUCCESS_( pthread_key_create(&key, &DeleteThreadLocalValue)); return key; diff --git a/test/gtest-port_test.cc b/test/gtest-port_test.cc index 577f6099..37258602 100644 --- a/test/gtest-port_test.cc +++ b/test/gtest-port_test.cc @@ -911,47 +911,93 @@ TEST(ThreadLocalTest, ParameterizedConstructorSetsDefault) { EXPECT_STREQ("foo", result.c_str()); } -// DestructorTracker keeps track of whether the class instances have been -// destroyed. The static synchronization mutex has to be defined outside -// of the class, due to syntax of its definition. -static GTEST_DEFINE_STATIC_MUTEX_(destructor_tracker_mutex); - +// DestructorTracker keeps track of whether its instances have been +// destroyed. static std::vector g_destroyed; class DestructorTracker { public: DestructorTracker() : index_(GetNewIndex()) {} + DestructorTracker(const DestructorTracker& /* rhs */) + : index_(GetNewIndex()) {} ~DestructorTracker() { - MutexLock lock(&destructor_tracker_mutex); + // We never access g_destroyed concurrently, so we don't need to + // protect the write operation under a mutex. g_destroyed[index_] = true; } private: static int GetNewIndex() { - MutexLock lock(&destructor_tracker_mutex); g_destroyed.push_back(false); return g_destroyed.size() - 1; } const int index_; }; -template -void CallThreadLocalGet(ThreadLocal* threadLocal) { - threadLocal->get(); +typedef ThreadLocal* ThreadParam; + +void CallThreadLocalGet(ThreadParam thread_local) { + thread_local->get(); +} + +// Tests that when a ThreadLocal object dies in a thread, it destroys +// the managed object for that thread. +TEST(ThreadLocalTest, DestroysManagedObjectForOwnThreadWhenDying) { + g_destroyed.clear(); + + { + // The next line default constructs a DestructorTracker object as + // the default value of objects managed by thread_local. + ThreadLocal thread_local; + ASSERT_EQ(1U, g_destroyed.size()); + ASSERT_FALSE(g_destroyed[0]); + + // This creates another DestructorTracker object for the main thread. + thread_local.get(); + ASSERT_EQ(2U, g_destroyed.size()); + ASSERT_FALSE(g_destroyed[0]); + ASSERT_FALSE(g_destroyed[1]); + } + + // Now thread_local has died. It should have destroyed both the + // default value shared by all threads and the value for the main + // thread. + ASSERT_EQ(2U, g_destroyed.size()); + EXPECT_TRUE(g_destroyed[0]); + EXPECT_TRUE(g_destroyed[1]); + + g_destroyed.clear(); } -TEST(ThreadLocalTest, DestroysManagedObjectsNoLaterThanSelf) { +// Tests that when a thread exits, the thread-local object for that +// thread is destroyed. +TEST(ThreadLocalTest, DestroysManagedObjectAtThreadExit) { g_destroyed.clear(); + { + // The next line default constructs a DestructorTracker object as + // the default value of objects managed by thread_local. ThreadLocal thread_local; - ThreadWithParam*> thread( - &CallThreadLocalGet, &thread_local, NULL); + ASSERT_EQ(1U, g_destroyed.size()); + ASSERT_FALSE(g_destroyed[0]); + + // This creates another DestructorTracker object in the new thread. + ThreadWithParam thread( + &CallThreadLocalGet, &thread_local, NULL); thread.Join(); + + // Now the new thread has exited. The per-thread object for it + // should have been destroyed. + ASSERT_EQ(2U, g_destroyed.size()); + ASSERT_FALSE(g_destroyed[0]); + ASSERT_TRUE(g_destroyed[1]); } - // Verifies that all DestructorTracker objects there were have been - // destroyed. - for (size_t i = 0; i < g_destroyed.size(); ++i) - EXPECT_TRUE(g_destroyed[i]) << "at index " << i; + + // Now thread_local has died. The default value should have been + // destroyed too. + ASSERT_EQ(2U, g_destroyed.size()); + EXPECT_TRUE(g_destroyed[0]); + EXPECT_TRUE(g_destroyed[1]); g_destroyed.clear(); } @@ -965,6 +1011,7 @@ TEST(ThreadLocalTest, ThreadLocalMutationsAffectOnlyCurrentThread) { RunFromThread(&RetrieveThreadLocalValue, make_pair(&thread_local, &result)); EXPECT_TRUE(result.c_str() == NULL); } + #endif // GTEST_IS_THREADSAFE } // namespace internal -- cgit v1.2.3