From 79212dac968aad6e2a9b8fb1320daad6faf49f4a Mon Sep 17 00:00:00 2001 From: Aaryaman Sagar Date: Thu, 18 Aug 2016 19:32:18 -0700 Subject: [PATCH] Adding upgradable locks to Synchronized Summary: This diff adds support for upgradeable locks to folly::Synchronized Reviewed By: yfeldblum Differential Revision: D3683205 fbshipit-source-id: 1b91ab07076566b4e5b535f2a2dbe1c8d9f3d1c2 --- folly/Synchronized.h | 192 +++++++++++++++++++- folly/test/SynchronizedTest.cpp | 312 ++++++++++++++++++++++++++++++++ 2 files changed, 499 insertions(+), 5 deletions(-) diff --git a/folly/Synchronized.h b/folly/Synchronized.h index 10c711ca..beef1b75 100644 --- a/folly/Synchronized.h +++ b/folly/Synchronized.h @@ -25,6 +25,7 @@ #pragma once +#include #include #include #include @@ -42,13 +43,30 @@ class LockedPtr; template class LockedGuardPtr; +/** + * Public version of LockInterfaceDispatcher that contains the MutexLevel enum + * for the passed in mutex type + * + * This is decoupled from MutexLevelValueImpl in LockTraits.h because this + * ensures that a heterogenous mutex with a different API can be used. For + * example - if a mutex does not have a lock_shared() method but the + * LockTraits specialization for it supports a static non member + * lock_shared(Mutex&) it can be used as a shared mutex and will provide + * rlock() and wlock() functions. + */ +template +using MutexLevelValue = detail::MutexLevelValueImpl< + true, + LockTraits::is_shared, + LockTraits::is_upgrade>; + /** * SynchronizedBase is a helper parent class for Synchronized. * * It provides wlock() and rlock() methods for shared mutex types, * or lock() methods for purely exclusive mutex types. */ -template +template class SynchronizedBase; /** @@ -58,7 +76,7 @@ class SynchronizedBase; * accessing the data. */ template -class SynchronizedBase { +class SynchronizedBase { public: using LockedPtr = ::folly::LockedPtr; using ConstWLockedPtr = @@ -185,6 +203,102 @@ class SynchronizedBase { } }; +/** + * SynchronizedBase specialization for upgrade mutex types. + * + * This class provides all the functionality provided by the SynchronizedBase + * specialization for shared mutexes and a ulock() method that returns an + * upgradable lock RAII proxy + */ +template +class SynchronizedBase + : public SynchronizedBase { + public: + using UpgradeLockedPtr = ::folly::LockedPtr; + using ConstUpgradeLockedPtr = + ::folly::LockedPtr; + using UpgradeLockedGuardPtr = + ::folly::LockedGuardPtr; + using ConstUpgradeLockedGuardPtr = + ::folly::LockedGuardPtr; + + /** + * Acquire an upgrade lock and return a LockedPtr that can be used to safely + * access the datum + * + * And the const version + */ + UpgradeLockedPtr ulock() { + return UpgradeLockedPtr(static_cast(this)); + } + ConstUpgradeLockedPtr ulock() const { + return ConstUpgradeLockedPtr(static_cast(this)); + } + + /** + * Acquire an upgrade lock and return a LockedPtr that can be used to safely + * access the datum + * + * And the const version + */ + template + UpgradeLockedPtr ulock(const std::chrono::duration& timeout) { + return UpgradeLockedPtr(static_cast(this), timeout); + } + template + UpgradeLockedPtr ulock( + const std::chrono::duration& timeout) const { + return ConstUpgradeLockedPtr(static_cast(this), timeout); + } + + /** + * Invoke a function while holding the lock. + * + * A reference to the datum will be passed into the function as its only + * argument. + * + * This can be used with a lambda argument for easily defining small critical + * sections in the code. For example: + * + * auto value = obj.withULock([](auto& data) { + * data.doStuff(); + * return data.getValue(); + * }); + * + * This is probably not the function you want. If the intent is to read the + * data object and determine whether you should upgrade to a write lock then + * the withULockPtr() method should be called instead, since it gives access + * to the LockedPtr proxy (which can be upgraded via the + * moveFromUpgradeToWrite() method) + */ + template + auto withULock(Function&& function) const { + ConstUpgradeLockedGuardPtr guardPtr(static_cast(this)); + return function(*guardPtr); + } + + /** + * Invoke a function while holding the lock exclusively. + * + * This is similar to withULock(), but the function will be passed a + * LockedPtr rather than a reference to the data itself. + * + * This allows scopedUnlock() and getUniqueLock() to be called on the + * LockedPtr argument. + * + * This also allows you to upgrade the LockedPtr proxy to a write state so + * that changes can be made to the underlying data + */ + template + auto withULockPtr(Function&& function) { + return function(ulock()); + } + template + auto withULockPtr(Function&& function) const { + return function(ulock()); + } +}; + /** * SynchronizedBase specialization for non-shared mutex types. * @@ -192,7 +306,7 @@ class SynchronizedBase { * data. */ template -class SynchronizedBase { +class SynchronizedBase { public: using LockedPtr = ::folly::LockedPtr; using ConstLockedPtr = @@ -310,10 +424,10 @@ class SynchronizedBase { template struct Synchronized : public SynchronizedBase< Synchronized, - LockTraits::is_shared> { + MutexLevelValue::value> { private: using Base = - SynchronizedBase, LockTraits::is_shared>; + SynchronizedBase, MutexLevelValue::value>; static constexpr bool nxCopyCtor{ std::is_nothrow_copy_constructible::value}; static constexpr bool nxMoveCtor{ @@ -991,6 +1105,74 @@ class LockedPtr : public LockedPtrBase< ScopedUnlocker scopedUnlock() { return ScopedUnlocker(this); } + + /*************************************************************************** + * Upgradable lock methods. + * These are disabled via SFINAE when the mutex is not upgradable + **************************************************************************/ + /** + * Move the locked ptr from an upgrade state to an exclusive state. The + * current lock is left in a null state. + */ + template < + typename SyncType = SynchronizedType, + typename = typename std::enable_if< + LockTraits::is_upgrade>::type> + LockedPtr + moveFromUpgradeToWrite() { + auto* parent_to_pass_on = this->parent_; + this->parent_ = nullptr; + return LockedPtr( + parent_to_pass_on); + } + + /** + * Move the locked ptr from an exclusive state to an upgrade state. The + * current lock is left in a null state. + */ + template < + typename SyncType = SynchronizedType, + typename = typename std::enable_if< + LockTraits::is_upgrade>::type> + LockedPtr + moveFromWriteToUpgrade() { + auto* parent_to_pass_on = this->parent_; + this->parent_ = nullptr; + return LockedPtr( + parent_to_pass_on); + } + + /** + * Move the locked ptr from an upgrade state to a shared state. The + * current lock is left in a null state. + */ + template < + typename SyncType = SynchronizedType, + typename = typename std::enable_if< + LockTraits::is_upgrade>::type> + LockedPtr + moveFromUpgradeToShared() { + auto* parent_to_pass_on = this->parent_; + this->parent_ = nullptr; + return LockedPtr( + parent_to_pass_on); + } + + /** + * Move the locked ptr from an exclusive state to a shared state. The + * current lock is left in a null state. + */ + template < + typename SyncType = SynchronizedType, + typename = typename std::enable_if< + LockTraits::is_upgrade>::type> + LockedPtr + moveFromWriteToShared() { + auto* parent_to_pass_on = this->parent_; + this->parent_ = nullptr; + return LockedPtr( + parent_to_pass_on); + } }; /** diff --git a/folly/test/SynchronizedTest.cpp b/folly/test/SynchronizedTest.cpp index 5826ef28..b7349645 100644 --- a/folly/test/SynchronizedTest.cpp +++ b/folly/test/SynchronizedTest.cpp @@ -189,6 +189,152 @@ class SynchronizedLockTest : public testing::Test { } }; +/** + * To avoid typing + */ +// static constexpr auto one_ms = std::chrono::milliseconds(1); + +/** + * Test mutex to help to automate assertions, taken from LockTraitsTest.cpp + */ +class FakeAllPowerfulAssertingMutexInternal { + public: + enum class CurrentLockState { UNLOCKED, SHARED, UPGRADE, UNIQUE }; + + void lock() { + EXPECT_EQ(this->lock_state, CurrentLockState::UNLOCKED); + this->lock_state = CurrentLockState::UNIQUE; + } + void unlock() { + EXPECT_EQ(this->lock_state, CurrentLockState::UNIQUE); + this->lock_state = CurrentLockState::UNLOCKED; + } + void lock_shared() { + EXPECT_EQ(this->lock_state, CurrentLockState::UNLOCKED); + this->lock_state = CurrentLockState::SHARED; + } + void unlock_shared() { + EXPECT_EQ(this->lock_state, CurrentLockState::SHARED); + this->lock_state = CurrentLockState::UNLOCKED; + } + void lock_upgrade() { + EXPECT_EQ(this->lock_state, CurrentLockState::UNLOCKED); + this->lock_state = CurrentLockState::UPGRADE; + } + void unlock_upgrade() { + EXPECT_EQ(this->lock_state, CurrentLockState::UPGRADE); + this->lock_state = CurrentLockState::UNLOCKED; + } + + void unlock_upgrade_and_lock() { + EXPECT_EQ(this->lock_state, CurrentLockState::UPGRADE); + this->lock_state = CurrentLockState::UNIQUE; + } + void unlock_and_lock_upgrade() { + EXPECT_EQ(this->lock_state, CurrentLockState::UNIQUE); + this->lock_state = CurrentLockState::UPGRADE; + } + void unlock_and_lock_shared() { + EXPECT_EQ(this->lock_state, CurrentLockState::UNIQUE); + this->lock_state = CurrentLockState::SHARED; + } + void unlock_upgrade_and_lock_shared() { + EXPECT_EQ(this->lock_state, CurrentLockState::UPGRADE); + this->lock_state = CurrentLockState::SHARED; + } + + template + bool try_lock_for(const std::chrono::duration&) { + EXPECT_EQ(this->lock_state, CurrentLockState::UNLOCKED); + this->lock_state = CurrentLockState::UNIQUE; + return true; + } + + template + bool try_lock_upgrade_for(const std::chrono::duration&) { + EXPECT_EQ(this->lock_state, CurrentLockState::UNLOCKED); + this->lock_state = CurrentLockState::UPGRADE; + return true; + } + + template + bool try_unlock_upgrade_and_lock_for( + const std::chrono::duration&) { + EXPECT_EQ(this->lock_state, CurrentLockState::UPGRADE); + this->lock_state = CurrentLockState::UNIQUE; + return true; + } + + /* + * Initialize the FakeMutex with an unlocked state + */ + CurrentLockState lock_state{CurrentLockState::UNLOCKED}; +}; + +/** + * The following works around the internal mutex for synchronized being + * private + * + * This is horridly thread unsafe. + */ +static FakeAllPowerfulAssertingMutexInternal globalAllPowerfulAssertingMutex; + +class FakeAllPowerfulAssertingMutex { + public: + void lock() { + globalAllPowerfulAssertingMutex.lock(); + } + void unlock() { + globalAllPowerfulAssertingMutex.unlock(); + } + void lock_shared() { + globalAllPowerfulAssertingMutex.lock_shared(); + } + void unlock_shared() { + globalAllPowerfulAssertingMutex.unlock_shared(); + } + void lock_upgrade() { + globalAllPowerfulAssertingMutex.lock_upgrade(); + } + void unlock_upgrade() { + globalAllPowerfulAssertingMutex.unlock_upgrade(); + } + + void unlock_upgrade_and_lock() { + globalAllPowerfulAssertingMutex.unlock_upgrade_and_lock(); + } + void unlock_and_lock_upgrade() { + globalAllPowerfulAssertingMutex.unlock_and_lock_upgrade(); + } + void unlock_and_lock_shared() { + globalAllPowerfulAssertingMutex.unlock_and_lock_shared(); + } + void unlock_upgrade_and_lock_shared() { + globalAllPowerfulAssertingMutex.unlock_upgrade_and_lock_shared(); + } + + template + bool try_lock_for(const std::chrono::duration& arg) { + return globalAllPowerfulAssertingMutex.try_lock_for(arg); + } + + template + bool try_lock_upgrade_for(const std::chrono::duration& arg) { + return globalAllPowerfulAssertingMutex.try_lock_upgrade_for(arg); + } + + template + bool try_unlock_upgrade_and_lock_for( + const std::chrono::duration& arg) { + return globalAllPowerfulAssertingMutex.try_unlock_upgrade_and_lock_for(arg); + } + + // reset state on destruction + ~FakeAllPowerfulAssertingMutex() { + globalAllPowerfulAssertingMutex = FakeAllPowerfulAssertingMutexInternal{}; + } +}; + // Single level of SYNCHRONIZED and UNSYNCHRONIZED, although nested test are // super set of it, it is possible single level test passes while nested tests // fail @@ -253,3 +399,169 @@ TEST_F(SynchronizedLockTest, NestedSyncUnSync2) { } EXPECT_EQ((CountPair{4, 4}), FakeMutex::getLockUnlockCount()); } + +TEST_F(SynchronizedLockTest, UpgradableLocking) { + folly::Synchronized sync; + + // sanity assert + static_assert( + std::is_same::type, int>::value, + "The ulock function was not well configured, blame aary@instagram.com"); + + { + auto ulock = sync.ulock(); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UPGRADE); + } + + // should be unlocked here + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNLOCKED); + + // test going from upgrade to exclusive + { + auto ulock = sync.ulock(); + auto wlock = ulock.moveFromUpgradeToWrite(); + EXPECT_EQ(static_cast(ulock), false); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNIQUE); + } + + // should be unlocked here + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNLOCKED); + + // test going from upgrade to shared + { + auto ulock = sync.ulock(); + auto slock = ulock.moveFromUpgradeToShared(); + EXPECT_EQ(static_cast(ulock), false); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::SHARED); + } + + // should be unlocked here + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNLOCKED); + + // test going from exclusive to upgrade + { + auto wlock = sync.wlock(); + auto ulock = wlock.moveFromWriteToUpgrade(); + EXPECT_EQ(static_cast(wlock), false); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UPGRADE); + } + + // should be unlocked here + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNLOCKED); + + // test going from exclusive to shared + { + auto wlock = sync.wlock(); + auto slock = wlock.moveFromWriteToShared(); + EXPECT_EQ(static_cast(wlock), false); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::SHARED); + } + + // should be unlocked here + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNLOCKED); +} + +TEST_F(SynchronizedLockTest, UpgradableLockingWithULock) { + folly::Synchronized sync; + + // sanity assert + static_assert( + std::is_same::type, int>::value, + "The ulock function was not well configured, blame aary@instagram.com"); + + // test from upgrade to write + sync.withULockPtr([](auto ulock) { + EXPECT_EQ(static_cast(ulock), true); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UPGRADE); + + auto wlock = ulock.moveFromUpgradeToWrite(); + EXPECT_EQ(static_cast(ulock), false); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNIQUE); + }); + + // should be unlocked here + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNLOCKED); + + // test from write to upgrade + sync.withWLockPtr([](auto wlock) { + EXPECT_EQ(static_cast(wlock), true); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNIQUE); + + auto ulock = wlock.moveFromWriteToUpgrade(); + EXPECT_EQ(static_cast(wlock), false); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UPGRADE); + }); + + // should be unlocked here + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNLOCKED); + + // test from upgrade to shared + sync.withULockPtr([](auto ulock) { + EXPECT_EQ(static_cast(ulock), true); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UPGRADE); + + auto slock = ulock.moveFromUpgradeToShared(); + EXPECT_EQ(static_cast(ulock), false); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::SHARED); + }); + + // should be unlocked here + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNLOCKED); + + // test from write to shared + sync.withWLockPtr([](auto wlock) { + EXPECT_EQ(static_cast(wlock), true); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNIQUE); + + auto slock = wlock.moveFromWriteToShared(); + EXPECT_EQ(static_cast(wlock), false); + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::SHARED); + }); + + // should be unlocked here + EXPECT_EQ( + globalAllPowerfulAssertingMutex.lock_state, + FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UNLOCKED); +} -- 2.34.1