Skip to content

Fix benign race (detected by clang-12) in Keeper snapshots#24499

Merged
alesapin merged 2 commits intomasterfrom
fix_fake_race
May 26, 2021
Merged

Fix benign race (detected by clang-12) in Keeper snapshots#24499
alesapin merged 2 commits intomasterfrom
fix_fake_race

Conversation

@alesapin
Copy link
Copy Markdown
Member

@alesapin alesapin commented May 25, 2021 •
edited
Loading

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

For some reason, clang-11 cannot detect it. But it's not dangerous because we save snapshot size on snapshot creation.

==163==Launching Symbolizer process: /usr/bin/llvm-symbolizer-12 --inlines --default-arch=x86_64 
==================
WARNING: ThreadSanitizer: data race (pid=163)
  Read of size 8 at 0x7b3801a40d28 by thread T8 (mutexes: write M3395):
    #0 std::__1::__list_const_iterator<DB::ListNode<DB::KeeperStorage::Node>, void*>::operator++() obj-x86_64-linux-gnu/../contrib/libcxx/include/list:509:26 (clickhouse+0x146fee49)
    #1 DB::KeeperStorageSnapshot::serialize(DB::KeeperStorageSnapshot const&, DB::WriteBuffer&) obj-x86_64-linux-gnu/../src/Coordination/KeeperSnapshotManager.cpp:142:80 (clickhouse+0x146fee49)
    #2 DB::KeeperSnapshotManager::serializeSnapshotToBuffer(DB::KeeperStorageSnapshot const&) obj-x86_64-linux-gnu/../src/Coordination/KeeperSnapshotManager.cpp:324:5 (clickhouse+0x14702640)
    #3 DB::KeeperStateMachine::create_snapshot(nuraft::snapshot&, std::__1::function<void (bool&, std::__1::shared_ptr<std::exception>&)>&)::$_0::operator()(std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&) const obj-x86_64-linux-gnu/../src/Coordination/KeeperStateMachine.cpp:178:37 (clickhouse+0x146f708d)
    #4 decltype(std::__1::forward<DB::KeeperStateMachine::create_snapshot(nuraft::snapshot&, std::__1::function<void (bool&, std::__1::shared_ptr<std::exception>&)>&)::$_0&>(fp)(std::__1::forward<std::__1::shared_ptr<DB::KeeperStorageSnapshot> >(fp0))) std::__1::__invoke<DB::KeeperStateMachine::create_snapshot(nuraft::snapshot&, std::__1::function<void (bool&, std::__1::shared_ptr<std::exception>&)>&)::$_0&, std::__1::shared_ptr<DB::KeeperStorageSnapshot> >(DB::KeeperStateMachine::create_snapshot(nuraft::snapshot&, std::__1::function<void (bool&, std::__1::shared_ptr<std::exception>&)>&)::$_0&, std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&) obj-x86_64-linux-gnu/../contrib/libcxx/include/type_traits:3676:1 (clickhouse+0x146f708d)
    #5 void std::__1::__invoke_void_return_wrapper<void>::__call<DB::KeeperStateMachine::create_snapshot(nuraft::snapshot&, std::__1::function<void (bool&, std::__1::shared_ptr<std::exception>&)>&)::$_0&, std::__1::shared_ptr<DB::KeeperStorageSnapshot> >(DB::KeeperStateMachine::create_snapshot(nuraft::snapshot&, std::__1::function<void (bool&, std::__1::shared_ptr<std::exception>&)>&)::$_0&, std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&) obj-x86_64-linux-gnu/../contrib/libcxx/include/__functional_base:348:9 (clickhouse+0x146f708d)
    #6 std::__1::__function::__default_alloc_func<DB::KeeperStateMachine::create_snapshot(nuraft::snapshot&, std::__1::function<void (bool&, std::__1::shared_ptr<std::exception>&)>&)::$_0, void (std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&)>::operator()(std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&) obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:1608:12 (clickhouse+0x146f708d)
    #7 void std::__1::__function::__policy_invoker<void (std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&)>::__call_impl<std::__1::__function::__default_alloc_func<DB::KeeperStateMachine::create_snapshot(nuraft::snapshot&, std::__1::function<void (bool&, std::__1::shared_ptr<std::exception>&)>&)::$_0, void (std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&)> >(std::__1::__function::__policy_storage const*, std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&) obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:2089:16 (clickhouse+0x146f708d)
    #8 std::__1::__function::__policy_func<void (std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&)>::operator()(std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&) const obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:2221:16 (clickhouse+0x1472f987)
    #9 std::__1::function<void (std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&)>::operator()(std::__1::shared_ptr<DB::KeeperStorageSnapshot>&&) const obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:2560:12 (clickhouse+0x1472f987)
    #10 DB::KeeperStorageDispatcher::snapshotThread() obj-x86_64-linux-gnu/../src/Coordination/KeeperStorageDispatcher.cpp:176:13 (clickhouse+0x1472f987)
    #11 DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2::operator()() const obj-x86_64-linux-gnu/../src/Coordination/KeeperStorageDispatcher.cpp:251:53 (clickhouse+0x14734db0)
    #12 decltype(std::__1::forward<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&>(fp)()) std::__1::__invoke_constexpr<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&) obj-x86_64-linux-gnu/../contrib/libcxx/include/type_traits:3682:1 (clickhouse+0x14734db0)
    #13 decltype(auto) std::__1::__apply_tuple_impl<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&, std::__1::tuple<>&>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&, std::__1::tuple<>&, std::__1::__tuple_indices<>) obj-x86_64-linux-gnu/../contrib/libcxx/include/tuple:1415:1 (clickhouse+0x14734db0)
    #14 decltype(auto) std::__1::apply<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&, std::__1::tuple<>&>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&, std::__1::tuple<>&) obj-x86_64-linux-gnu/../contrib/libcxx/include/tuple:1424:1 (clickhouse+0x14734db0)
    #15 ThreadFromGlobalPool::ThreadFromGlobalPool<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&&)::'lambda'()::operator()() obj-x86_64-linux-gnu/../src/Common/ThreadPool.h:182:13 (clickhouse+0x14734db0)
    #16 decltype(std::__1::forward<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2>(fp)()) std::__1::__invoke<ThreadFromGlobalPool::ThreadFromGlobalPool<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&&)::'lambda'()&>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&&) obj-x86_64-linux-gnu/../contrib/libcxx/include/type_traits:3676:1 (clickhouse+0x14734db0)
    #17 void std::__1::__invoke_void_return_wrapper<void>::__call<ThreadFromGlobalPool::ThreadFromGlobalPool<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&&)::'lambda'()&>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&&...) obj-x86_64-linux-gnu/../contrib/libcxx/include/__functional_base:348:9 (clickhouse+0x14734db0)
    #18 std::__1::__function::__default_alloc_func<ThreadFromGlobalPool::ThreadFromGlobalPool<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&&)::'lambda'(), void ()>::operator()() obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:1608:12 (clickhouse+0x14734db0)
    #19 void std::__1::__function::__policy_invoker<void ()>::__call_impl<std::__1::__function::__default_alloc_func<ThreadFromGlobalPool::ThreadFromGlobalPool<DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2>(DB::KeeperStorageDispatcher::initialize(Poco::Util::AbstractConfiguration const&, bool)::$_2&&)::'lambda'(), void ()> >(std::__1::__function::__policy_storage const*) obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:2089:16 (clickhouse+0x14734db0)
    #20 std::__1::__function::__policy_func<void ()>::operator()() const obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:2221:16 (clickhouse+0x8fad5ba)
    #21 std::__1::function<void ()>::operator()() const obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:2560:12 (clickhouse+0x8fad5ba)
    #22 ThreadPoolImpl<std::__1::thread>::worker(std::__1::__list_iterator<std::__1::thread, void*>) obj-x86_64-linux-gnu/../src/Common/ThreadPool.cpp:254:17 (clickhouse+0x8fad5ba)
    #23 void ThreadPoolImpl<std::__1::thread>::scheduleImpl<void>(std::__1::function<void ()>, int, std::__1::optional<unsigned long>)::'lambda1'()::operator()() const obj-x86_64-linux-gnu/../src/Common/ThreadPool.cpp:124:73 (clickhouse+0x8fb1638)
    #24 decltype(std::__1::forward<void>(fp)(std::__1::forward<void ThreadPoolImpl<std::__1::thread>::scheduleImpl<void>(std::__1::function<void ()>, int, std::__1::optional<unsigned long>)::'lambda1'()>(fp0)...)) std::__1::__invoke<void ThreadPoolImpl<std::__1::thread>::scheduleImpl<void>(std::__1::function<void ()>, int, std::__1::optional<unsigned long>)::'lambda1'()>(void&&, void ThreadPoolImpl<std::__1::thread>::scheduleImpl<void>(std::__1::function<void ()>, int, std::__1::optional<unsigned long>)::'lambda1'()&&...) obj-x86_64-linux-gnu/../contrib/libcxx/include/type_traits:3676:1 (clickhouse+0x8fb1638)
    #25 void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void ThreadPoolImpl<std::__1::thread>::scheduleImpl<void>(std::__1::function<void ()>, int, std::__1::optional<unsigned long>)::'lambda1'()>(std::__1::tuple<void, void ThreadPoolImpl<std::__1::thread>::scheduleImpl<void>(std::__1::function<void ()>, int, std::__1::optional<unsigned long>)::'lambda1'()>&, std::__1::__tuple_indices<>) obj-x86_64-linux-gnu/../contrib/libcxx/include/thread:280:5 (clickhouse+0x8fb1638)
    #26 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void ThreadPoolImpl<std::__1::thread>::scheduleImpl<void>(std::__1::function<void ()>, int, std::__1::optional<unsigned long>)::'lambda1'()> >(void*) obj-x86_64-linux-gnu/../contrib/libcxx/include/thread:291:5 (clickhouse+0x8fb1638)

  Previous write of size 8 at 0x7b3801a40d28 by thread T41 (mutexes: write M3463, write M3476):
    #0 std::__1::list<DB::ListNode<DB::KeeperStorage::Node>, std::__1::allocator<DB::ListNode<DB::KeeperStorage::Node> > >::__link_nodes(std::__1::__list_node_base<DB::ListNode<DB::KeeperStorage::Node>, void*>*, std::__1::__list_node_base<DB::ListNode<DB::KeeperStorage::Node>, void*>*, std::__1::__list_node_base<DB::ListNode<DB::KeeperStorage::Node>, void*>*) obj-x86_64-linux-gnu/../contrib/libcxx/include/list:1167:27 (clickhouse+0x14704a36)
    #1 std::__1::list<DB::ListNode<DB::KeeperStorage::Node>, std::__1::allocator<DB::ListNode<DB::KeeperStorage::Node> > >::insert(std::__1::__list_const_iterator<DB::ListNode<DB::KeeperStorage::Node>, void*>, DB::ListNode<DB::KeeperStorage::Node> const&) obj-x86_64-linux-gnu/../contrib/libcxx/include/list:1460:5 (clickhouse+0x14704a36)
    #2 DB::SnapshotableHashTable<DB::KeeperStorage::Node>::updateValue(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (DB::KeeperStorage::Node&)>) obj-x86_64-linux-gnu/../src/Coordination/SnapshotableHashTable.h:118:29 (clickhouse+0x147038c7)
    #3 DB::KeeperStorageCreateRequest::process(DB::SnapshotableHashTable<DB::KeeperStorage::Node>&, std::__1::unordered_map<long, std::__1::unordered_set<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >, std::__1::hash<long>, std::__1::equal_to<long>, std::__1::allocator<std::__1::pair<long const, std::__1::unordered_set<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > > > >&, long, long) const obj-x86_64-linux-gnu/../src/Coordination/KeeperStorage.cpp:171:27 (clickhouse+0x14723411)
    #4 DB::KeeperStorage::processRequest(std::__1::shared_ptr<Coordination::ZooKeeperRequest> const&, long, std::__1::optional<long>) obj-x86_64-linux-gnu/../src/Coordination/KeeperStorage.cpp:692:47 (clickhouse+0x1471b3e1)
    #5 DB::KeeperStateMachine::commit(unsigned long, nuraft::buffer&) obj-x86_64-linux-gnu/../src/Coordination/KeeperStateMachine.cpp:119:47 (clickhouse+0x146f406e)
    #6 nuraft::state_machine::commit_ext(nuraft::state_machine::ext_op_params const&) obj-x86_64-linux-gnu/../contrib/NuRaft/include/libnuraft/state_machine.hxx:73:16 (clickhouse+0x146f87f2)
    #7 nuraft::raft_server::commit_app_log(unsigned long, std::__1::shared_ptr<nuraft::log_entry>&, bool) obj-x86_64-linux-gnu/../contrib/NuRaft/src/handle_commit.cxx:284:33 (clickhouse+0x1779be08)
    #8 nuraft::raft_server::commit_in_bg_exec(unsigned long) obj-x86_64-linux-gnu/../contrib/NuRaft/src/handle_commit.cxx:215:13 (clickhouse+0x1779b30f)
    #9 nuraft::raft_server::commit_in_bg() obj-x86_64-linux-gnu/../contrib/NuRaft/src/handle_commit.cxx:135:9 (clickhouse+0x1779a78a)
    #10 decltype(*(std::__1::forward<nuraft::raft_server*&>(fp0)).*fp()) std::__1::__invoke<void (nuraft::raft_server::*&)(), nuraft::raft_server*&, void>(void (nuraft::raft_server::*&)(), nuraft::raft_server*&) obj-x86_64-linux-gnu/../contrib/libcxx/include/type_traits:3617:1 (clickhouse+0x1778e6e9)
    #11 std::__1::__bind_return<void (nuraft::raft_server::*)(), std::__1::tuple<nuraft::raft_server*>, std::__1::tuple<>, __is_valid_bind_return<void (nuraft::raft_server::*)(), std::__1::tuple<nuraft::raft_server*>, std::__1::tuple<> >::value>::type std::__1::__apply_functor<void (nuraft::raft_server::*)(), std::__1::tuple<nuraft::raft_server*>, 0ul, std::__1::tuple<> >(void (nuraft::raft_server::*&)(), std::__1::tuple<nuraft::raft_server*>&, std::__1::__tuple_indices<0ul>, std::__1::tuple<>&&) obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:2857:12 (clickhouse+0x1778e6e9)
    #12 std::__1::__bind_return<void (nuraft::raft_server::*)(), std::__1::tuple<nuraft::raft_server*>, std::__1::tuple<>, __is_valid_bind_return<void (nuraft::raft_server::*)(), std::__1::tuple<nuraft::raft_server*>, std::__1::tuple<> >::value>::type std::__1::__bind<void (nuraft::raft_server::*)(), nuraft::raft_server*>::operator()<>() obj-x86_64-linux-gnu/../contrib/libcxx/include/functional:2890:20 (clickhouse+0x1778e6e9)
    #13 decltype(std::__1::forward<std::__1::__bind<void (nuraft::raft_server::*)(), nuraft::raft_server*> >(fp)()) std::__1::__invoke<std::__1::__bind<void (nuraft::raft_server::*)(), nuraft::raft_server*> >(std::__1::__bind<void (nuraft::raft_server::*)(), nuraft::raft_server*>&&) obj-x86_64-linux-gnu/../contrib/libcxx/include/type_traits:3676:1 (clickhouse+0x1778e6e9)
    #14 void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, std::__1::__bind<void (nuraft::raft_server::*)(), nuraft::raft_server*> >(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, std::__1::__bind<void (nuraft::raft_server::*)(), nuraft::raft_server*> >&, std::__1::__tuple_indices<>) obj-x86_64-linux-gnu/../contrib/libcxx/include/thread:280:5 (clickhouse+0x1778e6e9)
    #15 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, std::__1::__bind<void (nuraft::raft_server::*)(), nuraft::raft_server*> > >(void*) obj-x86_64-linux-gnu/../contrib/libcxx/include/thread:291:5 (clickhouse+0x1778e6e9)

SUMMARY: ThreadSanitizer: data race obj-x86_64-linux-gnu/../contrib/libcxx/include/list:509:26 in std::__1::__list_const_iterator<DB::ListNode<DB::KeeperStorage::Node>, void*>::operator++()
==================

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label May 25, 2021
@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented May 25, 2021

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented May 25, 2021

Command update: success

Branch has been successfully updated

nikitamikhaylov
nikitamikhaylov reviewed May 25, 2021
View reviewed changes
src/Coordination/KeeperSnapshotManager.cpp
writeBinary(snapshot.snapshot_container_size, out);
size_t counter = 0;
for (auto it = snapshot.begin; counter < snapshot.snapshot_container_size; ++it, ++counter)
for (auto it = snapshot.begin; counter < snapshot.snapshot_container_size; ++counter)
Copy link
Copy Markdown
Member

@nikitamikhaylov nikitamikhaylov May 25, 2021

Choose a reason for hiding this comment

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

auto it = snapshot.begin;
for (size_t counter = 0; counter < snapshot.snapshot_container_size; ++counter)

@nikitamikhaylov nikitamikhaylov self-assigned this May 25, 2021
@alesapin alesapin merged commit b63dfee into master May 26, 2021
@alesapin alesapin deleted the fix_fake_race branch May 26, 2021 08:43
@alexey-milovidov alexey-milovidov mentioned this pull request Nov 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikitamikhaylov nikitamikhaylov nikitamikhaylov approved these changes

Assignees

@nikitamikhaylov nikitamikhaylov

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alesapin @nikitamikhaylov @robot-clickhouse