Fix TIMED_SYNCHRONIZED_CONST bug

Summary:
TIMED_SYNCHRONIZED_CONST is acquiring a read-write lock but releasing a shared lock.  Chaos ensues.
@override-unit-failures

Test Plan: Add unit test, test times out before fix, passes after fix.

Reviewed By: andrei.alexandrescu@fb.com

FB internal diff: D1251518

Blame Revision: D327492

diff --git a/folly/Synchronized.h b/folly/Synchronized.h
index 538bdbab5529269ba7cb8d6bf3c5bb2479fc7a16..83e59e80cef189ca7cd20650a92953288375804b 100644 (file)
--- a/folly/Synchronized.h
+++ b/folly/Synchronized.h
@@ -427,7 +427,7 @@ struct Synchronized {
       acquire();
     }
     ConstLockedPtr(const Synchronized* parent, unsigned int milliseconds) {
-      if (parent->mutex_.timed_lock(
+      if (parent->mutex_.timed_lock_shared(
             boost::posix_time::milliseconds(milliseconds))) {
         parent_ = parent;
         return;

diff --git a/folly/test/SynchronizedTest.cpp b/folly/test/SynchronizedTest.cpp
index a741670d50875aa9c29539c5f4c5db177b1b374c..dd23dd45c87586ded5724a68141aeb333e0426d5 100644 (file)
--- a/folly/test/SynchronizedTest.cpp
+++ b/folly/test/SynchronizedTest.cpp
@@ -119,6 +119,8 @@ TEST(Synchronized, TimedSynchronized) {
   testTimedSynchronized<boost::timed_mutex>();
   testTimedSynchronized<boost::recursive_timed_mutex>();
   testTimedSynchronized<boost::shared_mutex>();
+
+  testTimedSynchronizedWithConst<boost::shared_mutex>();
 }
 #endif
 

diff --git a/folly/test/SynchronizedTestLib-inl.h b/folly/test/SynchronizedTestLib-inl.h
index 3daeba16fb8ace8f775f25ae9bb91d77547e9eca..cbd43624b81e4af2fef59573a62df88a37c21c48 100644 (file)
--- a/folly/test/SynchronizedTestLib-inl.h
+++ b/folly/test/SynchronizedTestLib-inl.h
@@ -274,6 +274,56 @@ template <class Mutex> void testTimedSynchronized() {
   }
 }
 
+template <class Mutex> void testTimedSynchronizedWithConst() {
+  folly::Synchronized<std::vector<int>, Mutex> v;
+
+  struct Local {
+    static bool threadMain(int i,
+                           folly::Synchronized<std::vector<int>, Mutex>& pv) {
+      usleep(::random(100 * 1000, 1000 * 1000));
+
+      // Test operator->
+      pv->push_back(i);
+
+      usleep(::random(5 * 1000, 1000 * 1000));
+      // Test TIMED_SYNCHRONIZED_CONST
+      for (;;) {
+        TIMED_SYNCHRONIZED_CONST (10, v, pv) {
+          if (v) {
+            auto found = std::find(v->begin(), v->end(),  i);
+            CHECK(found != v->end());
+            return true;
+          } else {
+            // do nothing
+            usleep(::random(10 * 1000, 100 * 1000));
+          }
+        }
+      }
+    }
+  };
+
+  std::vector<std::thread> results;
+
+  static const size_t threads = 100;
+  FOR_EACH_RANGE (i, 0, threads) {
+    results.push_back(std::thread([&, i]() { Local::threadMain(i, v); }));
+  }
+
+  FOR_EACH (i, results) {
+    i->join();
+  }
+
+  std::vector<int> result;
+  v.swap(result);
+
+  EXPECT_EQ(result.size(), threads);
+  sort(result.begin(), result.end());
+
+  FOR_EACH_RANGE (i, 0, threads) {
+    EXPECT_EQ(result[i], i);
+  }
+}
+
 template <class Mutex> void testConstCopy() {
   std::vector<int> input = {1, 2, 3};
   const folly::Synchronized<std::vector<int>, Mutex> v(input);

diff --git a/folly/test/SynchronizedTestLib.h b/folly/test/SynchronizedTestLib.h
index 43e51443abe7790602f562700ae4a9536e10470e..c3602cb7b49d8d20e8d6f83c78f8aa34adf316e0 100644 (file)
--- a/folly/test/SynchronizedTestLib.h
+++ b/folly/test/SynchronizedTestLib.h
@@ -40,6 +40,8 @@ template <class Mutex> void testDualLockingWithConst();
 
 template <class Mutex> void testTimedSynchronized();
 
+template <class Mutex> void testTimedSynchronizedWithConst();
+
 template <class Mutex> void testConstCopy();
 
 #include "folly/test/SynchronizedTestLib-inl.h"