Make LockedPtr assignable

Summary:
Currently each of the lock proxies have distinct types and they are not
convertible from one to the other, for example the code below does not work

```
auto wlock = sync.wlock();
wlock.unlock();

auto ulock = sync.ulock();
wlock = ulock.moveFromUpgradeToWrite();
```

Reviewed By: yfeldblum

Differential Revision: D7658256

fbshipit-source-id: 1746a67193fc712fd4d4ff2ce41aa295f998211e

folly/LockTraits.h

@@ -348,6 +348,48 @@ struct LockTraitsImpl<Mutex, MutexLevel::UPGRADE, true>
   }
 };
 
+/**
+ * Unlock helpers
+ *
+ * These help in determining whether it is safe for Synchronized::LockedPtr
+ * instances to be move assigned from one another.  It is safe if they both
+ * have the same unlock policy, and it is not if they don't have the same
+ * unlock policy.  For example
+ *
+ *    auto wlock = synchronized.wlock();
+ *    wlock.unlock();
+ *
+ *    wlock = synchronized.rlock();
+ *
+ * This code would try to release the shared lock with a call to unlock(),
+ * resulting in possibly undefined behavior.  By allowing the LockPolicy
+ * classes (defined below) to know what their unlocking behavior is, we can
+ * prevent against this by disabling unsafe conversions to and from
+ * incompatible LockedPtr types (they are incompatible if the underlying
+ * LockPolicy has different unlock policies.
+ */
+template <template <typename...> class LockTraits>
+struct UnlockPolicyExclusive {
+  template <typename Mutex>
+  static void unlock(Mutex& mutex) {
+    LockTraits<Mutex>::unlock(mutex);
+  }
+};
+template <template <typename...> class LockTraits>
+struct UnlockPolicyShared {
+  template <typename Mutex>
+  static void unlock(Mutex& mutex) {
+    LockTraits<Mutex>::unlock_shared(mutex);
+  }
+};
+template <template <typename...> class LockTraits>
+struct UnlockPolicyUpgrade {
+  template <typename Mutex>
+  static void unlock(Mutex& mutex) {
+    LockTraits<Mutex>::unlock_upgrade(mutex);
+  }
+};
+
 } // namespace detail
 
 /**
@@ -420,11 +462,12 @@ struct LockTraits : public LockTraitsBase<Mutex> {};
  * These can be used as template parameters to provide compile-time
  * selection over the type of lock operation to perform.
  */
-
 /**
  * A lock policy that performs exclusive lock operations.
  */
-struct LockPolicyExclusive {
+struct LockPolicyExclusive : detail::UnlockPolicyExclusive<LockTraits> {
+  using UnlockPolicy = detail::UnlockPolicyExclusive<LockTraits>;
+
   template <class Mutex>
   static std::true_type lock(Mutex& mutex) {
     LockTraits<Mutex>::lock(mutex);
@@ -436,17 +479,15 @@ struct LockPolicyExclusive {
       const std::chrono::duration<Rep, Period>& timeout) {
     return LockTraits<Mutex>::try_lock_for(mutex, timeout);
   }
-  template <class Mutex>
-  static void unlock(Mutex& mutex) {
-    LockTraits<Mutex>::unlock(mutex);
-  }
 };
 
 /**
  * A lock policy that performs shared lock operations.
  * This policy only works with shared mutex types.
  */
-struct LockPolicyShared {
+struct LockPolicyShared : detail::UnlockPolicyShared<LockTraits> {
+  using UnlockPolicy = detail::UnlockPolicyShared<LockTraits>;
+
   template <class Mutex>
   static std::true_type lock(Mutex& mutex) {
     LockTraits<Mutex>::lock_shared(mutex);
@@ -458,10 +499,6 @@ struct LockPolicyShared {
       const std::chrono::duration<Rep, Period>& timeout) {
     return LockTraits<Mutex>::try_lock_shared_for(mutex, timeout);
   }
-  template <class Mutex>
-  static void unlock(Mutex& mutex) {
-    LockTraits<Mutex>::unlock_shared(mutex);
-  }
 };
 
 /**
@@ -471,7 +508,9 @@ struct LockPolicyShared {
  *  unlock() -> unlock_upgrade()
  *  try_lock_for -> try_lock_upgrade_for()
  */
-struct LockPolicyUpgrade {
+struct LockPolicyUpgrade : detail::UnlockPolicyUpgrade<LockTraits> {
+  using UnlockPolicy = detail::UnlockPolicyUpgrade<LockTraits>;
+
   template <class Mutex>
   static std::true_type lock(Mutex& mutex) {
     LockTraits<Mutex>::lock_upgrade(mutex);
@@ -483,10 +522,6 @@ struct LockPolicyUpgrade {
       const std::chrono::duration<Rep, Period>& timeout) {
     return LockTraits<Mutex>::try_lock_upgrade_for(mutex, timeout);
   }
-  template <class Mutex>
-  static void unlock(Mutex& mutex) {
-    LockTraits<Mutex>::unlock_upgrade(mutex);
-  }
 };
 
 /*****************************************************************************
@@ -496,45 +531,39 @@ struct LockPolicyUpgrade {
  * A lock policy that tries to acquire write locks and returns true or false
  * based on whether the lock operation succeeds
  */
-struct LockPolicyTryExclusive {
+struct LockPolicyTryExclusive : detail::UnlockPolicyExclusive<LockTraits> {
+  using UnlockPolicy = detail::UnlockPolicyExclusive<LockTraits>;
+
   template <class Mutex>
   static bool lock(Mutex& mutex) {
     return LockTraits<Mutex>::try_lock(mutex);
   }
-  template <class Mutex>
-  static void unlock(Mutex& mutex) {
-    LockTraits<Mutex>::unlock(mutex);
-  }
 };
 
 /**
  * A lock policy that tries to acquire a read lock and returns true or false
  * based on whether the lock operation succeeds
  */
-struct LockPolicyTryShared {
+struct LockPolicyTryShared : detail::UnlockPolicyShared<LockTraits> {
+  using UnlockPolicy = detail::UnlockPolicyShared<LockTraits>;
+
   template <class Mutex>
   static bool lock(Mutex& mutex) {
     return LockTraits<Mutex>::try_lock_shared(mutex);
   }
-  template <class Mutex>
-  static void unlock(Mutex& mutex) {
-    LockTraits<Mutex>::unlock_shared(mutex);
-  }
 };
 
 /**
  * A lock policy that tries to acquire an upgrade lock and returns true or
  * false based on whether the lock operation succeeds
  */
-struct LockPolicyTryUpgrade {
+struct LockPolicyTryUpgrade : detail::UnlockPolicyUpgrade<LockTraits> {
+  using UnlockPolicy = detail::UnlockPolicyUpgrade<LockTraits>;
+
   template <class Mutex>
   static bool lock(Mutex& mutex) {
     return LockTraits<Mutex>::try_lock_upgrade(mutex);
   }
-  template <class Mutex>
-  static void unlock(Mutex& mutex) {
-    LockTraits<Mutex>::unlock_upgrade(mutex);
-  }
 };
 
 /*****************************************************************************
@@ -547,7 +576,7 @@ struct LockPolicyTryUpgrade {
  *  unlock() -> unlock()
  *  try_lock_for -> try_unlock_upgrade_and_lock_for()
  */
-struct LockPolicyFromUpgradeToExclusive : public LockPolicyExclusive {
+struct LockPolicyFromUpgradeToExclusive : LockPolicyExclusive {
   template <class Mutex>
   static std::true_type lock(Mutex& mutex) {
     LockTraits<Mutex>::unlock_upgrade_and_lock(mutex);
@@ -568,7 +597,7 @@ struct LockPolicyFromUpgradeToExclusive : public LockPolicyExclusive {
  *  unlock() -> unlock_upgrade()
  *  try_lock_for -> unlock_and_lock_upgrade()
  */
-struct LockPolicyFromExclusiveToUpgrade : public LockPolicyUpgrade {
+struct LockPolicyFromExclusiveToUpgrade : LockPolicyUpgrade {
   template <class Mutex>
   static std::true_type lock(Mutex& mutex) {
     LockTraits<Mutex>::unlock_and_lock_upgrade(mutex);
@@ -592,7 +621,7 @@ struct LockPolicyFromExclusiveToUpgrade : public LockPolicyUpgrade {
  *  unlock() -> unlock_shared()
  *  try_lock_for -> unlock_upgrade_and_lock_shared()
  */
-struct LockPolicyFromUpgradeToShared : public LockPolicyShared {
+struct LockPolicyFromUpgradeToShared : LockPolicyShared {
   template <class Mutex>
   static std::true_type lock(Mutex& mutex) {
     LockTraits<Mutex>::unlock_upgrade_and_lock_shared(mutex);
@@ -616,7 +645,7 @@ struct LockPolicyFromUpgradeToShared : public LockPolicyShared {
  *  unlock() -> unlock_shared()
  *  try_lock_for() -> unlock_and_lock_shared()
  */
-struct LockPolicyFromExclusiveToShared : public LockPolicyShared {
+struct LockPolicyFromExclusiveToShared : LockPolicyShared {
   template <class Mutex>
   static std::true_type lock(Mutex& mutex) {
     LockTraits<Mutex>::unlock_and_lock_shared(mutex);

folly/Synchronized.h

@@ -892,6 +892,14 @@ class LockedPtrBase {
   using MutexType = Mutex;
   friend class folly::ScopedUnlocker<SynchronizedType, LockPolicy>;
 
+  /**
+   * Friend all instantiations of LockedPtr and LockedPtrBase
+   */
+  template <typename S, typename L>
+  friend class folly::LockedPtr;
+  template <typename S, typename M, typename L>
+  friend class LockedPtrBase;
+
   /**
    * Destructor releases.
    */
@@ -931,19 +939,50 @@ class LockedPtrBase {
       this->parent_ = parent;
     }
   }
-  LockedPtrBase(LockedPtrBase&& rhs) noexcept : parent_(rhs.parent_) {
-    rhs.parent_ = nullptr;
-  }
+  LockedPtrBase(LockedPtrBase&& rhs) noexcept
+      : parent_{exchange(rhs.parent_, nullptr)} {}
   LockedPtrBase& operator=(LockedPtrBase&& rhs) noexcept {
-    if (parent_) {
-      LockPolicy::unlock(parent_->mutex_);
-    }
+    assignImpl(*this, rhs);
+    return *this;
+  }
 
-    parent_ = rhs.parent_;
-    rhs.parent_ = nullptr;
+  /**
+   * Templated move construct and assignment operators
+   *
+   * These allow converting LockedPtr types that have the same unlocking
+   * policy to each other.  This allows us to write code like
+   *
+   *  auto wlock = sync.wlock();
+   *  wlock.unlock();
+   *
+   *  auto ulock = sync.ulock();
+   *  wlock = ulock.moveFromUpgradeToWrite();
+   */
+  template <typename LockPolicyType>
+  LockedPtrBase(
+      LockedPtrBase<SynchronizedType, Mutex, LockPolicyType>&& rhs) noexcept
+      : parent_{exchange(rhs.parent_, nullptr)} {}
+  template <typename LockPolicyType>
+  LockedPtrBase& operator=(
+      LockedPtrBase<SynchronizedType, Mutex, LockPolicyType>&& rhs) noexcept {
+    assignImpl(*this, rhs);
     return *this;
   }
 
+  /**
+   * Implementation for the assignment operator
+   */
+  template <typename LockPolicyLhs, typename LockPolicyRhs>
+  void assignImpl(
+      LockedPtrBase<SynchronizedType, Mutex, LockPolicyLhs>& lhs,
+      LockedPtrBase<SynchronizedType, Mutex, LockPolicyRhs>& rhs) noexcept {
+    if (lhs.parent_) {
+      LockPolicy::unlock(lhs.parent_->mutex_);
+    }
+
+    lhs.parent_ = exchange(rhs.parent_, nullptr);
+  }
+
   using UnlockerData = SynchronizedType*;
 
   /**
@@ -986,6 +1025,14 @@ class LockedPtrBase<SynchronizedType, std::mutex, LockPolicy> {
   using MutexType = std::mutex;
   friend class folly::ScopedUnlocker<SynchronizedType, LockPolicy>;
 
+  /**
+   * Friend all instantiations of LockedPtr and LockedPtrBase
+   */
+  template <typename S, typename L>
+  friend class folly::LockedPtr;
+  template <typename S, typename M, typename L>
+  friend class LockedPtrBase;
+
   /**
    * Destructor releases.
    */
@@ -995,14 +1042,43 @@ class LockedPtrBase<SynchronizedType, std::mutex, LockPolicy> {
   }
 
   LockedPtrBase(LockedPtrBase&& rhs) noexcept
-      : lock_(std::move(rhs.lock_)), parent_(exchange(rhs.parent_, nullptr)) {}
+      : lock_{std::move(rhs.lock_)}, parent_{exchange(rhs.parent_, nullptr)} {}
   LockedPtrBase& operator=(LockedPtrBase&& rhs) noexcept {
-    lock_ = std::move(rhs.lock_);
-    parent_ = rhs.parent_;
-    rhs.parent_ = nullptr;
+    assignImpl(*this, rhs);
+    return *this;
+  }
+
+  /**
+   * Templated move construct and assignment operators
+   *
+   * These allow converting LockedPtr types that have the same unlocking
+   * policy to each other.
+   */
+  template <typename LockPolicyType>
+  LockedPtrBase(LockedPtrBase<SynchronizedType, std::mutex, LockPolicyType>&&
+                    other) noexcept
+      : lock_{std::move(other.lock_)},
+        parent_{exchange(other.parent_, nullptr)} {}
+  template <typename LockPolicyType>
+  LockedPtrBase& operator=(
+      LockedPtrBase<SynchronizedType, std::mutex, LockPolicyType>&&
+          rhs) noexcept {
+    assignImpl(*this, rhs);
     return *this;
   }
 
+  /**
+   * Implementation for the assignment operator
+   */
+  template <typename LockPolicyLhs, typename LockPolicyRhs>
+  void assignImpl(
+      LockedPtrBase<SynchronizedType, std::mutex, LockPolicyLhs>& lhs,
+      LockedPtrBase<SynchronizedType, std::mutex, LockPolicyRhs>&
+          rhs) noexcept {
+    lhs.lock_ = std::move(rhs.lock_);
+    lhs.parent_ = exchange(rhs.parent_, nullptr);
+  }
+
   /**
    * Get a reference to the std::unique_lock.
    *
@@ -1125,6 +1201,10 @@ class LockedPtr : public LockedPtrBase<
   // CDataType is the DataType with the appropriate const-qualification
   using CDataType = detail::SynchronizedDataType<SynchronizedType>;
 
+  // friend other LockedPtr types
+  template <typename SynchronizedTypeOther, typename LockPolicyOther>
+  friend class LockedPtr;
+
  public:
   using DataType = typename SynchronizedType::DataType;
   using MutexType = typename SynchronizedType::MutexType;
@@ -1167,6 +1247,19 @@ class LockedPtr : public LockedPtrBase<
    */
   LockedPtr& operator=(LockedPtr&& rhs) noexcept = default;
 
+  template <
+      typename LockPolicyType,
+      typename std::enable_if<
+          std::is_same<
+              typename LockPolicy::UnlockPolicy,
+              typename LockPolicyType::UnlockPolicy>::value,
+          int>::type = 0>
+  LockedPtr& operator=(
+      LockedPtr<SynchronizedType, LockPolicyType>&& other) noexcept {
+    Base::operator=(std::move(other));
+    return *this;
+  }
+
   /*
    * Copy constructor and assignment operator are deleted.
    */

folly/test/LockTraitsTest.cpp

@@ -373,3 +373,36 @@ TEST(LockTraits, LockPolicyTimed) {
                           "to shared";
   mutex.unlock_shared();
 }
+
+/**
+ * Test compatibility of the different lock policies
+ *
+ * This should be correct because the compatibilities here are used to
+ * determine whether or not the different LockedPtr instances can be moved
+ * from each other
+ */
+TEST(LockTraits, LockPolicyCompatibilities) {
+  EXPECT_TRUE((std::is_same<
+               LockPolicyExclusive::UnlockPolicy,
+               LockPolicyTryExclusive::UnlockPolicy>::value));
+  EXPECT_TRUE((std::is_same<
+               LockPolicyExclusive::UnlockPolicy,
+               LockPolicyFromUpgradeToExclusive::UnlockPolicy>::value));
+
+  EXPECT_TRUE((std::is_same<
+               LockPolicyShared::UnlockPolicy,
+               LockPolicyTryShared::UnlockPolicy>::value));
+  EXPECT_TRUE((std::is_same<
+               LockPolicyShared::UnlockPolicy,
+               LockPolicyFromUpgradeToShared::UnlockPolicy>::value));
+  EXPECT_TRUE((std::is_same<
+               LockPolicyShared::UnlockPolicy,
+               LockPolicyFromExclusiveToShared::UnlockPolicy>::value));
+
+  EXPECT_TRUE((std::is_same<
+               LockPolicyUpgrade::UnlockPolicy,
+               LockPolicyTryUpgrade::UnlockPolicy>::value));
+  EXPECT_TRUE((std::is_same<
+               LockPolicyUpgrade::UnlockPolicy,
+               LockPolicyFromExclusiveToUpgrade::UnlockPolicy>::value));
+}

folly/test/SynchronizedTest.cpp

@@ -688,4 +688,113 @@ TEST_F(SynchronizedLockTest, TestTryULock) {
       [](auto& synchronized) { return synchronized.tryULock(); });
 }
 
+template <typename LockPolicy>
+using LPtr = LockedPtr<Synchronized<int>, LockPolicy>;
+
+TEST_F(SynchronizedLockTest, TestLockedPtrCompatibilityExclusive) {
+  EXPECT_TRUE((std::is_assignable<
+               LPtr<LockPolicyExclusive>&,
+               LPtr<LockPolicyTryExclusive>&&>::value));
+  EXPECT_TRUE((std::is_assignable<
+               LPtr<LockPolicyExclusive>&,
+               LPtr<LockPolicyFromUpgradeToExclusive>&&>::value));
+
+  EXPECT_FALSE((
+      std::is_assignable<LPtr<LockPolicyExclusive>&, LPtr<LockPolicyShared>&&>::
+          value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyExclusive>&,
+                LPtr<LockPolicyTryShared>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyExclusive>&,
+                LPtr<LockPolicyUpgrade>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyExclusive>&,
+                LPtr<LockPolicyTryUpgrade>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyExclusive>&,
+                LPtr<LockPolicyFromExclusiveToUpgrade>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyExclusive>&,
+                LPtr<LockPolicyFromExclusiveToShared>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyExclusive>&,
+                LPtr<LockPolicyFromUpgradeToShared>&&>::value));
+}
+
+TEST_F(SynchronizedLockTest, TestLockedPtrCompatibilityShared) {
+  EXPECT_TRUE((
+      std::is_assignable<LPtr<LockPolicyShared>&, LPtr<LockPolicyTryShared>&&>::
+          value));
+  EXPECT_TRUE((std::is_assignable<
+               LPtr<LockPolicyShared>&,
+               LPtr<LockPolicyFromUpgradeToShared>&&>::value));
+  EXPECT_TRUE((std::is_assignable<
+               LPtr<LockPolicyShared>&,
+               LPtr<LockPolicyFromExclusiveToShared>&&>::value));
+
+  EXPECT_FALSE((
+      std::is_assignable<LPtr<LockPolicyShared>&, LPtr<LockPolicyExclusive>&&>::
+          value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyShared>&,
+                LPtr<LockPolicyTryExclusive>&&>::value));
+  EXPECT_FALSE(
+      (std::is_assignable<LPtr<LockPolicyShared>&, LPtr<LockPolicyUpgrade>&&>::
+           value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyShared>&,
+                LPtr<LockPolicyTryUpgrade>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyShared>&,
+                LPtr<LockPolicyFromExclusiveToUpgrade>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyShared>&,
+                LPtr<LockPolicyFromUpgradeToExclusive>&&>::value));
+}
+
+TEST_F(SynchronizedLockTest, TestLockedPtrCompatibilityUpgrade) {
+  EXPECT_TRUE((std::is_assignable<
+               LPtr<LockPolicyUpgrade>&,
+               LPtr<LockPolicyTryUpgrade>&&>::value));
+  EXPECT_TRUE((std::is_assignable<
+               LPtr<LockPolicyUpgrade>&,
+               LPtr<LockPolicyFromExclusiveToUpgrade>&&>::value));
+
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyUpgrade>&,
+                LPtr<LockPolicyExclusive>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyUpgrade>&,
+                LPtr<LockPolicyTryExclusive>&&>::value));
+  EXPECT_FALSE(
+      (std::is_assignable<LPtr<LockPolicyUpgrade>&, LPtr<LockPolicyShared>&&>::
+           value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyUpgrade>&,
+                LPtr<LockPolicyTryShared>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyUpgrade>&,
+                LPtr<LockPolicyFromExclusiveToShared>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyUpgrade>&,
+                LPtr<LockPolicyFromUpgradeToShared>&&>::value));
+  EXPECT_FALSE((std::is_assignable<
+                LPtr<LockPolicyUpgrade>&,
+                LPtr<LockPolicyFromUpgradeToExclusive>&&>::value));
+}
+
+TEST_F(SynchronizedLockTest, TestConvertTryLockToLock) {
+  auto synchronized = folly::Synchronized<int>{0};
+  auto wlock = synchronized.wlock();
+  wlock.unlock();
+
+  auto ulock = synchronized.ulock();
+  wlock = ulock.moveFromUpgradeToWrite();
+  wlock.unlock();
+
+  auto value = synchronized.withWLock([](auto& integer) { return integer; });
+  EXPECT_EQ(value, 0);
+}
+
 } // namespace folly