UNSYNCHRONIZED does NOT unlock the mutex

Summary: My colleague tuomaspelkonen discovered a weird UNSYNCHRONIZED issue a few weeks ago and we ever since stopped using it. Now I finally have some time to root cause it. It turns out UNSYNCHRONIZED unlock the mutex then lock the mutex again, because it copy constructs LockedPtr for overriding the name within the scope, and copy construct locks the mutex again. A one character fix here is to take a reference of LockedPtr instead of copy construct it. However since this is my first time look at the code here, please advise if this is horribly wrong or propose better fix. Also added a test to reproduce the issue without the fix as well as verify the fix.

Reviewed By: yfeldblum

Differential Revision: D2633028

fb-gh-sync-id: a9e8d39b08d4d1265979f8bdaae83619566d10a0

diff --git a/folly/Synchronized.h b/folly/Synchronized.h
index 8a2f9b95c731943a44453afa12bc3b707976edbe..109cea7c530d10859ba6cf08765581dc541e0656 100644 (file)
--- a/folly/Synchronized.h
+++ b/folly/Synchronized.h
@@ -722,7 +722,7 @@ void swap(Synchronized<T, M>& lhs, Synchronized<T, M>& rhs) {
   for (decltype(SYNCHRONIZED_lockedPtr.typeHackDoNotUse())      \
          SYNCHRONIZED_state3(&SYNCHRONIZED_lockedPtr);          \
        !SYNCHRONIZED_state; SYNCHRONIZED_state = true)          \
-    for (auto name = *SYNCHRONIZED_state3.operator->();         \
+    for (auto& name = *SYNCHRONIZED_state3.operator->();        \
          !SYNCHRONIZED_state; SYNCHRONIZED_state = true)
 
 /**

diff --git a/folly/test/SynchronizedTest.cpp b/folly/test/SynchronizedTest.cpp
index afd87f91f45f15b29804337c92071d17f2065230..d9a45d29aecba94f78837ad1c926d5161cd80b6b 100644 (file)
--- a/folly/test/SynchronizedTest.cpp
+++ b/folly/test/SynchronizedTest.cpp
@@ -121,4 +121,110 @@ TYPED_TEST(SynchronizedTest, InPlaceConstruction) {
   testInPlaceConstruction<TypeParam>();
 }
 
+using CountPair = std::pair<int, int>;
+// This class is specialized only to be uesed in SynchronizedLockTest
+class FakeMutex {
+ public:
+  bool lock() {
+    ++lockCount_;
+    return true;
+  }
+
+  bool unlock() {
+    ++unlockCount_;
+    return true;
+  }
+
+  static CountPair getLockUnlockCount() {
+    return CountPair{lockCount_, unlockCount_};
+  }
+
+  static void resetLockUnlockCount() {
+    lockCount_ = 0;
+    unlockCount_ = 0;
+  }
+ private:
+  // Keep these two static for test access
+  // Keep them thread_local in case of tests are run in parallel within one
+  // process
+  static thread_local int lockCount_;
+  static thread_local int unlockCount_;
+
+  // Adapters for Synchronized<>
+  friend void acquireReadWrite(FakeMutex& lock) { lock.lock(); }
+  friend void releaseReadWrite(FakeMutex& lock) { lock.unlock(); }
+};
+thread_local int FakeMutex::lockCount_{0};
+thread_local int FakeMutex::unlockCount_{0};
+
+// SynchronizedLockTest is used to verify the correct lock unlock behavior
+// happens per design
+class SynchronizedLockTest : public testing::Test {
+ public:
+  void SetUp() override {
+    FakeMutex::resetLockUnlockCount();
+  }
+};
+
+// Single level of SYNCHRONIZED and UNSYNCHRONIZED, although nested test are
+// super set of it, it is possible single level test passes while nested tests
+// fail
+TEST_F(SynchronizedLockTest, SyncUnSync) {
+  folly::Synchronized<std::vector<int>, FakeMutex> obj;
+  EXPECT_EQ((CountPair{0, 0}), FakeMutex::getLockUnlockCount());
+  SYNCHRONIZED(obj) {
+    EXPECT_EQ((CountPair{1, 0}), FakeMutex::getLockUnlockCount());
+    UNSYNCHRONIZED(obj) {
+      EXPECT_EQ((CountPair{1, 1}), FakeMutex::getLockUnlockCount());
+    }
+    EXPECT_EQ((CountPair{2, 1}), FakeMutex::getLockUnlockCount());
+  }
+  EXPECT_EQ((CountPair{2, 2}), FakeMutex::getLockUnlockCount());
+}
+
+// Nested SYNCHRONIZED UNSYNCHRONIZED test, 2 levels for each are used here
+TEST_F(SynchronizedLockTest, NestedSyncUnSync) {
+  folly::Synchronized<std::vector<int>, FakeMutex> obj;
+  EXPECT_EQ((CountPair{0, 0}), FakeMutex::getLockUnlockCount());
+  SYNCHRONIZED(objCopy, obj) {
+    EXPECT_EQ((CountPair{1, 0}), FakeMutex::getLockUnlockCount());
+    SYNCHRONIZED(obj) {
+      EXPECT_EQ((CountPair{2, 0}), FakeMutex::getLockUnlockCount());
+      UNSYNCHRONIZED(obj) {
+        EXPECT_EQ((CountPair{2, 1}), FakeMutex::getLockUnlockCount());
+        UNSYNCHRONIZED(obj) {
+          EXPECT_EQ((CountPair{2, 2}),
+                    FakeMutex::getLockUnlockCount());
+        }
+        EXPECT_EQ((CountPair{3, 2}), FakeMutex::getLockUnlockCount());
+      }
+      EXPECT_EQ((CountPair{4, 2}), FakeMutex::getLockUnlockCount());
+    }
+    EXPECT_EQ((CountPair{4, 3}), FakeMutex::getLockUnlockCount());
+  }
+  EXPECT_EQ((CountPair{4, 4}), FakeMutex::getLockUnlockCount());
+}
+
+// Different nesting behavior, UNSYNCHRONIZED called on differen depth of
+// SYNCHRONIZED
+TEST_F(SynchronizedLockTest, NestedSyncUnSync2) {
+  folly::Synchronized<std::vector<int>, FakeMutex> obj;
+  EXPECT_EQ((CountPair{0, 0}), FakeMutex::getLockUnlockCount());
+  SYNCHRONIZED(objCopy, obj) {
+    EXPECT_EQ((CountPair{1, 0}), FakeMutex::getLockUnlockCount());
+    SYNCHRONIZED(obj) {
+      EXPECT_EQ((CountPair{2, 0}), FakeMutex::getLockUnlockCount());
+      UNSYNCHRONIZED(obj) {
+        EXPECT_EQ((CountPair{2, 1}), FakeMutex::getLockUnlockCount());
+      }
+      EXPECT_EQ((CountPair{3, 1}), FakeMutex::getLockUnlockCount());
+    }
+    EXPECT_EQ((CountPair{3, 2}), FakeMutex::getLockUnlockCount());
+    UNSYNCHRONIZED(obj) {
+      EXPECT_EQ((CountPair{3, 3}), FakeMutex::getLockUnlockCount());
+    }
+    EXPECT_EQ((CountPair{4, 3}), FakeMutex::getLockUnlockCount());
+  }
+  EXPECT_EQ((CountPair{4, 4}), FakeMutex::getLockUnlockCount());
+}
 }