andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix valgrind issues in memtable_list_test

Browse Source 
Summary: Need to remember to unref MemTableList->current() before deleting.

Test Plan: ran test with valgrind

Reviewers: igor

Reviewed By: igor

Subscribers: dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D36855
This commit is contained in:
agiardullo 2015-04-10 14:16:03 -07:00
parent 697380f3d7
commit 753dd1fdd0
2 changed files with 15 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
db/memtable_list.h
View File

@ -97,6 +97,9 @@ class MemTableList {
flush_requested_(false) { flush_requested_(false) {
current_->Ref(); current_->Ref();
} }
// Should not delete MemTableList without making sure MemTableList::current()
// is Unref()'d.
~MemTableList() {} ~MemTableList() {}
MemTableListVersion* current() { return current_; } MemTableListVersion* current() { return current_; }

16
db/memtable_list_test.cc
View File

@ -101,6 +101,10 @@ TEST_F(MemTableListTest, Empty) {
autovector<MemTable*> mems; autovector<MemTable*> mems;
list.PickMemtablesToFlush(&mems); list.PickMemtablesToFlush(&mems);
ASSERT_EQ(0, mems.size()); ASSERT_EQ(0, mems.size());
autovector<MemTable*> to_delete;
list.current()->Unref(&to_delete);
ASSERT_EQ(0, to_delete.size());
} }
TEST_F(MemTableListTest, GetTest) { TEST_F(MemTableListTest, GetTest) {
@ -372,6 +376,7 @@ TEST_F(MemTableListTest, FlushPendingTest) {
ASSERT_EQ(m, m->Unref()); ASSERT_EQ(m, m->Unref());
delete m; delete m;
} }
to_delete.clear();
// Request a flush again. Should be nothing to flush // Request a flush again. Should be nothing to flush
list.FlushRequested(); list.FlushRequested();
@ -379,23 +384,26 @@ TEST_F(MemTableListTest, FlushPendingTest) {
ASSERT_FALSE(list.imm_flush_needed.load(std::memory_order_acquire)); ASSERT_FALSE(list.imm_flush_needed.load(std::memory_order_acquire));
// Flush the 1 memtable that was picked in to_flush2 // Flush the 1 memtable that was picked in to_flush2
autovector<MemTable*> to_delete2;
s = MemTableListTest::Mock_InstallMemtableFlushResults( s = MemTableListTest::Mock_InstallMemtableFlushResults(
&list, MutableCFOptions(options, ioptions), to_flush2, &to_delete2); &list, MutableCFOptions(options, ioptions), to_flush2, &to_delete);
ASSERT_OK(s); ASSERT_OK(s);
// This will actually intall 2 tables. The 1 we told it to flush, and also // This will actually intall 2 tables. The 1 we told it to flush, and also
// tables[4] which has been waiting for tables[3] to commit. // tables[4] which has been waiting for tables[3] to commit.
ASSERT_EQ(2, to_delete2.size()); ASSERT_EQ(2, to_delete.size());
ASSERT_EQ(0, list.size()); ASSERT_EQ(0, list.size());
for (const auto& m : to_delete2) { for (const auto& m : to_delete) {
// Refcount should be 0 after calling InstallMemtableFlushResults. // Refcount should be 0 after calling InstallMemtableFlushResults.
// Verify this, by Ref'ing then UnRef'ing: // Verify this, by Ref'ing then UnRef'ing:
m->Ref(); m->Ref();
ASSERT_EQ(m, m->Unref()); ASSERT_EQ(m, m->Unref());
delete m; delete m;
} }
to_delete.clear();
list.current()->Unref(&to_delete);
ASSERT_EQ(0, to_delete.size());
} }
} // namespace rocksdb } // namespace rocksdb