From 569853ed10dda98557360f52544bdf2873488f59 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Sat, 22 Nov 2014 00:04:41 -0800 Subject: [PATCH] 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 | 16 ++-------------- db/db_test.cc | 1 + db/deletefile_test.cc | 1 + util/thread_status_impl.cc | 13 +++++++------ util/thread_status_impl.h | 2 +- 5 files changed, 12 insertions(+), 21 deletions(-) diff --git a/db/db_impl.cc b/db/db_impl.cc index 9688af26b..4b9b96ec7 100644 --- a/db/db_impl.cc +++ b/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(h)->cfd()); - } } else { for (auto* h : *handles) { delete h; diff --git a/db/db_test.cc b/db/db_test.cc index 76a0b7ff6..6f8fb00c9 100644 --- a/db/db_test.cc +++ b/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(); diff --git a/db/deletefile_test.cc b/db/deletefile_test.cc index 6a6f8e953..9e3ea70aa 100644 --- a/db/deletefile_test.cc +++ b/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; diff --git a/util/thread_status_impl.cc b/util/thread_status_impl.cc index d1cd5ccdc..35dc181e2 100644 --- a/util/thread_status_impl.cc +++ b/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 ThreadStatusImpl::thread_data_set_; -std::unordered_map +std::unordered_map> ThreadStatusImpl::cf_info_map_; std::unordered_map> 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 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); } diff --git a/util/thread_status_impl.h b/util/thread_status_impl.h index a678e0988..a6e9a7e5b 100644 --- a/util/thread_status_impl.h +++ b/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> 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.