Fix deadlock in Folly RCU

Summary:
The deadlock repro is described https://github.com/facebook/folly/issues/1211.
Fix is to check the working epoch before retire() starts the sync work.

Reviewed By: yfeldblum

Differential Revision: D17908896

fbshipit-source-id: 85847fc17f4a75058d4a72c43dba0c570a2593d6

folly/synchronization/Rcu-inl.h

@@ -89,14 +89,21 @@ void rcu_domain<Tag>::retire(list_node* node) noexcept {
   if (time > syncTime + syncTimePeriod_ &&
       syncTime_.compare_exchange_strong(
           syncTime, time, std::memory_order_relaxed)) {
-    list_head finished;
-    {
-      std::lock_guard<std::mutex> g(syncMutex_);
-      half_sync(false, finished);
+    // Starting doing the sync work. However it's possible that someone else
+    // is already assigned, so add the work check here as well, and move on if
+    // the work is assigned.
+    auto target = version_.load(std::memory_order_acquire) + 1;
+    auto work = work_.load(std::memory_order_acquire);
+    if (work < target && work_.compare_exchange_strong(work, target)) {
+      list_head finished;
+      {
+        std::lock_guard<std::mutex> g(syncMutex_);
+        half_sync(false, finished);
+      }
+      // callbacks are called outside of syncMutex_
+      finished.forEach(
+          [&](list_node* item) { executor_->add(std::move(item->cb_)); });
     }
-    // callbacks are called outside of syncMutex_
-    finished.forEach(
-        [&](list_node* item) { executor_->add(std::move(item->cb_)); });
   }
 }
 

folly/synchronization/test/RcuTest.cpp

@@ -25,6 +25,8 @@
 #include <folly/Random.h>
 #include <folly/portability/GFlags.h>
 #include <folly/portability/GTest.h>
+#include <folly/synchronization/Baton.h>
+#include <folly/synchronization/test/Barrier.h>
 
 using namespace folly;
 
@@ -289,3 +291,34 @@ TEST(RcuTest, RcuObjBase) {
   synchronize_rcu();
   EXPECT_TRUE(retired);
 }
+
+namespace folly {
+
+TEST(RcuTest, DeadLock) {
+  // The test tries to reproduce the potential deadlock caused by 2 threads.
+  // T1 does rcu_read_lock(), T2 starts synchronization_rcu(), T1 tries to
+  // call_rcu(). Which made deadlock happens.
+
+  // Use fresh RcuTag and domain to consistantly reproduce the deadlock.
+  struct FreshTag;
+  rcu_domain<FreshTag> fresh_domain;
+
+  // Run the test several times to guarantee the deadlock sequence happens.
+  for (int i = 0; i < 5; i++) {
+    folly::test::Barrier sync_ready(2);
+    std::thread t1([&] {
+      folly::rcu_reader_domain guard(&fresh_domain);
+      sync_ready.wait();
+      folly::rcu_retire(new std::string(), {}, &fresh_domain);
+    });
+    std::thread t2([&] {
+      sync_ready.wait();
+      folly::synchronize_rcu(&fresh_domain);
+    });
+
+    t1.join();
+    t2.join();
+  }
+}
+
+} // namespace folly