search

basho / leveldb

Clone of http://code.google.com/p/leveldb/
BSD 3-Clause "New" or "Revised" License
408 stars 182 forks source link

Mv expiry iter bug #203

Closed matthewvon closed 8 years ago

matthewvon commented 8 years ago

DBImpl::NewIterator() was not setting expiry module when creating iterator object.

A detail discussion of this fix is found here:

https://github.com/basho/leveldb/wiki/mv-exiry-iter-bug

erikleitch commented 8 years ago

These changes look ok to me (I have not previously reviewed all of the expiry code, and do have profess to have gone through all of it here).

Note that class ExpiryDBTester appears to leak m_Expiry in the destructor. Probably not important in a unit testing context, unless it is ever used in a large loop!

matthewvon commented 8 years ago

Erik's observation is quite good. However there is an underlying "trick" in play that is not obvious (and know that I just now verified via valgrind that the next statement is true). The "local" pointer to the expiry module, m_Expiry, does not own the object. The Options object owns the expiry object. It uses a smart pointer to automatically delete the object once the database closes.

But seriously, good eyes on that one. I will add a comment to the code in explanation for the future.

matthewvon commented 8 years ago

mmaszewski@puma:~/leveldb$ valgrind --leak-check=summary --show-reachable=yes ./expiry_os_test ==31535== Memcheck, a memory error detector ==31535== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al. ==31535== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info ==31535== Command: ./expiry_os_test ==31535== ==== Test ExpiryTester.Defaults ==== Test ExpiryTester.MemTableInserterCallback ==== Test ExpiryTester.MemTableCallback ==== Test ExpiryTester.CompactionFinalizeCallback1 ==== Test ExpiryTester.OverlapTests ==== Test ExpiryManifestTester.Manifest1 ==== Test ExpiryManifestTester.Overlap1 ==== Test ExpiryManifestTester.Overlap2 ==== Test ExpiryManifestTester.Compact1 ==== Test ExpiryDBTester.Simple ==== PASSED 10 tests ==31535== ==31535== HEAP SUMMARY: ==31535== in use at exit: 0 bytes in 0 blocks ==31535== total heap usage: 8,238 allocs, 8,238 frees, 9,884,562 bytes allocated ==31535== ==31535== All heap blocks were freed -- no leaks are possible ==31535== ==31535== For counts of detected and suppressed errors, rerun with: -v ==31535== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)

paulplace commented 8 years ago

+1 eb43eb5