Fixes a leak in ThreadLocal.

diff --git a/include/gtest/internal/gtest-port.h b/include/gtest/internal/gtest-port.h
index 66cfe46ec5fd97aefddcf2ae632eb90c8c3cea23..a2a62be920965d52f03a947c8cba2d3d132423f5 100644 (file)
--- 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 <typename T>
 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 577f60996aa0c6ad0f69fa1f134d69b83d9103c8..37258602fa5cf5a779947cd427e20e909fc9258d 100644 (file)
--- 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<bool> 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 <typename T>
-void CallThreadLocalGet(ThreadLocal<T>* threadLocal) {
-  threadLocal->get();
+typedef ThreadLocal<DestructorTracker>* 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<DestructorTracker> 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<DestructorTracker> thread_local;
-    ThreadWithParam<ThreadLocal<DestructorTracker>*> thread(
-        &CallThreadLocalGet<DestructorTracker>, &thread_local, NULL);
+    ASSERT_EQ(1U, g_destroyed.size());
+    ASSERT_FALSE(g_destroyed[0]);
+
+    // This creates another DestructorTracker object in the new thread.
+    ThreadWithParam<ThreadParam> 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