[modular][testing] Fix race: uninitialized access in TestHarnessLauncher
Observed this race in flaky test:
TestHarnessBuilderTest.AddServiceFromComponent.
I suspect that the thread might be accessing an uninitialized
std::mutex. This CL changes the initialization order to avoid this
condition.
Test: I'm no longer able to make the test hang using:
fx run-test test_harness_fixture_test -t test_harness_fixture_test_bin
-- --gtest_filter='TestHarnessBuilderTest.AddServiceFromComponent'
--gtest_repeat=10000
Change-Id: I11824652f22b08a80a9e34533e5cdca2b9cc09e3
diff --git a/peridot/public/lib/modular_test_harness/cpp/test_harness_launcher.cc b/peridot/public/lib/modular_test_harness/cpp/test_harness_launcher.cc
index fbd9adc..7382f8a 100644
--- a/peridot/public/lib/modular_test_harness/cpp/test_harness_launcher.cc
+++ b/peridot/public/lib/modular_test_harness/cpp/test_harness_launcher.cc
@@ -14,13 +14,14 @@
"modular_test_harness.cmx";
};
-TestHarnessLauncher::TestHarnessLauncher()
- : harness_launcher_thread_(
- [this](fidl::InterfaceRequest<fuchsia::modular::testing::TestHarness>
- test_harness_req) {
- LaunchTestHarness(std::move(test_harness_req));
- },
- test_harness_.NewRequest()) {}
+TestHarnessLauncher::TestHarnessLauncher() {
+ harness_launcher_thread_.reset(new std::thread(
+ [this](fidl::InterfaceRequest<fuchsia::modular::testing::TestHarness>
+ test_harness_req) {
+ LaunchTestHarness(std::move(test_harness_req));
+ },
+ test_harness_.NewRequest()));
+}
TestHarnessLauncher::~TestHarnessLauncher() {
// Wait until the test harness thread's async::Loop is ready
@@ -36,7 +37,7 @@
[this] { test_harness_ctrl_->Kill(); });
// Wait for the thread to exit.
- harness_launcher_thread_.join();
+ harness_launcher_thread_->join();
}
void TestHarnessLauncher::LaunchTestHarness(
diff --git a/peridot/public/lib/modular_test_harness/cpp/test_harness_launcher.h b/peridot/public/lib/modular_test_harness/cpp/test_harness_launcher.h
index 35b0d3b..0f2caa9 100644
--- a/peridot/public/lib/modular_test_harness/cpp/test_harness_launcher.h
+++ b/peridot/public/lib/modular_test_harness/cpp/test_harness_launcher.h
@@ -41,14 +41,17 @@
fuchsia::modular::testing::TestHarnessPtr test_harness_;
// In order to avoid depending on the owning thread's run loop, the test
- // harness component is launched and manageed in a separate thread which
+ // harness component is launched and managed in a separate thread which
// contains its own async loop.
- std::thread harness_launcher_thread_;
- async::Loop* test_harness_loop_ = nullptr; // serving thread's loop.
- std::mutex test_harness_loop_mutex_; // protects |test_harness_loop_|
- std::condition_variable
- test_harness_loop_cv_; // used to signal when |test_harness_loop_| !=
- // nullptr
+ async::Loop* test_harness_loop_ = nullptr;
+ // protects |test_harness_loop_|
+ std::mutex test_harness_loop_mutex_;
+ // used to signal when |test_harness_loop_| !=
+ // nullptr
+ std::condition_variable test_harness_loop_cv_;
+ // IMPORTANT: To avoid racy uninitialized access, this thread should be
+ // initialized *after* all of the member variables it uses are initialized.
+ std::unique_ptr<std::thread> harness_launcher_thread_;
};
} // namespace testing
