Skip to content

Commit

Permalink
Fix vptr TSAN race in FsdbStreamClient
Browse files Browse the repository at this point in the history 
Summary:
Using command from test plan:
```
==================
WARNING: ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) (pid=2900615)
  Read of size 8 at 0x7b5400005000 by thread T7:
    #0 folly::AsyncTimeout::libeventCallback(int, short, void*) folly/io/async/AsyncTimeout.cpp:174 (libfolly_io_async_async_base.so+0x46551)
    #1 event_process_active /home/engshare/third-party2/libevent/1.4.14b_hphp/src/libevent-1.4.14b-stable/event.c:390:5 (libevent-1.4.so.2+0x85e8)
    #2 event_base_loop /home/engshare/third-party2/libevent/1.4.14b_hphp/src/libevent-1.4.14b-stable/event.c:532:4 (libevent-1.4.so.2+0x85e8)
    #3 folly::EventBase::loopMain(int, bool) folly/io/async/EventBase.cpp:400 (libfolly_io_async_async_base.so+0x55033)
    #4 folly::EventBase::loopBody(int, bool) folly/io/async/EventBase.cpp:326 (libfolly_io_async_async_base.so+0x54655)
    #5 folly::EventBase::loop() folly/io/async/EventBase.cpp:305 (libfolly_io_async_async_base.so+0x544cd)
    #6 folly::EventBase::loopForever() folly/io/async/EventBase.cpp:545 (libfolly_io_async_async_base.so+0x58632)
    #7 folly::run(folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> const&) folly/io/async/ScopedEventBaseThread.cpp:40 (libfolly_io_async_scoped_event_base_thread.so+0xc592)
    #8 void std::__invoke_impl<void, void (*)(folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> const&), folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> >(std::__invoke_other, void (*&&)(folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> const&), folly::EventBaseManager*&&, folly::EventBase*&&, folly::Baton<true, std::atomic>*&&, folly::Range<char const*>&&) third-party-buck/platform009/build/libgcc/include/c++/9.x/bits/invoke.h:60 (libfolly_io_async_scoped_event_base_thread.so+0x18545)
    #9 std::__invoke_result<void (*)(folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> const&), folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> >::type std::__invoke<void (*)(folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> const&), folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> >(void (*&&)(folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> const&), folly::EventBaseManager*&&, folly::EventBase*&&, folly::Baton<true, std::atomic>*&&, folly::Range<char const*>&&) third-party-buck/platform009/build/libgcc/include/c++/9.x/bits/invoke.h:95 (libfolly_io_async_scoped_event_base_thread.so+0x18267)
    #10 void std::thread::_Invoker<std::tuple<void (*)(folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> const&), folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> > >::_M_invoke<0ul, 1ul, 2ul, 3ul, 4ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul, 4ul>) third-party-buck/platform009/build/libgcc/include/c++/9.x/thread:244 (libfolly_io_async_scoped_event_base_thread.so+0x18193)
    #11 std::thread::_Invoker<std::tuple<void (*)(folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> const&), folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> > >::operator()() third-party-buck/platform009/build/libgcc/include/c++/9.x/thread:251 (libfolly_io_async_scoped_event_base_thread.so+0x180c8)
    #12 std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> const&), folly::EventBaseManager*, folly::EventBase*, folly::Baton<true, std::atomic>*, folly::Range<char const*> > > >::_M_run() third-party-buck/platform009/build/libgcc/include/c++/9.x/thread:195 (libfolly_io_async_scoped_event_base_thread.so+0x1784f)
    #13 execute_native_thread_routine /home/engshare/third-party2/libgcc/9.x/src/gcc-9.x/x86_64-facebook-linux/libstdc++-v3/src/c++11/../../../.././libstdc++-v3/src/c++11/thread.cc:80:18 (libstdc++.so.6+0xd9660)

  Previous write of size 8 at 0x7b5400005000 by main thread (mutexes: write M118355245544776416):
    #0 facebook::fboss::fsdb::FsdbSubscriber<facebook::fboss::fsdb::OperState>::FsdbSubscriber(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > 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> > > > const&, folly::EventBase*, folly::EventBase*, std::function<void (facebook::fboss::fsdb::OperState&&)>, bool, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>) fboss/fsdb/client/FsdbSubscriber.h:44 (libfboss_fsdb_client_fsdb_pub_sub.so+0x16eb06)
    #1 facebook::fboss::fsdb::FsdbStateSubscriber::FsdbStateSubscriber(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > 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> > > > const&, folly::EventBase*, folly::EventBase*, std::function<void (facebook::fboss::fsdb::OperState&&)>, bool, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>) fboss/fsdb/client/FsdbStateSubscriber.h:14 (libfboss_fsdb_client_fsdb_pub_sub.so+0x16e7d4)
    #2 std::_MakeUniq<facebook::fboss::fsdb::FsdbStateSubscriber>::__single_object std::make_unique<facebook::fboss::fsdb::FsdbStateSubscriber, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > 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> > > > const&, folly::EventBase*, folly::EventBase*, std::function<void (facebook::fboss::fsdb::OperState&&)>&, bool&, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>&>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > 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> > > > const&, folly::EventBase*&&, folly::EventBase*&&, std::function<void (facebook::fboss::fsdb::OperState&&)>&, bool&, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>&) third-party-buck/platform009/build/libgcc/include/c++/9.x/bits/unique_ptr.h:857 (libfboss_fsdb_client_fsdb_pub_sub.so+0x16d101)
    #3 _ZZN8facebook5fboss4fsdb17FsdbPubSubManager19addSubscriptionImplINS1_19FsdbStateSubscriberEEEvRKSt6vectorINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaISB_EESt8functionIFvNS1_16FsdbStreamClient5StateESI_EENT_19FsdbSubUnitUpdateCbEbRKSB_iENKUlTyRSL_E_clISt13unordered_mapISB_St10unique_ptrISH_St14default_deleteISH_EESt4hashISB_ESt8equal_toISB_ESaISt4pairISN_SW_EEEEEDaSP_ fboss/fsdb/client/FsdbPubSubManager.cpp:243 (libfboss_fsdb_client_fsdb_pub_sub.so+0x16c8cd)
    #4 _ZN5folly16SynchronizedBaseINS_12SynchronizedISt13unordered_mapINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESt10unique_ptrIN8facebook5fboss4fsdb16FsdbStreamClientESt14default_deleteISD_EESt4hashIS8_ESt8equal_toIS8_ESaISt4pairIKS8_SG_EEENS_15SharedMutexImplILb0EvSt6atomicNS_24SharedMutexPolicyDefaultEEEEELNS_6detail22SynchronizedMutexLevelE2EE9withWLockIZNSC_17FsdbPubSubManager19addSubscriptionImplINSC_19FsdbStateSubscriberEEEvRKSt6vectorIS8_SaIS8_EESt8functionIFvNSD_5StateES18_EENT_19FsdbSubUnitUpdateCbEbRSM_iEUlTyRS1B_E_EEDaOS1B_ folly/Synchronized.h:305 (libfboss_fsdb_client_fsdb_pub_sub.so+0x16c6a7)
    #5 void facebook::fboss::fsdb::FsdbPubSubManager::addSubscriptionImpl<facebook::fboss::fsdb::FsdbStateSubscriber>(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&, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>, facebook::fboss::fsdb::FsdbStateSubscriber::FsdbSubUnitUpdateCb, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int) fboss/fsdb/client/FsdbPubSubManager.cpp:240 (libfboss_fsdb_client_fsdb_pub_sub.so+0x139f75)
    #6 facebook::fboss::fsdb::FsdbPubSubManager::addStatePathSubscription(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&, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>, std::function<void (facebook::fboss::fsdb::OperState&&)>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int) fboss/fsdb/client/FsdbPubSubManager.cpp:190 (libfboss_fsdb_client_fsdb_pub_sub.so+0x139d50)
    #7 facebook::fboss::TestSubscription<thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > > >::TestSubscription(facebook::fboss::fsdb::FsdbPubSubManager*, thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > >) fboss/qsfp_service/fsdb/facebook/test/QsfpFsdbSyncerTest.cpp:69 (fsdb-syncer-test+0x104e5d)
    #8 facebook::fboss::TestSubscription<thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > > > facebook::fboss::QsfpFsdbTcvrMgrTest::subscribe<thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > > >(thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > >) fboss/qsfp_service/fsdb/facebook/test/QsfpFsdbSyncerTest.cpp:158 (fsdb-syncer-test+0xe3417)
    #9 facebook::fboss::QsfpFsdbTcvrMgrTest_testTcvrMgrConfigUpdate_Test::TestBody() fboss/qsfp_service/fsdb/facebook/test/QsfpFsdbSyncerTest.cpp:195 (fsdb-syncer-test+0xe2cd0)
    #10 testing::Test::Run() /home/engshare/third-party2/googletest/20201023/src/googletest/googletest/src/gtest.cc:2680:50 (libgtest.so.1.10.0+0x4d6a5)
    #11 testing::Test::Run() /home/engshare/third-party2/googletest/20201023/src/googletest/googletest/src/gtest.cc:2670:6 (libgtest.so.1.10.0+0x4d6a5)
    #12 main common/gtest/LightMain.cpp:20 (libcommon_gtest_light_main.so+0x218a)

  Location is heap block of size 592 at 0x7b5400005000 allocated by main thread:
    #0 operator new(unsigned long) <null> (fsdb-syncer-test+0x110b2c)
    #1 std::_MakeUniq<facebook::fboss::fsdb::FsdbStateSubscriber>::__single_object std::make_unique<facebook::fboss::fsdb::FsdbStateSubscriber, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > 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> > > > const&, folly::EventBase*, folly::EventBase*, std::function<void (facebook::fboss::fsdb::OperState&&)>&, bool&, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>&>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > 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> > > > const&, folly::EventBase*&&, folly::EventBase*&&, std::function<void (facebook::fboss::fsdb::OperState&&)>&, bool&, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>&) third-party-buck/platform009/build/libgcc/include/c++/9.x/bits/unique_ptr.h:857 (libfboss_fsdb_client_fsdb_pub_sub.so+0x16cf9f)
    #2 _ZZN8facebook5fboss4fsdb17FsdbPubSubManager19addSubscriptionImplINS1_19FsdbStateSubscriberEEEvRKSt6vectorINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaISB_EESt8functionIFvNS1_16FsdbStreamClient5StateESI_EENT_19FsdbSubUnitUpdateCbEbRKSB_iENKUlTyRSL_E_clISt13unordered_mapISB_St10unique_ptrISH_St14default_deleteISH_EESt4hashISB_ESt8equal_toISB_ESaISt4pairISN_SW_EEEEEDaSP_ fboss/fsdb/client/FsdbPubSubManager.cpp:243 (libfboss_fsdb_client_fsdb_pub_sub.so+0x16c8cd)
    #3 _ZN5folly16SynchronizedBaseINS_12SynchronizedISt13unordered_mapINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESt10unique_ptrIN8facebook5fboss4fsdb16FsdbStreamClientESt14default_deleteISD_EESt4hashIS8_ESt8equal_toIS8_ESaISt4pairIKS8_SG_EEENS_15SharedMutexImplILb0EvSt6atomicNS_24SharedMutexPolicyDefaultEEEEELNS_6detail22SynchronizedMutexLevelE2EE9withWLockIZNSC_17FsdbPubSubManager19addSubscriptionImplINSC_19FsdbStateSubscriberEEEvRKSt6vectorIS8_SaIS8_EESt8functionIFvNSD_5StateES18_EENT_19FsdbSubUnitUpdateCbEbRSM_iEUlTyRS1B_E_EEDaOS1B_ folly/Synchronized.h:305 (libfboss_fsdb_client_fsdb_pub_sub.so+0x16c6a7)
    #4 void facebook::fboss::fsdb::FsdbPubSubManager::addSubscriptionImpl<facebook::fboss::fsdb::FsdbStateSubscriber>(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&, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>, facebook::fboss::fsdb::FsdbStateSubscriber::FsdbSubUnitUpdateCb, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int) fboss/fsdb/client/FsdbPubSubManager.cpp:240 (libfboss_fsdb_client_fsdb_pub_sub.so+0x139f75)
    #5 facebook::fboss::fsdb::FsdbPubSubManager::addStatePathSubscription(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&, std::function<void (facebook::fboss::fsdb::FsdbStreamClient::State, facebook::fboss::fsdb::FsdbStreamClient::State)>, std::function<void (facebook::fboss::fsdb::OperState&&)>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int) fboss/fsdb/client/FsdbPubSubManager.cpp:190 (libfboss_fsdb_client_fsdb_pub_sub.so+0x139d50)
    #6 facebook::fboss::TestSubscription<thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > > >::TestSubscription(facebook::fboss::fsdb::FsdbPubSubManager*, thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > >) fboss/qsfp_service/fsdb/facebook/test/QsfpFsdbSyncerTest.cpp:69 (fsdb-syncer-test+0x104e5d)
    #7 facebook::fboss::TestSubscription<thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > > > facebook::fboss::QsfpFsdbTcvrMgrTest::subscribe<thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > > >(thriftpath::ChildThriftPath<facebook::fboss::cfg::QsfpServiceConfig, facebook::fboss::fsdb::FsdbOperStateRoot, thriftpath::Path<facebook::fboss::fsdb::QsfpServiceData, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, thriftpath::Path<facebook::fboss::fsdb::FsdbOperStateRoot, facebook::fboss::fsdb::FsdbOperStateRoot, apache::thrift::type_class::structure, folly::Unit> > >) fboss/qsfp_service/fsdb/facebook/test/QsfpFsdbSyncerTest.cpp:158 (fsdb-syncer-test+0xe3417)
    #8 facebook::fboss::QsfpFsdbTcvrMgrTest_testTcvrMgrConfigUpdate_Test::TestBody() fboss/qsfp_service/fsdb/facebook/test/QsfpFsdbSyncerTest.cpp:195 (fsdb-syncer-test+0xe2cd0)
    #9 testing::Test::Run() /home/engshare/third-party2/googletest/20201023/src/googletest/googletest/src/gtest.cc:2680:50 (libgtest.so.1.10.0+0x4d6a5)
    #10 testing::Test::Run() /home/engshare/third-party2/googletest/20201023/src/googletest/googletest/src/gtest.cc:2670:6 (libgtest.so.1.10.0+0x4d6a5)
    #11 main common/gtest/LightMain.cpp:20 (libcommon_gtest_light_main.so+0x218a)

  Mutex M118355245544776416 is already destroyed.

  Thread T7 'FsdbReconnectTh' (tid=2901715, running) created by main thread at:
    #0 pthread_create <null> (fsdb-syncer-test+0x1744ed)
    #1 __gthread_create /home/engshare/third-party2/libgcc/9.x/src/gcc-9.x/x86_64-facebook-linux/libstdc++-v3/include/x86_64-facebook-linux/bits/gthr-default.h:663:35 (libstdc++.so.6+0xd993c)
    #2 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) /home/engshare/third-party2/libgcc/9.x/src/gcc-9.x/x86_64-facebook-linux/libstdc++-v3/src/c++11/../../../.././libstdc++-v3/src/c++11/thread.cc:135:37 (libstdc++.so.6+0xd993c)
    #3 folly::ScopedEventBaseThread::ScopedEventBaseThread(folly::EventBase::Options, folly::EventBaseManager*, folly::Range<char const*>) folly/io/async/ScopedEventBaseThread.cpp:68 (libfolly_io_async_scoped_event_base_thread.so+0xc060)
    #4 folly::ScopedEventBaseThread::ScopedEventBaseThread(folly::EventBaseManager*, folly::Range<char const*>) folly/io/async/ScopedEventBaseThread.cpp:60 (libfolly_io_async_scoped_event_base_thread.so+0xb4b8)
    #5 folly::ScopedEventBaseThread::ScopedEventBaseThread(folly::Range<char const*>) folly/io/async/ScopedEventBaseThread.cpp:53 (libfolly_io_async_scoped_event_base_thread.so+0xb672)
    #6 facebook::fboss::fsdb::FsdbPubSubManager::FsdbPubSubManager(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) fboss/fsdb/client/FsdbPubSubManager.h:139 (libfboss_fsdb_client_fsdb_pub_sub.so+0x1371f2)
    #7 std::_MakeUniq<facebook::fboss::fsdb::FsdbPubSubManager>::__single_object std::make_unique<facebook::fboss::fsdb::FsdbPubSubManager, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) third-party-buck/platform009/build/libgcc/include/c++/9.x/bits/unique_ptr.h:857 (libfboss_fsdb_client_fsdb_syncer.so+0x10247)
    #8 facebook::fboss::fsdb::FsdbSyncManager::FsdbSyncManager(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > 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> > > > 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> > > > const&) fboss/fsdb/client/FsdbSyncManager.cpp:12 (libfboss_fsdb_client_fsdb_syncer.so+0x100be)
    #9 facebook::fboss::QsfpFsdbSyncManager::QsfpFsdbSyncManager() fboss/qsfp_service/fsdb/QsfpFsdbSyncManager.cpp:17 (libfboss_qsfp_service_fsdb_fsdb-syncer.so+0x33191)
    #10 std::_MakeUniq<facebook::fboss::QsfpFsdbSyncManager>::__single_object std::make_unique<facebook::fboss::QsfpFsdbSyncManager>() third-party-buck/platform009/build/libgcc/include/c++/9.x/bits/unique_ptr.h:857 (libfboss_qsfp_service_platforms_wedge_wedge-platform.so+0x1ec45f)
    #11 facebook::fboss::WedgeManager::WedgeManager(std::unique_ptr<facebook::fboss::TransceiverPlatformApi, std::default_delete<facebook::fboss::TransceiverPlatformApi> >, std::unique_ptr<facebook::fboss::PlatformMapping, std::default_delete<facebook::fboss::PlatformMapping> >, facebook::fboss::PlatformMode) fboss/qsfp_service/platforms/wedge/WedgeManager.cpp:63 (libfboss_qsfp_service_platforms_wedge_wedge-platform.so+0x1ebcab)
    #12 facebook::fboss::MockWedgeManager::MockWedgeManager(int, int) fboss/qsfp_service/platforms/wedge/tests/MockWedgeManager.h:25 (libfboss_qsfp_service_test_transceiver_manager_test_helper.so+0x5408f)
    #13 std::_MakeUniq<facebook::fboss::MockWedgeManager>::__single_object std::make_unique<facebook::fboss::MockWedgeManager, int const&>(int const&) third-party-buck/platform009/build/libgcc/include/c++/9.x/bits/unique_ptr.h:857 (libfboss_qsfp_service_test_transceiver_manager_test_helper.so+0x42edb)
    #14 facebook::fboss::TransceiverManagerTestHelper::resetTransceiverManager() fboss/qsfp_service/test/TransceiverManagerTestHelper.cpp:35 (libfboss_qsfp_service_test_transceiver_manager_test_helper.so+0x43138)
    #15 facebook::fboss::QsfpFsdbTcvrMgrTest_testTcvrMgrConfigUpdate_Test::TestBody() fboss/qsfp_service/fsdb/facebook/test/QsfpFsdbSyncerTest.cpp:191 (fsdb-syncer-test+0xe2c53)
    #16 testing::Test::Run() /home/engshare/third-party2/googletest/20201023/src/googletest/googletest/src/gtest.cc:2680:50 (libgtest.so.1.10.0+0x4d6a5)
    #17 testing::Test::Run() /home/engshare/third-party2/googletest/20201023/src/googletest/googletest/src/gtest.cc:2670:6 (libgtest.so.1.10.0+0x4d6a5)
    #18 main common/gtest/LightMain.cpp:20 (libcommon_gtest_light_main.so+0x218a)

ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) folly/io/async/AsyncTimeout.cpp:174 in folly::AsyncTimeout::libeventCallback(int, short, void*)
==================
```

What's going on here?

In FsdbStreamClient constructor, we immediately schedule the periodic reconnect on the reconnect thread. The periodic call back is implemented in folly::AsyncTimeout like this:
```
  auto timeout = reinterpret_cast<AsyncTimeout*>(arg);
...
  timeout->timeoutExpired();
```
We're arg being passed in as a void*. timeoutExpired() is a virtual function we overrided.

If this gets called after the base class constructor FsdbStreamClient has finished but before child constructor classes like FsdbDeltaSubscriber has started, the vtable will point at the base class so virtual method calls have incorrect behavior.

See https://github.com/google/sanitizers/wiki/ThreadSanitizerPopularDataRaces#data-race-on-vptr-during-construction

IMHO, this is probably benign. Because the wrong vtable doesn't actually trigger wrong behavior if nobody override timeoutExpired() in child classes or call virtual methods from the base class implementation. But let's fix this to shut up the warning and prevent future problems.

FsdbStreamClient is not an AsyncTimeout. It needed an AsyncTimeout. So making it a member instead of a parent is probably a better fit. That avoids this hard problem altogether. So we don't need to add start() and stop() from everywhere.

Reviewed By: jasmeetbagga

Differential Revision: D37692128

fbshipit-source-id: fadf86ff7850d37cae49c651cca29c1d2b819b2b
  • Loading branch information
@clement-cheung-fb @facebook-github-bot
clement-cheung-fb authored and facebook-github-bot committed Aug 12, 2022
1 parent 969acda commit 402e97a
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 29 deletions.
20 changes: 0 additions & 20 deletions fboss/fsdb/client/FsdbPubSubManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,6 @@ std::unique_ptr<PublisherT> FsdbPubSubManager::createPublisherImpl(
}
auto& evbThread = publishStats ? statsPublisherStreamEvbThread_
: statePublisherStreamEvbThread_;
// TSAN when creating publisher. Workaround while we work on D37692128.
//
// This vptr race is benign. In folly::AsyncTimeout, we're passed in as void*,
// re-casted and the virtual method timeoutExpired() is called. The thread is
// started in constructor of FsdbStreamClient. Vtable is pointed at
// FsdbStreamClient and not pointed at the final class yet. So the
// timeoutExpired() call is technically not correct. But we currently don't
// override that in children classes. So the incorrect vtable doesn't actually
// produce incorrect result.
folly::annotate_ignore_thread_sanitizer_guard tsanGuard(__FILE__, __LINE__);
auto publisher = std::make_unique<PublisherT>(
clientId_,
publishPath,
Expand Down Expand Up @@ -255,16 +245,6 @@ void FsdbPubSubManager::addSubscriptionImpl(
auto subsStr =
toSubscriptionStr(fsdbHost, subscribePath, isDelta, subscribeStats);
path2Subscriber_.withWLock([&](auto& path2Subscriber) {
// TSAN when creating subscriber. Workaround while we work on D37692128.
//
// This vptr race is benign. In folly::AsyncTimeout, we're passed in as
// void*, re-casted and the virtual method timeoutExpired() is called. The
// thread is started in constructor of FsdbStreamClient. Vtable is pointed
// at FsdbStreamClient and not pointed at the final class yet. So the
// timeoutExpired() call is technically not correct. But we currently don't
// override that in children classes. So the incorrect vtable doesn't
// actually produce incorrect result.
folly::annotate_ignore_thread_sanitizer_guard tsanGuard(__FILE__, __LINE__);
auto [itr, inserted] = path2Subscriber.emplace(std::make_pair(
subsStr,
std::make_unique<SubscriberT>(
Expand Down
14 changes: 9 additions & 5 deletions fboss/fsdb/client/FsdbStreamClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "fboss/fsdb/client/FsdbStreamClient.h"

#include <folly/experimental/coro/BlockingWait.h>
#include <folly/io/async/AsyncTimeout.h>
#include <folly/logging/xlog.h>

#ifndef IS_OSS
Expand All @@ -21,14 +22,16 @@ FsdbStreamClient::FsdbStreamClient(
folly::EventBase* connRetryEvb,
const std::string& counterPrefix,
FsdbStreamStateChangeCb stateChangeCb)
: folly::AsyncTimeout(connRetryEvb),
clientId_(clientId),
: clientId_(clientId),
streamEvb_(streamEvb),
connRetryEvb_(connRetryEvb),
counterPrefix_(counterPrefix),
clientEvbThread_(
std::make_unique<folly::ScopedEventBaseThread>(clientId)),
stateChangeCb_(stateChangeCb),
timer_(folly::AsyncTimeout::make(
*connRetryEvb,
[this]() noexcept { timeoutExpired(); })),
disconnectEvents_(
fb303::ThreadCachedServiceData::get()->getThreadStats(),
counterPrefix_ + ".disconnects",
Expand All @@ -40,7 +43,7 @@ FsdbStreamClient::FsdbStreamClient(
}
fb303::fbData->setCounter(getConnectedCounterName(), 0);
connRetryEvb->runInEventBaseThread(
[this] { scheduleTimeout(FLAGS_fsdb_reconnect_ms); });
[this] { timer_->scheduleTimeout(FLAGS_fsdb_reconnect_ms); });
}

FsdbStreamClient::~FsdbStreamClient() {
Expand Down Expand Up @@ -94,7 +97,7 @@ void FsdbStreamClient::timeoutExpired() noexcept {
connectToServer(
serverAddress_->getIPAddress().str(), serverAddress_->getPort());
}
scheduleTimeout(FLAGS_fsdb_reconnect_ms);
timer_->scheduleTimeout(FLAGS_fsdb_reconnect_ms);
}

bool FsdbStreamClient::isConnectedToServer() const {
Expand Down Expand Up @@ -150,7 +153,8 @@ void FsdbStreamClient::cancel() {
return;
}
serverAddress_.reset();
connRetryEvb_->runInEventBaseThreadAndWait([this] { cancelTimeout(); });
connRetryEvb_->runInEventBaseThreadAndWait(
[this] { timer_->cancelTimeout(); });
setState(State::CANCELLED);
resetClient();
// terminate event base getting ready for clean-up
Expand Down
10 changes: 6 additions & 4 deletions fboss/fsdb/client/FsdbStreamClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@

namespace folly {
class CancellationToken;
}
class AsyncTimeout;
} // namespace folly

namespace apache::thrift {
template <class>
Expand All @@ -27,7 +28,7 @@ class Client;
namespace facebook::fboss::fsdb {
class FsdbService;

class FsdbStreamClient : public folly::AsyncTimeout {
class FsdbStreamClient {
public:
enum class State : uint16_t { DISCONNECTED, CONNECTED, CANCELLED };

Expand All @@ -39,7 +40,7 @@ class FsdbStreamClient : public folly::AsyncTimeout {
const std::string& counterPrefix,
FsdbStreamStateChangeCb stateChangeCb = [](State /*old*/,
State /*newState*/) {});
virtual ~FsdbStreamClient() override;
virtual ~FsdbStreamClient();

void setServerToConnect(
const std::string& ip,
Expand Down Expand Up @@ -84,7 +85,7 @@ class FsdbStreamClient : public folly::AsyncTimeout {
void createClient(const std::string& ip, uint16_t port);
void resetClient();
void connectToServer(const std::string& ip, uint16_t port);
void timeoutExpired() noexcept override;
void timeoutExpired() noexcept;

#if FOLLY_HAS_COROUTINES && !defined(IS_OSS)
folly::coro::Task<void> serviceLoopWrapper();
Expand All @@ -111,6 +112,7 @@ class FsdbStreamClient : public folly::AsyncTimeout {
std::optional<folly::SocketAddress> serverAddress_;
FsdbStreamStateChangeCb stateChangeCb_;
std::atomic<bool> serviceLoopRunning_{false};
std::unique_ptr<folly::AsyncTimeout> timer_;
#if FOLLY_HAS_COROUTINES
folly::coro::CancellableAsyncScope serviceLoopScope_;
#endif
Expand Down

0 comments on commit 402e97a

Please sign in to comment.