Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix data race in SetCurrentConnection #440

Merged
merged 2 commits into from
Dec 31, 2021
Merged

Fix data race in SetCurrentConnection #440

merged 2 commits into from
Dec 31, 2021

Conversation

calvinxiao
Copy link
Contributor

I found one data race using Thread Sanitizer.

SetCurrentConnection was introduced by #363 , and it can only be used with exclusive commands.

Fixed by moving svr_->SetCurrentConnection(this); after the ExclusivityGuard is acquired.

Verified with these benchmark commands:

  • redis-benchmark -p 6666 -r 10000 -n 10000 eval 'return redis.call("ping")' 0
  • redis-benchmark -p 6666

Here is the Thread Sanitizer output:

./src/kvrocks -c ./kvrocks.conf
Version: 999.999.999 @fc00bf69
==================
WARNING: ThreadSanitizer: data race (pid=299262)
  Write of size 8 at 0x7b6800000140 by thread T26:
    #0 Server::SetCurrentConnection(Redis::Connection*) /home/calvin/github/kvrocks/src/./server.h:140:73 (kvrocks+0x71a801)
    #1 Redis::Connection::ExecuteCommands(std::vector<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> > > >, std::allocator<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/calvin/github/kvrocks/src/redis_connection.cc:295:9 (kvrocks+0x71a801)
    #2 Redis::Connection::OnRead(bufferevent*, void*) /home/calvin/github/kvrocks/src/redis_connection.cc:68:9 (kvrocks+0x71a1cf)
    #3 bufferevent_run_deferred_callbacks_unlocked /home/calvin/github/kvrocks/external/libevent/bufferevent.c:208:3 (kvrocks+0x7d4aed)
    #4 WorkerThread::Start()::$_0::operator()() const /home/calvin/github/kvrocks/src/worker.cc:332:22 (kvrocks+0x7aba65)
    #5 void std::__invoke_impl<void, WorkerThread::Start()::$_0>(std::__invoke_other, WorkerThread::Start()::$_0&&) /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/invoke.h:61:14 (kvrocks+0x7aba65)
    #6 std::__invoke_result<WorkerThread::Start()::$_0>::type std::__invoke<WorkerThread::Start()::$_0>(WorkerThread::Start()::$_0&&) /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/invoke.h:96:14 (kvrocks+0x7aba65)
    #7 void std::thread::_Invoker<std::tuple<WorkerThread::Start()::$_0> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/std_thread.h:253:13 (kvrocks+0x7aba65)
    #8 std::thread::_Invoker<std::tuple<WorkerThread::Start()::$_0> >::operator()() /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/std_thread.h:260:11 (kvrocks+0x7aba65)
    #9 std::thread::_State_impl<std::thread::_Invoker<std::tuple<WorkerThread::Start()::$_0> > >::_M_run() /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/std_thread.h:211:13 (kvrocks+0x7aba65)
    #10 execute_native_thread_routine /usr/src/debug/gcc-11.2.1-1.fc34.x86_64/obj-x86_64-redhat-linux/x86_64-redhat-linux/libstdc++-v3/src/c++11/../../../../../libstdc++-v3/src/c++11/thread.cc:82:18 (libstdc++.so.6+0xd9c83)

  Previous write of size 8 at 0x7b6800000140 by thread T23:
    #0 Server::SetCurrentConnection(Redis::Connection*) /home/calvin/github/kvrocks/src/./server.h:140:73 (kvrocks+0x71a801)
    #1 Redis::Connection::ExecuteCommands(std::vector<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> > > >, std::allocator<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/calvin/github/kvrocks/src/redis_connection.cc:295:9 (kvrocks+0x71a801)
    #2 Redis::Connection::OnRead(bufferevent*, void*) /home/calvin/github/kvrocks/src/redis_connection.cc:68:9 (kvrocks+0x71a1cf)
    #3 bufferevent_run_deferred_callbacks_unlocked /home/calvin/github/kvrocks/external/libevent/bufferevent.c:208:3 (kvrocks+0x7d4aed)
    #4 WorkerThread::Start()::$_0::operator()() const /home/calvin/github/kvrocks/src/worker.cc:332:22 (kvrocks+0x7aba65)
    #5 void std::__invoke_impl<void, WorkerThread::Start()::$_0>(std::__invoke_other, WorkerThread::Start()::$_0&&) /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/invoke.h:61:14 (kvrocks+0x7aba65)
    #6 std::__invoke_result<WorkerThread::Start()::$_0>::type std::__invoke<WorkerThread::Start()::$_0>(WorkerThread::Start()::$_0&&) /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/invoke.h:96:14 (kvrocks+0x7aba65)
    #7 void std::thread::_Invoker<std::tuple<WorkerThread::Start()::$_0> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/std_thread.h:253:13 (kvrocks+0x7aba65)
    #8 std::thread::_Invoker<std::tuple<WorkerThread::Start()::$_0> >::operator()() /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/std_thread.h:260:11 (kvrocks+0x7aba65)
    #9 std::thread::_State_impl<std::thread::_Invoker<std::tuple<WorkerThread::Start()::$_0> > >::_M_run() /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/std_thread.h:211:13 (kvrocks+0x7aba65)
    #10 execute_native_thread_routine /usr/src/debug/gcc-11.2.1-1.fc34.x86_64/obj-x86_64-redhat-linux/x86_64-redhat-linux/libstdc++-v3/src/c++11/../../../../../libstdc++-v3/src/c++11/thread.cc:82:18 (libstdc++.so.6+0xd9c83)

  Location is heap block of size 1360 at 0x7b6800000000 allocated by main thread:
    #0 operator new(unsigned long) <null> (kvrocks+0x66f61c)
    #1 main /home/calvin/github/kvrocks/src/main.cc:318:9 (kvrocks+0x7bbb61)

  Thread T26 'worker' (tid=299351, running) created by main thread at:
    #0 pthread_create <null> (kvrocks+0x6283f1)
    #1 <null> /usr/src/debug/gcc-11.2.1-1.fc34.x86_64/obj-x86_64-redhat-linux/x86_64-redhat-linux/libstdc++-v3/include/x86_64-redhat-linux/bits/gthr-default.h:663:35 (libstdc++.so.6+0xd9f39)
    #2 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) /usr/src/debug/gcc-11.2.1-1.fc34.x86_64/obj-x86_64-redhat-linux/x86_64-redhat-linux/libstdc++-v3/src/c++11/../../../../../libstdc++-v3/src/c++11/thread.cc:147:37 (libstdc++.so.6+0xd9f39)
    #3 Server::Start() /home/calvin/github/kvrocks/src/server.cc:77:13 (kvrocks+0x76e38f)
    #4 main /home/calvin/github/kvrocks/src/main.cc:325:8 (kvrocks+0x7bbc11)

  Thread T23 'worker' (tid=299348, running) created by main thread at:
    #0 pthread_create <null> (kvrocks+0x6283f1)
    #1 <null> /usr/src/debug/gcc-11.2.1-1.fc34.x86_64/obj-x86_64-redhat-linux/x86_64-redhat-linux/libstdc++-v3/include/x86_64-redhat-linux/bits/gthr-default.h:663:35 (libstdc++.so.6+0xd9f39)
    #2 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) /usr/src/debug/gcc-11.2.1-1.fc34.x86_64/obj-x86_64-redhat-linux/x86_64-redhat-linux/libstdc++-v3/src/c++11/../../../../../libstdc++-v3/src/c++11/thread.cc:147:37 (libstdc++.so.6+0xd9f39)
    #3 Server::Start() /home/calvin/github/kvrocks/src/server.cc:77:13 (kvrocks+0x76e38f)
    #4 main /home/calvin/github/kvrocks/src/main.cc:325:8 (kvrocks+0x7bbc11)

git-hulk
git-hulk previously approved these changes Dec 30, 2021
View reviewed changes
Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks calvin.

@git-hulk git-hulk requested a review from ShooterIT December 30, 2021 23:29
ShooterIT
ShooterIT previously approved these changes Dec 31, 2021
View reviewed changes
Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @calvinxiao , cool job. I add some comments, please have a look.

@git-hulk We should add memory and thread sanitizer to CI ASAP.

src/redis_connection.cc Show resolved Hide resolved
@git-hulk
Copy link
Member

Thanks @calvinxiao , cool job. I add some comments, please have a look.

@git-hulk We should add memory and thread sanitizer to CI ASAP.

yeah

@calvinxiao calvinxiao dismissed stale reviews from ShooterIT and git-hulk via a59b146 December 31, 2021 05:49
calvinxiao and others added 2 commits December 31, 2021 13:50
@calvinxiao
Copy link
Contributor Author

Rebased, tests should pass.

@ShooterIT ShooterIT merged commit e83f143 into apache:unstable Dec 31, 2021
ShooterIT pushed a commit to ShooterIT/kvrocks that referenced this pull request Jan 27, 2022
@calvinxiao @ShooterIT
Fix data race in SetCurrentConnection (apache#440)
08c3e0b
@ShooterIT ShooterIT mentioned this pull request Jan 27, 2022
Merged
ShooterIT pushed a commit that referenced this pull request Jan 28, 2022
@calvinxiao @ShooterIT
Fix data race in SetCurrentConnection (#440)
131d2ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShooterIT ShooterIT ShooterIT approved these changes

@git-hulk git-hulk git-hulk left review comments

Assignees
No one assigned
Labels
bug type bug release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@calvinxiao @git-hulk @ShooterIT