Fix leak when create_missing_column_families=true on ThreadStatus

Summary:
An entry of ConstantColumnFamilyInfo is created when:
1. DB::Open
2. CreateColumnFamily.

However, there are cases that DB::Open could also call CreateColumnFamily
when create_missing_column_families=true.  As a result, it will create
duplicate ConstantColumnFamilyInfo and one of them would be leaked.

Test Plan: ./deletefile_test

Reviewers: igor, sdong, ljin

Reviewed By: ljin

Subscribers: dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D29307

db/db_impl.cc

@@ -2493,7 +2493,7 @@ Status DBImpl::CreateColumnFamily(const ColumnFamilyOptions& cf_options,
           "Creating column family [%s] FAILED -- %s",
           column_family_name.c_str(), s.ToString().c_str());
     }
-  }
+  }  // MutexLock l(&mutex_)
 
   // this is outside the mutex
   if (s.ok()) {
@@ -3545,6 +3545,7 @@ Status DB::Open(const DBOptions& db_options, const std::string& dbname,
         if (cfd != nullptr) {
           handles->push_back(
               new ColumnFamilyHandleImpl(cfd, impl, &impl->mutex_));
+          impl->NewThreadStatusCfInfo(cfd);
         } else {
           if (db_options.create_missing_column_families) {
             // missing column family, create it
@@ -3609,19 +3610,6 @@ Status DB::Open(const DBOptions& db_options, const std::string& dbname,
   if (s.ok()) {
     impl->opened_successfully_ = true;
     *dbptr = impl;
-    // TODO(yhchiang): Add NotifyOnDatabaseOpen() here.
-    // Since the column-family handles are only available after DB::Open(),
-    // typically developers will need to pass the returned ColumnFamilyHandles
-    // to their EventListeners in order to maintain the mapping between
-    // column-family-name to ColumnFamilyHandle.  However, some database
-    // events might happen before the user passing those ColumnFamilyHandle to
-    // their Listeners.  To address this, we should have NotifyOnDatabaseOpen()
-    // here which passes the created ColumnFamilyHandle to the Listeners
-    // as the first event after DB::Open().
-    for (auto* h : *handles) {
-      impl->NewThreadStatusCfInfo(
-          reinterpret_cast<ColumnFamilyHandleImpl*>(h)->cfd());
-    }
   } else {
     for (auto* h : *handles) {
       delete h;

db/db_test.cc

@@ -9031,6 +9031,7 @@ TEST(DBTest, GetThreadList) {
     }
   }
   db_->DropColumnFamily(handles_[2]);
+  delete handles_[2];
   handles_.erase(handles_.begin() + 2);
   ThreadStatusImpl::TEST_VerifyColumnFamilyInfoMap(handles_, true);
   Close();

db/deletefile_test.cc

@@ -34,6 +34,7 @@ class DeleteFileTest {
   DeleteFileTest() {
     db_ = nullptr;
     env_ = Env::Default();
+    options_.enable_thread_tracking = true;
     options_.max_background_flushes = 0;
     options_.write_buffer_size = 1024*1024*1000;
     options_.target_file_size_base = 1024*1024*1000;

util/thread_status_impl.cc

@@ -13,7 +13,7 @@ namespace rocksdb {
 __thread ThreadStatusData* ThreadStatusImpl::thread_status_data_ = nullptr;
 std::mutex ThreadStatusImpl::thread_list_mutex_;
 std::unordered_set<ThreadStatusData*> ThreadStatusImpl::thread_data_set_;
-std::unordered_map<const void*, ConstantColumnFamilyInfo*>
+std::unordered_map<const void*, std::unique_ptr<ConstantColumnFamilyInfo>>
     ThreadStatusImpl::cf_info_map_;
 std::unordered_map<const void*, std::unordered_set<const void*>>
     ThreadStatusImpl::db_key_map_;
@@ -66,7 +66,7 @@ Status ThreadStatusImpl::GetThreadList(
     auto iter = cf_info_map_.find(cf_key);
     assert(cf_key == 0 || iter != cf_info_map_.end());
     auto* cf_info = iter != cf_info_map_.end() ?
-        iter->second : nullptr;
+        iter->second.get() : nullptr;
     auto* event_info = thread_data->event_info.load(
         std::memory_order_relaxed);
     const std::string* db_name = nullptr;
@@ -106,7 +106,8 @@ void ThreadStatusImpl::NewColumnFamilyInfo(
     const void* cf_key, const std::string& cf_name) {
   std::lock_guard<std::mutex> lck(thread_list_mutex_);
 
-  cf_info_map_[cf_key] = new ConstantColumnFamilyInfo(db_key, db_name, cf_name);
+  cf_info_map_[cf_key].reset(
+      new ConstantColumnFamilyInfo(db_key, db_name, cf_name));
   db_key_map_[db_key].insert(cf_key);
 }
 
@@ -115,7 +116,7 @@ void ThreadStatusImpl::EraseColumnFamilyInfo(const void* cf_key) {
   auto cf_pair = cf_info_map_.find(cf_key);
   assert(cf_pair != cf_info_map_.end());
 
-  auto* cf_info = cf_pair->second;
+  auto* cf_info = cf_pair->second.get();
   assert(cf_info);
 
   // Remove its entry from db_key_map_ by the following steps:
@@ -126,7 +127,7 @@ void ThreadStatusImpl::EraseColumnFamilyInfo(const void* cf_key) {
   size_t result __attribute__((unused)) = db_pair->second.erase(cf_key);
   assert(result);
 
-  delete cf_info;
+  cf_pair->second.reset();
   result = cf_info_map_.erase(cf_key);
   assert(result);
 }
@@ -144,7 +145,7 @@ void ThreadStatusImpl::EraseDatabaseInfo(const void* db_key) {
   for (auto cf_key : db_pair->second) {
     auto cf_pair = cf_info_map_.find(cf_key);
     assert(cf_pair != cf_info_map_.end());
-    delete cf_pair->second;
+    cf_pair->second.reset();
     result = cf_info_map_.erase(cf_key);
     assert(result);
   }

util/thread_status_impl.h

@@ -151,7 +151,7 @@ class ThreadStatusImpl {
   // closing while GetThreadList function already get the pointer to its
   // CopnstantColumnFamilyInfo.
   static std::unordered_map<
-      const void*, ConstantColumnFamilyInfo*> cf_info_map_;
+      const void*, std::unique_ptr<ConstantColumnFamilyInfo>> cf_info_map_;
 
   // A db_key to cf_key map that allows erasing elements in cf_info_map
   // associated to the same db_key faster.