mptest: fix race condition in TestSetup constructor

ryanofsky · ryanofsky · commit 0d986ff144cd · 2025-06-27T11:22:07.000-04:00
Fix race condition caught by TSAN between TestSetup constructor and std::thread launched by the constructor, which could lead to the constructor setting member variables to default values after the std::thread set them, which would not be the intended order and could lead to bugs. This change fixes all TSAN errors from the test except one, which will be fixed in a subsequent commit in this PR. The TSAN errors looked like the following (this is just the first error, there were different ones as well): [ TEST ] test.cpp:108: Call FooInterface methods ================== WARNING: ThreadSanitizer: data race (pid=1970628) Write of size 8 at 0x7ffcf7ba4c30 by thread T1: #0 mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const::{lambda(mp::Connection&)#1}::operator()(mp::Connection&) const test/mp/test/test.cpp:71 (mptest+0x131c3d) #1 capnp::Capability::Client std::__invoke_impl<capnp::Capability::Client, mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const::{lambda(mp::Connection&)#1}&, mp::Connection&>(std::__invoke_other, mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const::{lambda(mp::Connection&)#1}&, mp::Connection&) /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/invoke.h:61 (mptest+0x131a25) #2 std::enable_if<is_invocable_r_v<capnp::Capability::Client, mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const::{lambda(mp::Connection&)#1}&, mp::Connection&>, capnp::Capability::Client>::type std::__invoke_r<capnp::Capability::Client, mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const::{lambda(mp::Connection&)#1}&, mp::Connection&>(mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const::{lambda(mp::Connection&)#1}&, mp::Connection&) /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/invoke.h:114 (mptest+0x131a25) #3 std::_Function_handler<capnp::Capability::Client (mp::Connection&), mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const::{lambda(mp::Connection&)#1}>::_M_invoke(std::_Any_data const&, mp::Connection&) /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/std_function.h:290 (mptest+0x131a25) #4 mp::Connection::Connection(mp::EventLoop&, kj::Own<kj::AsyncIoStream, decltype(nullptr)>&&, std::function<capnp::Capability::Client (mp::Connection&)> const&) /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/std_function.h:591 (mptest+0x13167f) #5 mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/unique_ptr.h:1077 (mptest+0x13054c) #6 void std::__invoke_impl<void, mp::test::TestSetup::TestSetup(bool)::{lambda()#1}>(std::__invoke_other, mp::test::TestSetup::TestSetup(bool)::{lambda()#1}&&) /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/invoke.h:61 (mptest+0x1303f9) #7 std::__invoke_result<mp::test::TestSetup::TestSetup(bool)::{lambda()#1}>::type std::__invoke<mp::test::TestSetup::TestSetup(bool)::{lambda()#1}>(mp::test::TestSetup::TestSetup(bool)::{lambda()#1}&&) /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/invoke.h:96 (mptest+0x1303f9) #8 void std::thread::_Invoker<std::tuple<mp::test::TestSetup::TestSetup(bool)::{lambda()#1}> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/std_thread.h:301 (mptest+0x1303f9) #9 std::thread::_Invoker<std::tuple<mp::test::TestSetup::TestSetup(bool)::{lambda()#1}> >::operator()() /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/std_thread.h:308 (mptest+0x1303f9) #10 std::thread::_State_impl<std::thread::_Invoker<std::tuple<mp::test::TestSetup::TestSetup(bool)::{lambda()#1}> > >::_M_run() /nix/store/9ds850ifd4jwcccpp3v14818kk74ldf2-gcc-14.2.1.20250322/include/c++/14.2.1.20250322/bits/std_thread.h:253 (mptest+0x1303f9) #11 execute_native_thread_routine ??:? (libstdc++.so.6+0xed063) Previous write of size 8 at 0x7ffcf7ba4c30 by main thread: #0 __tsan_memset ??:? (mptest+0x8811f) #1 mp::test::TestSetup::TestSetup(bool) test/mp/test/test.cpp:57 (mptest+0x12c3b4) #2 mp::test::TestCase108::run() test/mp/test/test.cpp:110 (mptest+0x128160) #3 kj::Maybe<kj::Exception> kj::runCatchingExceptions<kj::TestRunner::run()::{lambda()#1}>(kj::TestRunner::run()::{lambda()#1}&&) ??:? (libkj-test.so.1.1.0+0x7290) (BuildId: 50ddf81234cd06daf5f2d3f11713ed193ade4eb7) Location is stack of main thread. Thread T1 (tid=1970630, running) created by main thread at: #0 pthread_create ??:? (mptest+0x9a2a5) #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) ??:? (libstdc++.so.6+0xed138) #2 mp::test::TestCase108::run() test/mp/test/test.cpp:110 (mptest+0x128160) #3 kj::Maybe<kj::Exception> kj::runCatchingExceptions<kj::TestRunner::run()::{lambda()#1}>(kj::TestRunner::run()::{lambda()#1}&&) ??:? (libkj-test.so.1.1.0+0x7290) (BuildId: 50ddf81234cd06daf5f2d3f11713ed193ade4eb7) SUMMARY: ThreadSanitizer: data race test/mp/test/test.cpp:71 in mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const::{lambda(mp::Connection&)#1}::operator()(mp::Connection&) const
diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp
@@ -49,12 +49,14 @@ namespace test {
 class TestSetup
 {
 public:
-    std::thread thread;
     std::function<void()> server_disconnect;
     std::function<void()> client_disconnect;
     std::promise<std::unique_ptr<ProxyClient<messages::FooInterface>>> client_promise;
     std::unique_ptr<ProxyClient<messages::FooInterface>> client;
     ProxyServer<messages::FooInterface>* server{nullptr};
+    //! Thread variable should be after other struct members so the thread does
+    //! not start until the other members are initialized.
+    std::thread thread;
 
     TestSetup(bool client_owns_connection = true)
         : thread{[&] {
