Make SyncPoint return immediately when disabled

Summary:
We were frequently seeing a race between SyncPoint::Process() and
SyncPoint::~SyncPoint() (e.g.,
https://our.intern.facebook.com/intern/sandcastle/log/?instance_id=207289975&step_id=2412725431).
The issue was marked_thread_id_ gets deleted when the main thread is exiting and
simultaneously background threads may access it. We can prevent this race
condition by checking whether sync points are disabled (assuming the test terminates
with them disabled) before attempting to access that member. I do not understand
why accesses to other members (mutex_ and enabled_) are ok but anyways the
test no longer fails tsan.

Test Plan: ran tests

Reviewers: sdong, yhchiang, IslamAbdelRahman, yiwu, wanning

Subscribers: andrewkr, dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D62133
main
Andrew Kryczka 8 years ago
parent 64a0082c69
commit 236756f2cf
  1. 8
      util/sync_point.cc

@ -120,6 +120,10 @@ bool SyncPoint::DisabledByMarker(const std::string& point,
void SyncPoint::Process(const std::string& point, void* cb_arg) {
std::unique_lock<std::mutex> lock(mutex_);
if (!enabled_) {
return;
}
auto thread_id = std::this_thread::get_id();
auto marker_iter = markers_.find(point);
@ -133,10 +137,6 @@ void SyncPoint::Process(const std::string& point, void* cb_arg) {
return;
}
if (!enabled_) {
return;
}
while (!PredecessorsAllCleared(point)) {
cv_.wait(lock);
if (DisabledByMarker(point, thread_id)) {

Loading…
Cancel
Save