From: Adam Simpkins <simpkins@fb.com>
Date: Wed, 27 Jul 2016 19:32:20 +0000 (-0700)
Subject: add an unlock() method to Synchronized<T>::LockedPtr
X-Git-Tag: v2016.07.29.00~5
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=382372728d0f77c8d430577ea4cf77b53a56a4de;p=folly.git

add an unlock() method to Synchronized<T>::LockedPtr

Summary:
Add an unlock() method to Synchronized LockedPtr objects.  This will make it
easier to replace current users of the UNSYNCHRONIZED macro.

Of the handful of places currently using UNSYNCHRONIZED, many of them want to
simply release the lock before logging a message and returning.  However,
UNSYNCHRONIZED is a poor choice for this, since it will re-acquire the lock on
scope exit.  In these situations where the function returns from inside an
UNSYNCHRONIZED block the code unnecessarily re-acquires the lock just to
immediately release it.  The unlock() method will provide a cleaner mechanism
for these call sites to simply drop the lock early before returning.

Reviewed By: yfeldblum

Differential Revision: D3547652

fbshipit-source-id: 4d28fe9f3aad0d7348e918d1a3d6c705bfec242b
---

diff --git a/folly/Synchronized.h b/folly/Synchronized.h
index 01619bc2..1966087f 100644
--- a/folly/Synchronized.h
+++ b/folly/Synchronized.h
@@ -659,6 +659,20 @@ class LockedPtrBase {
     }
   }
 
+  /**
+   * Unlock the synchronized data.
+   *
+   * The LockedPtr can no longer be dereferenced after unlock() has been
+   * called.  isValid() will return false on an unlocked LockedPtr.
+   *
+   * unlock() can only be called on a LockedPtr that is valid.
+   */
+  void unlock() {
+    DCHECK(parent_ != nullptr);
+    LockPolicy::unlock(parent_->mutex_);
+    parent_ = nullptr;
+  }
+
  protected:
   LockedPtrBase() {}
   explicit LockedPtrBase(SynchronizedType* parent) : parent_(parent) {
@@ -699,6 +713,7 @@ class LockedPtrBase {
   }
 
   UnlockerData releaseLock() {
+    DCHECK(parent_ != nullptr);
     auto current = parent_;
     parent_ = nullptr;
     LockPolicy::unlock(current->mutex_);
@@ -759,6 +774,20 @@ class LockedPtrBase<SynchronizedType, std::mutex, LockPolicy> {
     return lock_;
   }
 
+  /**
+   * Unlock the synchronized data.
+   *
+   * The LockedPtr can no longer be dereferenced after unlock() has been
+   * called.  isValid() will return false on an unlocked LockedPtr.
+   *
+   * unlock() can only be called on a LockedPtr that is valid.
+   */
+  void unlock() {
+    DCHECK(parent_ != nullptr);
+    lock_.unlock();
+    parent_ = nullptr;
+  }
+
  protected:
   LockedPtrBase() {}
   explicit LockedPtrBase(SynchronizedType* parent)
@@ -772,6 +801,7 @@ class LockedPtrBase<SynchronizedType, std::mutex, LockPolicy> {
   }
 
   UnlockerData releaseLock() {
+    DCHECK(parent_ != nullptr);
     UnlockerData data(std::move(lock_), parent_);
     parent_ = nullptr;
     data.first.unlock();
diff --git a/folly/test/SynchronizedTest.cpp b/folly/test/SynchronizedTest.cpp
index de45a404..fb77721e 100644
--- a/folly/test/SynchronizedTest.cpp
+++ b/folly/test/SynchronizedTest.cpp
@@ -63,6 +63,10 @@ TYPED_TEST(SynchronizedTest, WithLock) {
   testWithLock<TypeParam>();
 }
 
+TYPED_TEST(SynchronizedTest, Unlock) {
+  testUnlock<TypeParam>();
+}
+
 TYPED_TEST(SynchronizedTest, Deprecated) {
   testDeprecated<TypeParam>();
 }
diff --git a/folly/test/SynchronizedTestLib-inl.h b/folly/test/SynchronizedTestLib-inl.h
index 99968fd9..faf60f7c 100644
--- a/folly/test/SynchronizedTestLib-inl.h
+++ b/folly/test/SynchronizedTestLib-inl.h
@@ -42,6 +42,7 @@ inline std::mt19937& getRNG() {
 void randomSleep(std::chrono::milliseconds min, std::chrono::milliseconds max) {
   std::uniform_int_distribution<> range(min.count(), max.count());
   std::chrono::milliseconds duration(range(getRNG()));
+  /* sleep override */
   std::this_thread::sleep_for(duration);
 }
 
@@ -354,6 +355,104 @@ testWithLock() {
   });
 }
 
+template <class Mutex>
+void testUnlockCommon() {
+  folly::Synchronized<int, Mutex> value{7};
+  const auto& cv = value;
+
+  {
+    auto lv = value.contextualLock();
+    EXPECT_EQ(7, *lv);
+    *lv = 5;
+    lv.unlock();
+    EXPECT_TRUE(lv.isNull());
+    EXPECT_FALSE(lv);
+
+    auto rlv = cv.contextualLock();
+    EXPECT_EQ(5, *rlv);
+    rlv.unlock();
+    EXPECT_TRUE(rlv.isNull());
+    EXPECT_FALSE(rlv);
+
+    auto rlv2 = cv.contextualRLock();
+    EXPECT_EQ(5, *rlv2);
+    rlv2.unlock();
+
+    lv = value.contextualLock();
+    EXPECT_EQ(5, *lv);
+    *lv = 9;
+  }
+
+  EXPECT_EQ(9, *value.contextualRLock());
+}
+
+// testUnlock() version for shared lock types
+template <class Mutex>
+typename std::enable_if<folly::LockTraits<Mutex>::is_shared>::type
+testUnlock() {
+  folly::Synchronized<int, Mutex> value{10};
+  {
+    auto lv = value.wlock();
+    EXPECT_EQ(10, *lv);
+    *lv = 5;
+    lv.unlock();
+    EXPECT_FALSE(lv);
+    EXPECT_TRUE(lv.isNull());
+
+    auto rlv = value.rlock();
+    EXPECT_EQ(5, *rlv);
+    rlv.unlock();
+    EXPECT_FALSE(rlv);
+    EXPECT_TRUE(rlv.isNull());
+
+    auto lv2 = value.wlock();
+    EXPECT_EQ(5, *lv2);
+    *lv2 = 7;
+
+    lv = std::move(lv2);
+    EXPECT_FALSE(lv2);
+    EXPECT_TRUE(lv2.isNull());
+    EXPECT_FALSE(lv.isNull());
+    EXPECT_EQ(7, *lv);
+  }
+
+  testUnlockCommon<Mutex>();
+}
+
+// testUnlock() version for non-shared lock types
+template <class Mutex>
+typename std::enable_if<!folly::LockTraits<Mutex>::is_shared>::type
+testUnlock() {
+  folly::Synchronized<int, Mutex> value{10};
+  {
+    auto lv = value.lock();
+    EXPECT_EQ(10, *lv);
+    *lv = 5;
+    lv.unlock();
+    EXPECT_TRUE(lv.isNull());
+    EXPECT_FALSE(lv);
+
+    auto lv2 = value.lock();
+    EXPECT_EQ(5, *lv2);
+    *lv2 = 6;
+    lv2.unlock();
+    EXPECT_TRUE(lv2.isNull());
+    EXPECT_FALSE(lv2);
+
+    lv = value.lock();
+    EXPECT_EQ(6, *lv);
+    *lv = 7;
+
+    lv2 = std::move(lv);
+    EXPECT_TRUE(lv.isNull());
+    EXPECT_FALSE(lv);
+    EXPECT_FALSE(lv2.isNull());
+    EXPECT_EQ(7, *lv2);
+  }
+
+  testUnlockCommon<Mutex>();
+}
+
 // Testing the deprecated SYNCHRONIZED and SYNCHRONIZED_CONST APIs
 template <class Mutex>
 void testDeprecated() {