Fix data race for accessing database in some commands #253

calvinxiao · 2021-05-08T10:22:22Z

Describe this PR

Found several data races with -fsanitize=thread flags

1. Data race when starting up the server

Output:

==================
WARNING: ThreadSanitizer: data race (pid=11804)
  Read of size 8 at 0x7b0400004900 by thread T16:
    #0 operator() /home/eagle/github/my/kvrocks/src/worker.cc:362 (kvrocks+0x222b68)
    #1 __invoke_impl<void, WorkerThread::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:60 (kvrocks+0x222b68)
    #2 __invoke<WorkerThread::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:95 (kvrocks+0x222b68)
    #3 _M_invoke<0> /usr/include/c++/9/thread:244 (kvrocks+0x222b68)
    #4 operator() /usr/include/c++/9/thread:251 (kvrocks+0x222b68)
    #5 _M_run /usr/include/c++/9/thread:195 (kvrocks+0x222b68)
    #6 <null> <null> (libstdc++.so.6+0xd6d83)

  Previous write of size 8 at 0x7b0400004900 by main thread:
    #0 std::enable_if<std::__and_<std::__not_<std::__is_tuple_like<std::thread::id> >, std::is_move_constructible<std::thread::id>, std::is_move_assignable<std::thread::id> >::value, void>::type std::swap<std::thread::id>(std::thread::id&, std::thread::id&) /usr/include/c++/9/bits/move.h:194 (kvrocks+0x223742)
    #1 std::thread::swap(std::thread&) /usr/include/c++/9/thread:159 (kvrocks+0x223742)
    #2 std::thread::operator=(std::thread&&) /usr/include/c++/9/thread:153 (kvrocks+0x223742)
    #3 WorkerThread::Start() /home/eagle/github/my/kvrocks/src/worker.cc:363 (kvrocks+0x223742)
    #4 Server::Start() /home/eagle/github/my/kvrocks/src/server.cc:60 (kvrocks+0x1e9427)
    #5 __libc_start_main <null> (libc.so.6+0x270b2)

  Location is heap block of size 16 at 0x7b0400004900 allocated by main thread:
    #0 operator new(unsigned long) <null> (libtsan.so.0+0x8c012)
    #1 Server::Server(Engine::Storage*, Config*) /home/eagle/github/my/kvrocks/src/server.cc:35 (kvrocks+0x1ed2dd)
    #2 main /home/eagle/github/my/kvrocks/src/main.cc:310 (kvrocks+0x9471b)

  Thread T16 'worker' (tid=11882, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x5ea99)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 Server::Start() /home/eagle/github/my/kvrocks/src/server.cc:60 (kvrocks+0x1e9427)
    #3 main /home/eagle/github/my/kvrocks/src/main.cc:317 (kvrocks+0x94839)

SUMMARY: ThreadSanitizer: data race /home/eagle/github/my/kvrocks/src/worker.cc:362 in operator()
==================

2. Thread Sanitizer warning in `make test`

==================
WARNING: ThreadSanitizer: data race (pid=12120)
  Read of size 1 at 0x7b7c00009031 by thread T1:
    #0 memcmp <null> (libtsan.so.0+0x651af)
    #1 rocksdb::Slice::compare(rocksdb::Slice const&) const include/rocksdb/slice.h:250 (unittest+0x56987d)
    #2 Compare util/comparator.cc:27 (unittest+0x56987d)
    #3 Engine::SubKeyFilter::Filter(int, rocksdb::Slice const&, rocksdb::Slice const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const /home/eagle/github/my/kvrocks/src/compact_filter.cc:94 (unittest+0xdb539)
    #4 rocksdb::CompactionFilter::FilterV2(int, rocksdb::Slice const&, rocksdb::CompactionFilter::ValueType, rocksdb::Slice const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) const include/rocksdb/compaction_filter.h:167 (unittest+0x5c4d32)
    #5 rocksdb::CompactionIterator::InvokeFilterIfNeeded(bool*, rocksdb::Slice*) db/compaction/compaction_iterator.cc:273 (unittest+0x5c4d32)

  Previous write of size 1 at 0x7b7c00009031 by main thread (mutexes: write M668075411208911328):
    #0 memcpy <null> (libtsan.so.0+0x42333)
    #1 memcpy /usr/include/x86_64-linux-gnu/bits/string_fortified.h:34 (unittest+0x3c9539)
    #2 rocksdb::MemTable::Add(unsigned long, rocksdb::ValueType, rocksdb::Slice const&, rocksdb::Slice const&, rocksdb::ProtectionInfoKVOTS<unsigned long> const*, bool, rocksdb::MemTablePostProcessInfo*, void**) db/memtable.cc:555 (unittest+0x3c9539)
    #3 Redis::Set::Overwrite(rocksdb::Slice, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /home/eagle/github/my/kvrocks/src/redis_set.cc:31 (unittest+0x1c0a0b)
    #4 RedisSetTest_Overwrite_Test::TestBody() ../tests/t_set_test.cc:156 (unittest+0x2931b6)
    #5 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) googletest/src/gtest.cc:2433 (unittest+0x6b99a0)
    #6 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) googletest/src/gtest.cc:2469 (unittest+0x6b99a0)
    #7 __libc_start_main <null> (libc.so.6+0x270b2)

  Location is heap block of size 3216 at 0x7b7c00008c00 allocated by main thread:
    #0 operator new(unsigned long) <null> (libtsan.so.0+0x8c012)
    #1 rocksdb::ColumnFamilyData::ConstructNewMemtable(rocksdb::MutableCFOptions const&, unsigned long) db/column_family.cc:1057 (unittest+0x5b30b1)
    #2 Engine::Storage::Open() /home/eagle/github/my/kvrocks/src/storage.cc:227 (unittest+0x222707)
    #3 TestBase::TestBase() ../tests/test_base.h:15 (unittest+0x2958a8)
    #4 RedisSetTest::RedisSetTest() ../tests/t_set_test.cc:7 (unittest+0x2958a8)
    #5 RedisSetTest_Overwrite_Test::RedisSetTest_Overwrite_Test() ../tests/t_set_test.cc:152 (unittest+0x2961a9)
    #6 testing::internal::TestFactoryImpl<RedisSetTest_Overwrite_Test>::CreateTest() /usr/include/gtest/internal/gtest-internal.h:460 (unittest+0x2961a9)
    #7 testing::Test* testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test*>(testing::internal::TestFactoryBase*, testing::Test* (testing::internal::TestFactoryBase::*)(), char const*) googletest/src/gtest.cc:2433 (unittest+0x6b9b70)
    #8 testing::Test* testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test*>(testing::internal::TestFactoryBase*, testing::Test* (testing::internal::TestFactoryBase::*)(), char const*) googletest/src/gtest.cc:2469 (unittest+0x6b9b70)
    #9 __libc_start_main <null> (libc.so.6+0x270b2)

  Mutex M668075411208911328 is already destroyed.

  Thread T1 'rocksdb:low0' (tid=12122, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x5ea99)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 Engine::Storage::Open(bool) /home/eagle/github/my/kvrocks/src/storage.cc:165 (unittest+0x2207af)
    #3 Engine::Storage::Open() /home/eagle/github/my/kvrocks/src/storage.cc:227 (unittest+0x222707)
    #4 TestBase::TestBase() ../tests/test_base.h:15 (unittest+0x25bbbd)
    #5 RedisTypeTest::RedisTypeTest() ../tests/t_metadata_test.cc:48 (unittest+0x25bbbd)
    #6 RedisTypeTest_GetMetadata_Test::RedisTypeTest_GetMetadata_Test() ../tests/t_metadata_test.cc:64 (unittest+0x25c249)
    #7 testing::internal::TestFactoryImpl<RedisTypeTest_GetMetadata_Test>::CreateTest() /usr/include/gtest/internal/gtest-internal.h:460 (unittest+0x25c249)
    #8 testing::Test* testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test*>(testing::internal::TestFactoryBase*, testing::Test* (testing::internal::TestFactoryBase::*)(), char const*) googletest/src/gtest.cc:2433 (unittest+0x6b9b70)
    #9 testing::Test* testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test*>(testing::internal::TestFactoryBase*, testing::Test* (testing::internal::TestFactoryBase::*)(), char const*) googletest/src/gtest.cc:2469 (unittest+0x6b9b70)
    #10 __libc_start_main <null> (libc.so.6+0x270b2)

SUMMARY: ThreadSanitizer: data race (/lib/x86_64-linux-gnu/libtsan.so.0+0x651af) in memcmp
==================

3. Thread Sanitizer warning when benchmarking

Benchmark command redis-benchmark -p 6666 -n 100000 -q

Full output

Part of the output:

==================
WARNING: ThreadSanitizer: data race (pid=20423)
  Write of size 8 at 0x7f1282ce71c0 by thread T19:
    #0 localtime <null> (libtsan.so.0+0x4701c)
    #1 operator() /home/eagle/github/kvrocks/src/server.cc:78 (kvrocks+0x15d669)
    #2 __invoke_impl<void, Server::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:60 (kvrocks+0x15d669)
    #3 __invoke<Server::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:95 (kvrocks+0x15d669)
    #4 _M_invoke<0> /usr/include/c++/9/thread:244 (kvrocks+0x15d669)
    #5 operator() /usr/include/c++/9/thread:251 (kvrocks+0x15d669)
    #6 _M_run /usr/include/c++/9/thread:195 (kvrocks+0x15d669)
    #7 <null> <null> (libstdc++.so.6+0xd6d83)

  Previous write of size 8 at 0x7f1282ce71c0 by thread T18:
    #0 localtime <null> (libtsan.so.0+0x4701c)
    #1 Server::cron() /home/eagle/github/kvrocks/src/server.cc:463 (kvrocks+0x15ea8a)
    #2 operator() /home/eagle/github/kvrocks/src/server.cc:66 (kvrocks+0x15f3b4)
    #3 __invoke_impl<void, Server::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:60 (kvrocks+0x15f3b4)
    #4 __invoke<Server::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:95 (kvrocks+0x15f3b4)
    #5 _M_invoke<0> /usr/include/c++/9/thread:244 (kvrocks+0x15f3b4)
    #6 operator() /usr/include/c++/9/thread:251 (kvrocks+0x15f3b4)
    #7 _M_run /usr/include/c++/9/thread:195 (kvrocks+0x15f3b4)
    #8 <null> <null> (libstdc++.so.6+0xd6d83)

  As if synchronized via sleep:
    #0 usleep <null> (libtsan.so.0+0x62f5b)
    #1 operator() /home/eagle/github/kvrocks/src/server.cc:94 (kvrocks+0x15d719)
    #2 __invoke_impl<void, Server::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:60 (kvrocks+0x15d719)
    #3 __invoke<Server::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:95 (kvrocks+0x15d719)
    #4 _M_invoke<0> /usr/include/c++/9/thread:244 (kvrocks+0x15d719)
    #5 operator() /usr/include/c++/9/thread:251 (kvrocks+0x15d719)
    #6 _M_run /usr/include/c++/9/thread:195 (kvrocks+0x15d719)
    #7 <null> <null> (libstdc++.so.6+0xd6d83)

  Location is global '<null>' at 0x000000000000 (libc.so.6+0x0000001f11c0)

  Thread T19 'compaction-checker' (tid=20504, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x5ea99)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 main /home/eagle/github/kvrocks/src/main.cc:317 (kvrocks+0x8b0d9)

  Thread T18 'server-cron' (tid=20503, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x5ea99)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 main /home/eagle/github/kvrocks/src/main.cc:317 (kvrocks+0x8b0d9)

SUMMARY: ThreadSanitizer: data race (/lib/x86_64-linux-gnu/libtsan.so.0+0x4701c) in localtime
==================
==================
WARNING: ThreadSanitizer: data race (pid=14056)
  Read of size 1 at 0x7b2400014ebc by thread T9:
    #0 memcpy <null> (libtsan.so.0+0x42333)
    #1 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_mutate(unsigned long, unsigned long, char const*, unsigned long) <null> (libstdc++.so.6+0x142ec3)
    #2 Redis::Set::Take(rocksdb::Slice const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >*, int, bool) /home/eagle/github/my/kvrocks/src/redis_set.cc:201 (kvrocks+0x19d8ea)
    #3 Redis::CommandSPop::Execute(Server*, Redis::Connection*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) /home/eagle/github/my/kvrocks/src/redis_cmd.cc:1822 (kvrocks+0x14ffb9)
    #4 Redis::Request::ExecuteCommands(Redis::Connection*) /home/eagle/github/my/kvrocks/src/redis_request.cc:211 (kvrocks+0x195d1d)
    #5 Redis::Connection::OnRead(bufferevent*, void*) /home/eagle/github/my/kvrocks/src/redis_connection.cc:64 (kvrocks+0x16a3f9)
    #6 bufferevent_run_deferred_callbacks_unlocked /home/eagle/github/my/kvrocks/external/libevent/bufferevent.c:208 (kvrocks+0x250401)
    #7 operator() /home/eagle/github/my/kvrocks/src/worker.cc:362 (kvrocks+0x222b73)
    #8 __invoke_impl<void, WorkerThread::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:60 (kvrocks+0x222b73)
    #9 __invoke<WorkerThread::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:95 (kvrocks+0x222b73)
    #10 _M_invoke<0> /usr/include/c++/9/thread:244 (kvrocks+0x222b73)
    #11 operator() /usr/include/c++/9/thread:251 (kvrocks+0x222b73)
    #12 _M_run /usr/include/c++/9/thread:195 (kvrocks+0x222b73)
    #13 <null> <null> (libstdc++.so.6+0xd6d83)

  Previous write of size 8 at 0x7b2400014eb8 by thread T13:
    #0 operator new(unsigned long) <null> (libtsan.so.0+0x8c012)
    #1 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_mutate(unsigned long, unsigned long, char const*, unsigned long) <null> (libstdc++.so.6+0x142e7d)
    #2 Redis::CommandSPop::Execute(Server*, Redis::Connection*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) /home/eagle/github/my/kvrocks/src/redis_cmd.cc:1822 (kvrocks+0x14ffb9)
    #3 Redis::Request::ExecuteCommands(Redis::Connection*) /home/eagle/github/my/kvrocks/src/redis_request.cc:211 (kvrocks+0x195d1d)
    #4 Redis::Connection::OnRead(bufferevent*, void*) /home/eagle/github/my/kvrocks/src/redis_connection.cc:64 (kvrocks+0x16a3f9)
    #5 bufferevent_run_deferred_callbacks_unlocked /home/eagle/github/my/kvrocks/external/libevent/bufferevent.c:208 (kvrocks+0x250401)
    #6 operator() /home/eagle/github/my/kvrocks/src/worker.cc:362 (kvrocks+0x222b73)
    #7 __invoke_impl<void, WorkerThread::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:60 (kvrocks+0x222b73)
    #8 __invoke<WorkerThread::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:95 (kvrocks+0x222b73)
    #9 _M_invoke<0> /usr/include/c++/9/thread:244 (kvrocks+0x222b73)
    #10 operator() /usr/include/c++/9/thread:251 (kvrocks+0x222b73)
    #11 _M_run /usr/include/c++/9/thread:195 (kvrocks+0x222b73)
    #12 <null> <null> (libstdc++.so.6+0xd6d83)

  Location is heap block of size 133 at 0x7b2400014eb0 allocated by thread T13:
    #0 operator new(unsigned long) <null> (libtsan.so.0+0x8c012)
    #1 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_mutate(unsigned long, unsigned long, char const*, unsigned long) <null> (libstdc++.so.6+0x142e7d)
    #2 Redis::CommandSPop::Execute(Server*, Redis::Connection*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) /home/eagle/github/my/kvrocks/src/redis_cmd.cc:1822 (kvrocks+0x14ffb9)
    #3 Redis::Request::ExecuteCommands(Redis::Connection*) /home/eagle/github/my/kvrocks/src/redis_request.cc:211 (kvrocks+0x195d1d)
    #4 Redis::Connection::OnRead(bufferevent*, void*) /home/eagle/github/my/kvrocks/src/redis_connection.cc:64 (kvrocks+0x16a3f9)
    #5 bufferevent_run_deferred_callbacks_unlocked /home/eagle/github/my/kvrocks/external/libevent/bufferevent.c:208 (kvrocks+0x250401)
    #6 operator() /home/eagle/github/my/kvrocks/src/worker.cc:362 (kvrocks+0x222b73)
    #7 __invoke_impl<void, WorkerThread::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:60 (kvrocks+0x222b73)
    #8 __invoke<WorkerThread::Start()::<lambda()> > /usr/include/c++/9/bits/invoke.h:95 (kvrocks+0x222b73)
    #9 _M_invoke<0> /usr/include/c++/9/thread:244 (kvrocks+0x222b73)
    #10 operator() /usr/include/c++/9/thread:251 (kvrocks+0x222b73)
    #11 _M_run /usr/include/c++/9/thread:195 (kvrocks+0x222b73)
    #12 <null> <null> (libstdc++.so.6+0xd6d83)

  Thread T9 'worker' (tid=14127, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x5ea99)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 Server::Start() /home/eagle/github/my/kvrocks/src/server.cc:60 (kvrocks+0x1e9427)
    #3 main /home/eagle/github/my/kvrocks/src/main.cc:317 (kvrocks+0x94839)

  Thread T13 'worker' (tid=14131, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x5ea99)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 Server::Start() /home/eagle/github/my/kvrocks/src/server.cc:60 (kvrocks+0x1e9427)
    #3 main /home/eagle/github/my/kvrocks/src/main.cc:317 (kvrocks+0x94839)

SUMMARY: ThreadSanitizer: data race (/lib/x86_64-linux-gnu/libtsan.so.0+0x42333) in memcpy
==================

Steps to reproduce

add -fsanitize=thread to compiler flags
g++ -v

Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/9/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none:hsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 9.3.0-17ubuntu1~20.04' --with-bugurl=file:///usr/share/doc/gcc-9/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++,gm2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-9 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-plugin --enable-default-pie --with-system-zlib --with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none=/build/gcc-9-HskZEa/gcc-9-9.3.0/debian/tmp-nvptx/usr,hsa --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04)

startup command ./src/kvrocks -c kvrocks.conf
test command make test
benchmark command redis-benchmark -p 6666 -n 100000 -q
platform: Windows 10 + WSL2

Questions:

I am not C++ expert, can we use localtime_r?
The destructor of LockGuard is called at the end of if block, is it standard behavior across all C++ versions?

calvinxiao · 2021-05-08T10:25:58Z

Result of this PR:

No more warning from tsan when starting up the server
No more warning from tsan when make test
All tests passed.
Only one warning left

==================
WARNING: ThreadSanitizer: signal-unsafe call inside of a signal (pid=24508)
    #0 operator new(unsigned long) <null> (libtsan.so.0+0x8c012)
    #1 google::LogMessage::Init(char const*, int, int, void (google::LogMessage::*)()) <null> (kvrocks+0x2359cb)
    #2 std::function<void ()>::operator()() const /usr/include/c++/9/bits/std_function.h:688 (kvrocks+0x23298d)
    #3 signal_handler /home/eagle/github/my/kvrocks/src/main.cc:38 (kvrocks+0x23298d)
    #4 <null> <null> (libtsan.so.0+0x2c4e3)
    #5 std::thread::join() <null> (libstdc++.so.6+0xd6fe6)
    #6 Server::Join() /home/eagle/github/my/kvrocks/src/server.cc:118 (kvrocks+0x1dd18f)
    #7 main /home/eagle/github/my/kvrocks/src/main.cc:318 (kvrocks+0x94882)

SUMMARY: ThreadSanitizer: signal-unsafe call inside of a signal (/lib/x86_64-linux-gnu/libtsan.so.0+0x8c012) in operator new(unsigned long)
==================

git-hulk · 2021-05-08T10:57:40Z

Good catch, many thanks to @calvinxiao. maybe we should integrate some sanitizers into CI to help us find out those issues.

git-hulk · 2021-05-08T11:00:44Z

I am not C++ expert, can we use localtime_r?

localtime_r was the same with localtime, except using the user storage buf

The destructor of LockGuard is called at the end of if block, is it standard behavior across all C++ versions?

It shouldn't use LockGuard here since it would release after exiting if block

src/redis_set.cc

calvinxiao · 2021-05-08T12:58:11Z

@git-hulk Looking at all the usage of LockGuard, I found ZSet::RangeByScore and ZSet::Range have the same conditional lock problem.

git-hulk · 2021-05-08T13:09:13Z

@git-hulk Looking at all the usage of LockGuard, I found ZSet::RangeByScore and ZSet::Range have the same conditional lock problem.

nice! also fixed that too

calvinxiao

nice! also fixed that too

I saw you fixed them in #256 , why not use unique_ptr?

src/redis_set.cc

git-hulk · 2021-05-08T13:26:09Z

nice! also fixed that too

I saw you fixed them in #256 , why not use unique_ptr?

We lock/unlock manually seems fine within few return branches

git-hulk · 2021-05-08T13:38:51Z

Both were ok for me, I’ll glad to close it if you fix those issues in this PR.

git-hulk

LGTM

ShooterIT · 2021-05-10T03:57:53Z

They are not only warnings of compiler but bugs

git-hulk · 2021-05-10T05:00:44Z

@Mergifyio backport 1.3

Before this commit, in spop, zremrangebylex, zremrangebyscore commands, the lock guard for accessing database may not work actually. Other changes - Use 'localtime_r' instead of 'localtime' - Use 'std::this_thread::get_id()' to get thread id (cherry picked from commit 1ac04ca)

mergify · 2021-05-10T05:01:27Z

Command backport 1.3: success

Backports have been created

#258 Fix data race for accessing database in some commands (backport #253) has been created for branch 1.3

Before this commit, in spop, zremrangebylex, zremrangebyscore commands, the lock guard for accessing the database may not work actually. Other changes - Use 'localtime_r' instead of 'localtime' - Use 'std::this_thread::get_id()' to get thread id (cherry picked from commit 1ac04ca) Co-authored-by: Calvin Xiao <calvin325@gmail.com>

Before this commit, in spop, zremrangebylex, zremrangebyscore commands, the lock guard for accessing database may not work actually. Other changes - Use 'localtime_r' instead of 'localtime' - Use 'std::this_thread::get_id()' to get thread id

calvinxiao added 2 commits May 8, 2021 16:27

Fix thread sanitizer warning when creating workers

2e82449

Fix data race using localtime_r and improper use of RAII

6151fe5

git-hulk requested review from karelrooted and ShooterIT May 8, 2021 10:55

git-hulk reviewed May 8, 2021

View reviewed changes

src/redis_set.cc Show resolved Hide resolved

calvinxiao added 2 commits May 8, 2021 20:52

Use smart pointer to optimize lock condition

33983b0

revert commit of Makefile

4fdc6c1

git-hulk added the enhancement type enhancement label May 8, 2021

calvinxiao commented May 8, 2021

View reviewed changes

src/redis_set.cc Show resolved Hide resolved

Include memory header according to cpplint

9598bcb

Fix conditional locking problem in zsetx

b7fb6a2

git-hulk approved these changes May 9, 2021

View reviewed changes

karelrooted approved these changes May 9, 2021

View reviewed changes

ShooterIT changed the title ~~Fix warning produced by thread sanitizer~~ Fix data race in some commands May 10, 2021

ShooterIT added bug type bug release notes labels May 10, 2021

ShooterIT changed the title ~~Fix data race in some commands~~ Fix data race for accessing database in some commands May 10, 2021

ShooterIT merged commit 1ac04ca into apache:unstable May 10, 2021

git-hulk added the need back patch label May 10, 2021

mergify bot mentioned this pull request May 10, 2021

Fix data race for accessing database in some commands (backport #253) #258

Merged

This was referenced May 18, 2021

Release 1.3.3 #267

Merged

Release 2.0.1 #268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix data race for accessing database in some commands #253

Fix data race for accessing database in some commands #253

calvinxiao commented May 8, 2021

calvinxiao commented May 8, 2021

git-hulk commented May 8, 2021

git-hulk commented May 8, 2021 •

edited

Loading

calvinxiao commented May 8, 2021 •

edited

Loading

git-hulk commented May 8, 2021

calvinxiao left a comment

git-hulk commented May 8, 2021 •

edited

Loading

git-hulk commented May 8, 2021

git-hulk left a comment

ShooterIT commented May 10, 2021

git-hulk commented May 10, 2021

mergify bot commented May 10, 2021

Fix data race for accessing database in some commands #253

Fix data race for accessing database in some commands #253

Conversation

calvinxiao commented May 8, 2021

Describe this PR

1. Data race when starting up the server

2. Thread Sanitizer warning in make test

3. Thread Sanitizer warning when benchmarking

Steps to reproduce

Questions:

calvinxiao commented May 8, 2021

git-hulk commented May 8, 2021

git-hulk commented May 8, 2021 • edited Loading

calvinxiao commented May 8, 2021 • edited Loading

git-hulk commented May 8, 2021

calvinxiao left a comment

Choose a reason for hiding this comment

git-hulk commented May 8, 2021 • edited Loading

git-hulk commented May 8, 2021

git-hulk left a comment

Choose a reason for hiding this comment

ShooterIT commented May 10, 2021

git-hulk commented May 10, 2021

mergify bot commented May 10, 2021

2. Thread Sanitizer warning in `make test`

git-hulk commented May 8, 2021 •

edited

Loading

calvinxiao commented May 8, 2021 •

edited

Loading

git-hulk commented May 8, 2021 •

edited

Loading