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 01619bc2e0f0046e7c158ed7f08a95b8cb3d1396..1966087f844acfe7e3167bae31c740b8e03676fd 100644 (file)
--- 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) {
  protected:
   LockedPtrBase() {}
   explicit LockedPtrBase(SynchronizedType* parent) : parent_(parent) {


@@ -699,6 +713,7 @@ class LockedPtrBase {
   }
 
   UnlockerData releaseLock() {
   }
 
   UnlockerData releaseLock() {

+    DCHECK(parent_ != nullptr);

     auto current = parent_;
     parent_ = nullptr;
     LockPolicy::unlock(current->mutex_);
     auto current = parent_;
     parent_ = nullptr;
     LockPolicy::unlock(current->mutex_);


@@ -759,6 +774,20 @@ class LockedPtrBase<SynchronizedType, std::mutex, LockPolicy> {
     return lock_;
   }
 
     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)
  protected:
   LockedPtrBase() {}
   explicit LockedPtrBase(SynchronizedType* parent)


@@ -772,6 +801,7 @@ class LockedPtrBase<SynchronizedType, std::mutex, LockPolicy> {
   }
 
   UnlockerData releaseLock() {
   }
 
   UnlockerData releaseLock() {

+    DCHECK(parent_ != nullptr);

     UnlockerData data(std::move(lock_), parent_);
     parent_ = nullptr;
     data.first.unlock();
     UnlockerData data(std::move(lock_), parent_);
     parent_ = nullptr;
     data.first.unlock();

diff --git a/folly/test/SynchronizedTest.cpp b/folly/test/SynchronizedTest.cpp
index de45a4045b97d237dec722a4654f146fb99c9262..fb77721eeb0975167871c80148723dd258dfb428 100644 (file)
--- a/folly/test/SynchronizedTest.cpp
+++ b/folly/test/SynchronizedTest.cpp
@@ -63,6 +63,10 @@ TYPED_TEST(SynchronizedTest, WithLock) {
   testWithLock<TypeParam>();
 }
 
   testWithLock<TypeParam>();
 }
 

+TYPED_TEST(SynchronizedTest, Unlock) {
+  testUnlock<TypeParam>();
+}
+

 TYPED_TEST(SynchronizedTest, Deprecated) {
   testDeprecated<TypeParam>();
 }
 TYPED_TEST(SynchronizedTest, Deprecated) {
   testDeprecated<TypeParam>();
 }

diff --git a/folly/test/SynchronizedTestLib-inl.h b/folly/test/SynchronizedTestLib-inl.h
index 99968fd93ac0795b3a8313527b6bf1b204aa79a3..faf60f7cb93176197f2b3f9b739d152dcd6c9346 100644 (file)
--- 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()));
 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);
 }
 
   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() {
 // Testing the deprecated SYNCHRONIZED and SYNCHRONIZED_CONST APIs
 template <class Mutex>
 void testDeprecated() {