Commit 013b287c authored by Robin Cheng's avatar Robin Cheng Committed by Facebook GitHub Bot

Fix a race condition in the thread exit/join code in DeterministicSchedule.

Summary:
Previously, there was a race between exitingSems_[...] = tls.sem (in beforeThreadExit) and exitingSems_[...].post() (in joinAll). The race exists because exitingSems_ is a map that is not protected by the "shared access" synchronization mechanism used by DeterministicSchedule (i.e. it does not sit between a beforeSharedAccess and afterSharedAccess pair). Specifically, the main thread can be reading exitingSems_ while a child thread is writing to it.

This diff fixes that: after all threads are ready to exit (this part isn't changed), we first wait for the joiner thread itself to be scheduled, and then, it will post the semaphore to wake up the child thread to run its thread-local destructors, and at the same time call .join() on the child. All this runs in a "critical section" between beforeSharedAccess and afterSharedAccess in the joiner thread.

For clarity also changed the child thread to use descheduleCurrentThread, and to make the DeterministicSchedule destructor no longer call beforeThreadExit since it makes little sense.

Reviewed By: yfeldblum

Differential Revision: D23119075

fbshipit-source-id: 22bc6ffd9a9d888cc09bdee066dcac7f05b29fa7
parent 8622363b
......@@ -139,7 +139,8 @@ DeterministicSchedule::~DeterministicSchedule() {
assert(tls.sched == this);
assert(sems_.size() == 1);
assert(sems_[0] == tls.sem);
beforeThreadExit();
delete tls.sem;
tls = PerThreadState();
}
std::function<size_t(size_t)> DeterministicSchedule::uniform(uint64_t seed) {
......@@ -324,21 +325,16 @@ void DeterministicSchedule::beforeThreadExit() {
reschedule(parent->second);
joins_.erase(parent);
}
sems_.erase(std::find(sems_.begin(), sems_.end(), tls.sem));
descheduleCurrentThread();
active_.erase(std::this_thread::get_id());
if (!sems_.empty()) {
FOLLY_TEST_DSCHED_VLOG("exiting");
/* Wait here so that parent thread can control when the thread
* enters the thread local destructors. */
exitingSems_[std::this_thread::get_id()] = tls.sem;
afterSharedAccess();
// Wait for the parent thread to allow us to run thread-local destructors.
tls.sem->wait();
}
tls.sched = nullptr;
tls.aux_act = nullptr;
tls.exiting = true;
delete tls.sem;
tls.sem = nullptr;
tls = PerThreadState();
}
void DeterministicSchedule::waitForBeforeThreadExit(std::thread& child) {
......@@ -371,9 +367,13 @@ void DeterministicSchedule::joinAll(std::vector<std::thread>& children) {
* shared access during thread local destructors.*/
for (auto& child : children) {
if (sched) {
beforeSharedAccess();
sched->exitingSems_[child.get_id()]->post();
}
child.join();
if (sched) {
afterSharedAccess();
}
}
}
......@@ -386,9 +386,13 @@ void DeterministicSchedule::join(std::thread& child) {
atomic_thread_fence(std::memory_order_seq_cst);
FOLLY_TEST_DSCHED_VLOG("joined " << std::hex << child.get_id());
if (sched) {
beforeSharedAccess();
sched->exitingSems_[child.get_id()]->post();
}
child.join();
if (sched) {
afterSharedAccess();
}
}
void DeterministicSchedule::callAux(bool success) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment