From cec2c6436b999c8b6bd6f3b8d8f8c012d554cc40 Mon Sep 17 00:00:00 2001 From: Aaron Gao Date: Tue, 23 Aug 2016 18:20:41 -0700 Subject: [PATCH] fix data race in NewIndexIterator() in block_based_table_reader.cc Summary: fixed data race described in https://github.com/facebook/rocksdb/issues/1267 and add regression test Test Plan: ./table_test --gtest_filter=BlockBasedTableTest.NewIndexIteratorLeak make all check -j64 core dump before fix. ok after fix. Reviewers: andrewkr, sdong Reviewed By: sdong Subscribers: igor, andrewkr, dhruba, leveldb Differential Revision: https://reviews.facebook.net/D62361 --- table/block_based_table_reader.cc | 16 ++++++--- table/table_test.cc | 60 +++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index 4f1e49b5c..15ac6a7c5 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -45,6 +45,7 @@ #include "util/perf_context_imp.h" #include "util/stop_watch.h" #include "util/string_util.h" +#include "util/sync_point.h" namespace rocksdb { @@ -538,12 +539,19 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions, // We've successfully read the footer and the index block: we're // ready to serve requests. + // Better not mutate rep_ after the creation. eg. internal_prefix_transform + // raw pointer will be used to create HashIndexReader, whose reset may + // access a dangling pointer. Rep* rep = new BlockBasedTable::Rep(ioptions, env_options, table_options, internal_comparator, skip_filters); rep->file = std::move(file); rep->footer = footer; rep->index_type = table_options.index_type; rep->hash_index_allow_collision = table_options.hash_index_allow_collision; + // We need to wrap data with internal_prefix_transform to make sure it can + // handle prefix correctly. + rep->internal_prefix_transform.reset( + new InternalKeySliceTransform(rep->ioptions.prefix_extractor)); SetupCacheKeyPrefix(rep, file_size); unique_ptr new_table(new BlockBasedTable(rep)); @@ -1092,7 +1100,11 @@ InternalIterator* BlockBasedTable::NewIndexIterator( } else { // Create index reader and put it in the cache. Status s; + TEST_SYNC_POINT("BlockBasedTable::NewIndexIterator::thread2:2"); s = CreateIndexReader(&index_reader); + TEST_SYNC_POINT("BlockBasedTable::NewIndexIterator::thread1:1"); + TEST_SYNC_POINT("BlockBasedTable::NewIndexIterator::thread2:3"); + TEST_SYNC_POINT("BlockBasedTable::NewIndexIterator::thread1:4"); if (s.ok()) { assert(index_reader != nullptr); s = block_cache->Insert( @@ -1662,10 +1674,6 @@ Status BlockBasedTable::CreateIndexReader( meta_index_iter = meta_iter_guard.get(); } - // We need to wrap data with internal_prefix_transform to make sure it can - // handle prefix correctly. - rep_->internal_prefix_transform.reset( - new InternalKeySliceTransform(rep_->ioptions.prefix_extractor)); return HashIndexReader::Create( rep_->internal_prefix_transform.get(), footer, file, rep_->ioptions, comparator, footer.index_handle(), meta_index_iter, index_reader, diff --git a/table/table_test.cc b/table/table_test.cc index 91377b2e7..a5efca69a 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -45,6 +45,7 @@ #include "util/random.h" #include "util/statistics.h" #include "util/string_util.h" +#include "util/sync_point.h" #include "util/testharness.h" #include "util/testutil.h" #include "utilities/merge_operators.h" @@ -2090,6 +2091,65 @@ TEST_F(BlockBasedTableTest, BlockCacheLeak) { c.ResetTableReader(); } +TEST_F(BlockBasedTableTest, NewIndexIteratorLeak) { + // A regression test to avoid data race described in + // https://github.com/facebook/rocksdb/issues/1267 + TableConstructor c(BytewiseComparator(), true /* convert_to_internal_key_ */); + std::vector keys; + stl_wrappers::KVMap kvmap; + c.Add("a1", "val1"); + Options options; + options.prefix_extractor.reset(NewFixedPrefixTransform(1)); + BlockBasedTableOptions table_options; + table_options.index_type = BlockBasedTableOptions::kHashSearch; + table_options.cache_index_and_filter_blocks = true; + table_options.block_cache = NewLRUCache(0); + options.table_factory.reset(NewBlockBasedTableFactory(table_options)); + const ImmutableCFOptions ioptions(options); + c.Finish(options, ioptions, table_options, + GetPlainInternalComparator(options.comparator), &keys, &kvmap); + + rocksdb::SyncPoint::GetInstance()->LoadDependencyAndMarkers( + { + {"BlockBasedTable::NewIndexIterator::thread1:1", + "BlockBasedTable::NewIndexIterator::thread2:2"}, + {"BlockBasedTable::NewIndexIterator::thread2:3", + "BlockBasedTable::NewIndexIterator::thread1:4"}, + }, + { + {"BlockBasedTableTest::NewIndexIteratorLeak:Thread1Marker", + "BlockBasedTable::NewIndexIterator::thread1:1"}, + {"BlockBasedTableTest::NewIndexIteratorLeak:Thread1Marker", + "BlockBasedTable::NewIndexIterator::thread1:4"}, + {"BlockBasedTableTest::NewIndexIteratorLeak:Thread2Marker", + "BlockBasedTable::NewIndexIterator::thread2:2"}, + {"BlockBasedTableTest::NewIndexIteratorLeak:Thread2Marker", + "BlockBasedTable::NewIndexIterator::thread2:3"}, + }); + + rocksdb::SyncPoint::GetInstance()->EnableProcessing(); + ReadOptions ro; + auto* reader = c.GetTableReader(); + + std::function func1 = [&]() { + TEST_SYNC_POINT("BlockBasedTableTest::NewIndexIteratorLeak:Thread1Marker"); + std::unique_ptr iter(reader->NewIterator(ro)); + iter->Seek(InternalKey("a1", 0, kTypeValue).Encode()); + }; + + std::function func2 = [&]() { + TEST_SYNC_POINT("BlockBasedTableTest::NewIndexIteratorLeak:Thread2Marker"); + std::unique_ptr iter(reader->NewIterator(ro)); + }; + + auto thread1 = std::thread(func1); + auto thread2 = std::thread(func2); + thread1.join(); + thread2.join(); + rocksdb::SyncPoint::GetInstance()->DisableProcessing(); + c.ResetTableReader(); +} + // Plain table is not supported in ROCKSDB_LITE #ifndef ROCKSDB_LITE TEST_F(PlainTableTest, BasicPlainTableProperties) { -- GitLab