add an unlock() method to Synchronized<T>::LockedPtr
authorAdam Simpkins <simpkins@fb.com>
Wed, 27 Jul 2016 19:32:20 +0000 (12:32 -0700)
committerFacebook Github Bot 7 <facebook-github-bot-7-bot@fb.com>
Wed, 27 Jul 2016 19:38:32 +0000 (12:38 -0700)
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

folly/Synchronized.h
folly/test/SynchronizedTest.cpp
folly/test/SynchronizedTestLib-inl.h

index 01619bc..1966087 100644 (file)
@@ -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();
index de45a40..fb77721 100644 (file)
@@ -63,6 +63,10 @@ TYPED_TEST(SynchronizedTest, WithLock) {
   testWithLock<TypeParam>();
 }
 
+TYPED_TEST(SynchronizedTest, Unlock) {
+  testUnlock<TypeParam>();
+}
+
 TYPED_TEST(SynchronizedTest, Deprecated) {
   testDeprecated<TypeParam>();
 }
index 99968fd..faf60f7 100644 (file)
@@ -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() {