From: Andrew Birchall Date: Thu, 28 Apr 2016 23:39:10 +0000 (-0700) Subject: Create ReadHolder::unlock X-Git-Tag: 2016.07.26~303 X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=95e7a3bad41357c9f78d0a993b542b002f17e00e;p=folly.git 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 782a6536..0bb62037 100644 --- 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 3710709e..70661b5e 100644 --- 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);