Create ReadHolder::unlock

Summary:
Currently you need to depend on the destructor of `ReadHolder` (using closures as in code block #1 below or empty assignment as in code block #2 below) to ensure that a `ReadHolder` goes out of scope (and unlocks) in order to subsequently acquire a write lock via `WriteHolder` without deadlocking.
This diff introduces a way of unlocking a `ReadHolder` while it's still in scope such that a `WriteHolder` can be acquired. This makes the code more straight forward (reducing the risk of deadlock due to a programmer's misunderstanding of how `SharedMutex` / the holder framework works) => see code block # 3 below
Also add some documentation about why `WriteHolder::WriteHolder(ReadHolder&&)` doesn't exist

Code Block #1 : Use of closures
```
class foo {
 public:
  std::string getMemoizedData() {
    {
      folly::SharedMutex::ReadHolder readHolder(lock_);
      if (!data_.empty()) {
        // important to return by value, otherwise caller might access
        // data_ after we release the read lock
        return data_;
      }
    }
    {
      // try again with a write lock
      folly::SharedMutex::WriteHolder writeHolder(lock_);
      if (data_.empty()) {
        data_ = "my awesome string";
      }
      return data_;
    }
  }

 private:
  folly::SharedMutex lock_;
  std::string data_;
};
```

Code Block #2 : Use of empty assignment
```
class foo {
 public:
  std::string getMemoizedData() {
    folly::SharedMutex::ReadHolder readHolder(lock_);
    if (!data_.empty()) {
      // important to return by value, otherwise caller might access
      // data_ after we release the read lock
      return data_;
    }
    readHolder = {};

    // try again with a write lock
    folly::SharedMutex::WriteHolder writeHolder(lock_);
    if (data_.empty()) {
      data_ = "my awesome string";
    }
    return data_;
  }

 private:
  folly::SharedMutex lock_;
  std::string data_;
};
```

Code Block #3 : Use of unlock()
```
class foo {
 public:
  std::string getMemoizedData() {
    folly::SharedMutex::ReadHolder readHolder(lock_);
    if (!data_.empty()) {
      // important to return by value, otherwise caller might access
      // data_ after we release the read lock
      return data_;
    }
    readHolder->unlock();

    // try again with a write lock
    folly::SharedMutex::WriteHolder writeHolder(lock_);
    if (data_.empty()) {
      data_ = "my awesome string";
    }
    return data_;
  }

 private:
  folly::SharedMutex lock_;
  std::string data_;
};
```

Reviewed By: yfeldblum

Differential Revision: D3176025

fb-gh-sync-id: c7d47ca71df08673c7c1f1fd5ed9e01a663c1797
fbshipit-source-id: c7d47ca71df08673c7c1f1fd5ed9e01a663c1797

diff --git a/folly/SharedMutex.h b/folly/SharedMutex.h
index 782a6536636c46fd55804aae909e71d97b39396a..0bb62037bf22549912aaf47089406fca47a309ac 100644 (file)
--- a/folly/SharedMutex.h
+++ b/folly/SharedMutex.h
@@ -1283,8 +1283,13 @@ class SharedMutexImpl {
     ReadHolder& operator=(const ReadHolder& rhs) = delete;
 
     ~ReadHolder() {
+      unlock();
+    }
+
+    void unlock() {
       if (lock_) {
         lock_->unlock_shared(token_);
+        lock_ = nullptr;
       }
     }
 
@@ -1325,8 +1330,13 @@ class SharedMutexImpl {
     UpgradeHolder& operator=(const UpgradeHolder& rhs) = delete;
 
     ~UpgradeHolder() {
+      unlock();
+    }
+
+    void unlock() {
       if (lock_) {
         lock_->unlock_upgrade();
+        lock_ = nullptr;
       }
     }
 
@@ -1353,6 +1363,30 @@ class SharedMutexImpl {
       lock_->unlock_upgrade_and_lock();
     }
 
+    // README:
+    //
+    // It is intended that WriteHolder(ReadHolder&& rhs) do not exist.
+    //
+    // Shared locks (read) can not safely upgrade to unique locks (write).
+    // That upgrade path is a well-known recipe for deadlock, so we explicitly
+    // disallow it.
+    //
+    // If you need to do a conditional mutation, you have a few options:
+    // 1. Check the condition under a shared lock and release it.
+    //    Then maybe check the condition again under a unique lock and maybe do
+    //    the mutation.
+    // 2. Check the condition once under an upgradeable lock.
+    //    Then maybe upgrade the lock to a unique lock and do the mutation.
+    // 3. Check the condition and maybe perform the mutation under a unique
+    //    lock.
+    //
+    // Relevant upgradeable lock notes:
+    // * At most one upgradeable lock can be held at a time for a given shared
+    //   mutex, just like a unique lock.
+    // * An upgradeable lock may be held concurrently with any number of shared
+    //   locks.
+    // * An upgradeable lock may be upgraded atomically to a unique lock.
+
     WriteHolder(WriteHolder&& rhs) noexcept : lock_(rhs.lock_) {
       rhs.lock_ = nullptr;
     }
@@ -1366,8 +1400,13 @@ class SharedMutexImpl {
     WriteHolder& operator=(const WriteHolder& rhs) = delete;
 
     ~WriteHolder() {
+      unlock();
+    }
+
+    void unlock() {
       if (lock_) {
         lock_->unlock();
+        lock_ = nullptr;
       }
     }
 

diff --git a/folly/test/SharedMutexTest.cpp b/folly/test/SharedMutexTest.cpp
index 3710709ec29fdafa04f89e3d4947614b20722d5d..70661b5e6fa965af87846ed2a354d83e8d9cbcbf 100644 (file)
--- a/folly/test/SharedMutexTest.cpp
+++ b/folly/test/SharedMutexTest.cpp
@@ -92,19 +92,39 @@ void runBasicHoldersTest() {
   SharedMutexToken token;
 
   {
+    // create an exclusive write lock via holder
     typename Lock::WriteHolder holder(lock);
     EXPECT_FALSE(lock.try_lock());
     EXPECT_FALSE(lock.try_lock_shared(token));
 
+    // move ownership to another write holder via move constructor
     typename Lock::WriteHolder holder2(std::move(holder));
+    EXPECT_FALSE(lock.try_lock());
+    EXPECT_FALSE(lock.try_lock_shared(token));
+
+    // move ownership to another write holder via assign operator
     typename Lock::WriteHolder holder3;
     holder3 = std::move(holder2);
+    EXPECT_FALSE(lock.try_lock());
+    EXPECT_FALSE(lock.try_lock_shared(token));
 
+    // downgrade from exclusive to upgrade lock via move constructor
     typename Lock::UpgradeHolder holder4(std::move(holder3));
+
+    // ensure we can lock from a shared source
+    EXPECT_FALSE(lock.try_lock());
+    EXPECT_TRUE(lock.try_lock_shared(token));
+    lock.unlock_shared(token);
+
+    // promote from upgrade to exclusive lock via move constructor
     typename Lock::WriteHolder holder5(std::move(holder4));
+    EXPECT_FALSE(lock.try_lock());
+    EXPECT_FALSE(lock.try_lock_shared(token));
 
+    // downgrade exclusive to shared lock via move constructor
     typename Lock::ReadHolder holder6(std::move(holder5));
 
+    // ensure we can lock from another shared source
     EXPECT_FALSE(lock.try_lock());
     EXPECT_TRUE(lock.try_lock_shared(token));
     lock.unlock_shared(token);