From 50f4f92c284492f4bb87f3735c82e180a0e49327 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Mon, 11 Jul 2016 18:36:00 -0700 Subject: [PATCH] improve Synchronized LockedPtr class, and add new lock() APIs Summary: This refactors the Synchronized::LockedPtr class, and adds new lock()/wlock()/rlock() APIs to Synchronized. The LockedPtr changes include: - Consolidate code so a single template class can be used for both const and non-const operation, rather than requiring separate class definitions. A LockPolicy template parameter controls if the lock should be acquired in exclusive or shared mode, and a SynchronizedType parameter controls whether or not the internal data is const or not. - Specialize LockedPtr for std::mutex, so it uses std::unique_lock internally. This requires slightly more storage space internally, but it allows Synchronized to be used with std::condition_variable. - Implement operator*() to dereference the pointer and retrieve a reference to the locked data. - Implement operator!() and provide an isValid() method to check the validity of the LockedPtr. These are needed to tell if a timed acquire operation succeeded or timed out. - Drop the LockedPtr copy constructor. Previously the copy constructor acquired the lock a second time. If anyone needs the ability to hold a shared lock a second time solely via a LockedPtr (and not via the original Synchronized object), I think we should add back a much more explicit API to do this. Furthermore, this adds lock(), wlock(), and rlock() methods to Synchronized to explicitly obtain a LockedPtr. These APIs behave similar to operator->(), but are more explicit, and require the caller to make a concious choice about whether or not an exclusive or shared lock should be acquired. The lock() method is present only on Synchronized instantiations using an exclusive mutex, and the wlock() and rlock() methods are present only on Synchronized instantiations that use a shared mutex. I plan to deprecate the existing Synchronized::operator->() method and the various SYNCHRONIZED macros in upcoming diffs. For now this adds comments directing users to the new methods, but does not start any of the technical deprecation changes yet. Reviewed By: yfeldblum Differential Revision: D3526489 fbshipit-source-id: 8a96a09b68656ff9215dcdfdf32ecd2bfbb1727f --- folly/LockTraits.h | 73 +++ folly/Synchronized.h | 938 +++++++++++++++++++-------- folly/test/SynchronizedTest.cpp | 35 +- folly/test/SynchronizedTestLib-inl.h | 323 +++++++-- folly/test/SynchronizedTestLib.h | 36 +- 5 files changed, 1070 insertions(+), 335 deletions(-) diff --git a/folly/LockTraits.h b/folly/LockTraits.h index 3b0d21a2..3b84f503 100644 --- a/folly/LockTraits.h +++ b/folly/LockTraits.h @@ -249,4 +249,77 @@ typename std::enable_if::is_shared>::type unlock_shared_or_unique(Mutex& mutex) { LockTraits::unlock(mutex); } + +/* + * Lock policy classes. + * + * 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. + */ +class LockPolicyExclusive { + public: + template + static void lock(Mutex& mutex) { + LockTraits::lock(mutex); + } + template + static bool try_lock_for( + Mutex& mutex, + const std::chrono::duration& timeout) { + return LockTraits::try_lock_for(mutex, timeout); + } + template + static void unlock(Mutex& mutex) { + LockTraits::unlock(mutex); + } +}; + +/** + * A lock policy that performs shared lock operations. + * This policy only works with shared mutex types. + */ +class LockPolicyShared { + public: + template + static void lock(Mutex& mutex) { + LockTraits::lock_shared(mutex); + } + template + static bool try_lock_for( + Mutex& mutex, + const std::chrono::duration& timeout) { + return LockTraits::try_lock_shared_for(mutex, timeout); + } + template + static void unlock(Mutex& mutex) { + LockTraits::unlock_shared(mutex); + } +}; + +/** + * A lock policy that performs a shared lock operation if a shared mutex type + * is given, or a normal exclusive lock operation on non-shared mutex types. + */ +class LockPolicyShareable { + public: + template + static void lock(Mutex& mutex) { + lock_shared_or_unique(mutex); + } + template + static bool try_lock_for( + Mutex& mutex, + const std::chrono::duration& timeout) { + return try_lock_shared_or_unique_for(mutex, timeout); + } + template + static void unlock(Mutex& mutex) { + unlock_shared_or_unique(mutex); + } +}; + } // folly diff --git a/folly/Synchronized.h b/folly/Synchronized.h index 87e293eb..f36bf559 100644 --- a/folly/Synchronized.h +++ b/folly/Synchronized.h @@ -18,7 +18,9 @@ * This module implements a Synchronized abstraction useful in * mutex-based concurrency. * - * @author: Andrei Alexandrescu (andrei.alexandrescu@fb.com) + * The Synchronized class is the primary public API exposed by this + * module. See folly/docs/Synchronized.md for a more complete explanation of + * this class and its benefits. */ #pragma once @@ -27,14 +29,138 @@ #include #include #include +#include #include #include namespace folly { -namespace detail { -enum InternalDoNotUse {}; -} // namespace detail +template +class LockedPtrBase; +template +class LockedPtr; + +/** + * 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 +class SynchronizedBase; + +/** + * SynchronizedBase specialization for shared mutex types. + * + * This class provides wlock() and rlock() methods for acquiring the lock and + * accessing the data. + */ +template +class SynchronizedBase { + public: + using LockedPtr = ::folly::LockedPtr; + using ConstWLockedPtr = + ::folly::LockedPtr; + using ConstLockedPtr = ::folly::LockedPtr; + + /** + * Acquire an exclusive lock, and return a LockedPtr that can be used to + * safely access the datum. + * + * LockedPtr offers operator -> and * to provide access to the datum. + * The lock will be released when the LockedPtr is destroyed. + */ + LockedPtr wlock() { + return LockedPtr(static_cast(this)); + } + ConstWLockedPtr wlock() const { + return ConstWLockedPtr(static_cast(this)); + } + + /** + * Acquire a read lock, and return a ConstLockedPtr that can be used to + * safely access the datum. + */ + ConstLockedPtr rlock() const { + return ConstLockedPtr(static_cast(this)); + } + + /** + * Attempts to acquire the lock, or fails if the timeout elapses first. + * If acquisition is unsuccessful, the returned LockedPtr will be null. + * + * (Use LockedPtr::isNull() to check for validity.) + */ + template + LockedPtr wlock(const std::chrono::duration& timeout) { + return LockedPtr(static_cast(this), timeout); + } + template + ConstWLockedPtr wlock( + const std::chrono::duration& timeout) const { + return ConstWLockedPtr(static_cast(this), timeout); + } + + /** + * Attempts to acquire the lock, or fails if the timeout elapses first. + * If acquisition is unsuccessful, the returned LockedPtr will be null. + * + * (Use LockedPtr::isNull() to check for validity.) + */ + template + ConstLockedPtr rlock( + const std::chrono::duration& timeout) const { + return ConstLockedPtr(static_cast(this), timeout); + } +}; + +/** + * SynchronizedBase specialization for non-shared mutex types. + * + * This class provides lock() methods for acquiring the lock and accessing the + * data. + */ +template +class SynchronizedBase { + public: + using LockedPtr = ::folly::LockedPtr; + using ConstLockedPtr = + ::folly::LockedPtr; + + /** + * Acquire a lock, and return a LockedPtr that can be used to safely access + * the datum. + */ + LockedPtr lock() { + return LockedPtr(static_cast(this)); + } + + /** + * Acquire a lock, and return a ConstLockedPtr that can be used to safely + * access the datum. + */ + ConstLockedPtr lock() const { + return ConstLockedPtr(static_cast(this)); + } + + /** + * Attempts to acquire the lock, or fails if the timeout elapses first. + * If acquisition is unsuccessful, the returned LockedPtr will be null. + */ + template + LockedPtr lock(const std::chrono::duration& timeout) { + return LockedPtr(static_cast(this), timeout); + } + + /** + * Attempts to acquire the lock, or fails if the timeout elapses first. + * If acquisition is unsuccessful, the returned LockedPtr will be null. + */ + template + ConstLockedPtr lock(const std::chrono::duration& timeout) const { + return ConstLockedPtr(static_cast(this), timeout); + } +}; /** * Synchronized encapsulates an object of type T (a "datum") paired @@ -48,8 +174,8 @@ enum InternalDoNotUse {}; * The second parameter must be a mutex type. Any mutex type supported by * LockTraits can be used. By default any class with lock() and * unlock() methods will work automatically. LockTraits can be specialized to - * teach Locked how to use other custom mutex types. See the documentation in - * LockTraits.h for additional details. + * teach Synchronized how to use other custom mutex types. See the + * documentation in LockTraits.h for additional details. * * Supported mutexes that work by default include std::mutex, * std::recursive_mutex, std::timed_mutex, std::recursive_timed_mutex, @@ -60,49 +186,43 @@ enum InternalDoNotUse {}; * boost::recursive_timed_mutex. */ template -struct Synchronized { - /** - * Default constructor leaves both members call their own default - * constructor. - */ - Synchronized() = default; - +struct Synchronized : public SynchronizedBase< + Synchronized, + LockTraits::is_shared> { private: + using Base = + SynchronizedBase, LockTraits::is_shared>; static constexpr bool nxCopyCtor{ std::is_nothrow_copy_constructible::value}; static constexpr bool nxMoveCtor{ std::is_nothrow_move_constructible::value}; + public: + using LockedPtr = typename Base::LockedPtr; + using ConstLockedPtr = typename Base::ConstLockedPtr; + using DataType = T; + using MutexType = Mutex; + /** - * Helper constructors to enable Synchronized for - * non-default constructible types T. - * Guards are created in actual public constructors and are alive - * for the time required to construct the object + * Default constructor leaves both members call their own default + * constructor. */ - template - Synchronized(const Synchronized& rhs, - const Guard& /*guard*/) noexcept(nxCopyCtor) - : datum_(rhs.datum_) {} - - template - Synchronized(Synchronized&& rhs, const Guard& /*guard*/) noexcept(nxMoveCtor) - : datum_(std::move(rhs.datum_)) {} + Synchronized() = default; - public: /** * Copy constructor copies the data (with locking the source and * all) but does NOT copy the mutex. Doing so would result in * deadlocks. */ Synchronized(const Synchronized& rhs) noexcept(nxCopyCtor) - : Synchronized(rhs, rhs.operator->()) {} + : Synchronized(rhs, rhs.contextualRLock()) {} /** * Move constructor moves the data (with locking the source and all) * but does not move the mutex. */ Synchronized(Synchronized&& rhs) noexcept(nxMoveCtor) - : Synchronized(std::move(rhs), rhs.operator->()) {} + : Synchronized(std::move(rhs), rhs.contextualLock()) {} /** * Constructor taking a datum as argument copies it. There is no @@ -184,218 +304,66 @@ struct Synchronized { } /** - * A LockedPtr lp keeps a modifiable (i.e. non-const) - * Synchronized object locked for the duration of lp's - * existence. Because of this, you get to access the datum's methods - * directly by using lp->fun(). + * Acquire an appropriate lock based on the context. + * + * If the mutex is a shared mutex, and the Synchronized instance is const, + * this acquires a shared lock. Otherwise this acquires an exclusive lock. + * + * In general, prefer using the explicit rlock() and wlock() methods + * for read-write locks, and lock() for purely exclusive locks. + * + * contextualLock() is primarily intended for use in other template functions + * that do not necessarily know the lock type. */ - struct LockedPtr { - /** - * Found no reason to leave this hanging. - */ - LockedPtr() = delete; - - /** - * Takes a Synchronized and locks it. - */ - explicit LockedPtr(Synchronized* parent) : parent_(parent) { - acquire(); - } - - /** - * Takes a Synchronized and attempts to lock it for some - * milliseconds. If not, the LockedPtr will be subsequently null. - */ - LockedPtr(Synchronized* parent, unsigned int milliseconds) { - std::chrono::milliseconds chronoMS(milliseconds); - if (LockTraits::try_lock_for(parent->mutex_, chronoMS)) { - parent_ = parent; - return; - } - // Could not acquire the resource, pointer is null - parent_ = nullptr; - } - - /** - * This is used ONLY inside SYNCHRONIZED_DUAL. It initializes - * everything properly, but does not lock the parent because it - * "knows" someone else will lock it. Please do not use. - */ - LockedPtr(Synchronized* parent, detail::InternalDoNotUse) - : parent_(parent) { - } - - /** - * Copy ctor adds one lock. - */ - LockedPtr(const LockedPtr& rhs) : parent_(rhs.parent_) { - acquire(); - } - - /** - * Assigning from another LockedPtr results in freeing the former - * lock and acquiring the new one. The method works with - * self-assignment (does nothing). - */ - LockedPtr& operator=(const LockedPtr& rhs) { - if (parent_ != rhs.parent_) { - if (parent_) parent_->mutex_.unlock(); - parent_ = rhs.parent_; - acquire(); - } - return *this; - } - - /** - * Destructor releases. - */ - ~LockedPtr() { - if (parent_) { - LockTraits::unlock(parent_->mutex_); - } - } - - /** - * Safe to access the data. Don't save the obtained pointer by - * invoking lp.operator->() by hand. Also, if the method returns a - * handle stored inside the datum, don't use this idiom - use - * SYNCHRONIZED below. - */ - T* operator->() { - return parent_ ? &parent_->datum_ : nullptr; - } - - /** - * This class temporarily unlocks a LockedPtr in a scoped - * manner. It is used inside of the UNSYNCHRONIZED macro. - */ - struct Unsynchronizer { - explicit Unsynchronizer(LockedPtr* p) : parent_(p) { - LockTraits::unlock(parent_->parent_->mutex_); - } - Unsynchronizer(const Unsynchronizer&) = delete; - Unsynchronizer& operator=(const Unsynchronizer&) = delete; - ~Unsynchronizer() { - parent_->acquire(); - } - LockedPtr* operator->() const { - return parent_; - } - private: - LockedPtr* parent_; - }; - friend struct Unsynchronizer; - Unsynchronizer typeHackDoNotUse(); - - template - friend void lockInOrder(P1& p1, P2& p2); - - private: - void acquire() { - if (parent_) { - LockTraits::lock(parent_->mutex_); - } - } - - // This is the entire state of LockedPtr. - Synchronized* parent_; - }; - + LockedPtr contextualLock() { + return LockedPtr(this); + } + ConstLockedPtr contextualLock() const { + return ConstLockedPtr(this); + } + template + LockedPtr contextualLock(const std::chrono::duration& timeout) { + return LockedPtr(this, timeout); + } + template + ConstLockedPtr contextualLock( + const std::chrono::duration& timeout) const { + return ConstLockedPtr(this, timeout); + } /** - * ConstLockedPtr does exactly what LockedPtr does, but for const - * Synchronized objects. Of interest is that ConstLockedPtr only - * uses a read lock, which is faster but more restrictive - you only - * get to call const methods of the datum. + * contextualRLock() acquires a read lock if the mutex type is shared, + * or a regular exclusive lock for non-shared mutex types. * - * Much of the code between LockedPtr and - * ConstLockedPtr is identical and could be factor out, but there - * are enough nagging little differences to not justify the trouble. - */ - struct ConstLockedPtr { - ConstLockedPtr() = delete; - explicit ConstLockedPtr(const Synchronized* parent) : parent_(parent) { - acquire(); - } - ConstLockedPtr(const Synchronized* parent, detail::InternalDoNotUse) - : parent_(parent) { - } - ConstLockedPtr(const ConstLockedPtr& rhs) : parent_(rhs.parent_) { - acquire(); - } - explicit ConstLockedPtr(const LockedPtr& rhs) : parent_(rhs.parent_) { - acquire(); - } - ConstLockedPtr(const Synchronized* parent, unsigned int milliseconds) { - if (try_lock_shared_or_unique_for( - parent->mutex_, std::chrono::milliseconds(milliseconds))) { - parent_ = parent; - return; - } - // Could not acquire the resource, pointer is null - parent_ = nullptr; - } - - ConstLockedPtr& operator=(const ConstLockedPtr& rhs) { - if (parent_ != rhs.parent_) { - if (parent_) parent_->mutex_.unlock_shared(); - parent_ = rhs.parent_; - acquire(); - } - } - ~ConstLockedPtr() { - if (parent_) { - unlock_shared_or_unique(parent_->mutex_); - } - } - - const T* operator->() const { - return parent_ ? &parent_->datum_ : nullptr; - } - - struct Unsynchronizer { - explicit Unsynchronizer(ConstLockedPtr* p) : parent_(p) { - unlock_shared_or_unique(parent_->parent_->mutex_); - } - Unsynchronizer(const Unsynchronizer&) = delete; - Unsynchronizer& operator=(const Unsynchronizer&) = delete; - ~Unsynchronizer() { - lock_shared_or_unique(parent_->parent_->mutex_); - } - ConstLockedPtr* operator->() const { - return parent_; - } - private: - ConstLockedPtr* parent_; - }; - friend struct Unsynchronizer; - Unsynchronizer typeHackDoNotUse(); - - template - friend void lockInOrder(P1& p1, P2& p2); - - private: - void acquire() { - if (parent_) { - lock_shared_or_unique(parent_->mutex_); - } - } - - const Synchronized* parent_; - }; + * contextualRLock() when you know that you prefer a read lock (if + * available), even if the Synchronized object itself is non-const. + */ + ConstLockedPtr contextualRLock() const { + return ConstLockedPtr(this); + } + template + ConstLockedPtr contextualRLock( + const std::chrono::duration& timeout) const { + return ConstLockedPtr(this, timeout); + } /** - * This accessor offers a LockedPtr. In turn. LockedPtr offers + * This accessor offers a LockedPtr. In turn, LockedPtr offers * operator-> returning a pointer to T. The operator-> keeps * expanding until it reaches a pointer, so syncobj->foo() will lock * the object and call foo() against it. - */ + * + * NOTE: This API is planned to be deprecated in an upcoming diff. + * Prefer using lock(), wlock(), or rlock() instead. + */ LockedPtr operator->() { return LockedPtr(this); } /** - * Same, for constant objects. You will be able to invoke only const - * methods. + * Obtain a ConstLockedPtr. + * + * NOTE: This API is planned to be deprecated in an upcoming diff. + * Prefer using lock(), wlock(), or rlock() instead. */ ConstLockedPtr operator->() const { return ConstLockedPtr(this); @@ -404,30 +372,25 @@ struct Synchronized { /** * Attempts to acquire for a given number of milliseconds. If * acquisition is unsuccessful, the returned LockedPtr is NULL. + * + * NOTE: This API is deprecated. Use lock(), wlock(), or rlock() instead. + * In the future it will be marked with a deprecation attribute to emit + * build-time warnings, and then it will be removed entirely. */ LockedPtr timedAcquire(unsigned int milliseconds) { - return LockedPtr(this, milliseconds); + return LockedPtr(this, std::chrono::milliseconds(milliseconds)); } /** - * As above, for a constant object. + * Attempts to acquire for a given number of milliseconds. If + * acquisition is unsuccessful, the returned ConstLockedPtr is NULL. + * + * NOTE: This API is deprecated. Use lock(), wlock(), or rlock() instead. + * In the future it will be marked with a deprecation attribute to emit + * build-time warnings, and then it will be removed entirely. */ ConstLockedPtr timedAcquire(unsigned int milliseconds) const { - return ConstLockedPtr(this, milliseconds); - } - - /** - * Used by SYNCHRONIZED_DUAL. - */ - LockedPtr internalDoNotUse() { - return LockedPtr(this, detail::InternalDoNotUse()); - } - - /** - * ditto - */ - ConstLockedPtr internalDoNotUse() const { - return ConstLockedPtr(this, detail::InternalDoNotUse()); + return ConstLockedPtr(this, std::chrono::milliseconds(milliseconds)); } /** @@ -435,6 +398,9 @@ struct Synchronized { * call a const method against it. The most efficient way to achieve * that is by using a read lock. You get to do so by using * obj.asConst()->method() instead of obj->method(). + * + * NOTE: This API is planned to be deprecated in an upcoming diff. + * Use rlock() instead. */ const Synchronized& asConst() const { return *this; @@ -464,7 +430,7 @@ struct Synchronized { * held only briefly. */ void swap(T& rhs) { - LockedPtr guard = operator->(); + LockedPtr guard(this); using std::swap; swap(datum_, rhs); @@ -474,7 +440,7 @@ struct Synchronized { * Copies datum to a given target. */ void copy(T* target) const { - ConstLockedPtr guard = operator->(); + ConstLockedPtr guard(this); *target = datum_; } @@ -482,15 +448,427 @@ struct Synchronized { * Returns a fresh copy of the datum. */ T copy() const { - ConstLockedPtr guard = operator->(); + ConstLockedPtr guard(this); return datum_; } -private: + private: + template + friend class folly::LockedPtrBase; + template + friend class folly::LockedPtr; + + /** + * Helper constructors to enable Synchronized for + * non-default constructible types T. + * Guards are created in actual public constructors and are alive + * for the time required to construct the object + */ + Synchronized( + const Synchronized& rhs, + const ConstLockedPtr& /*guard*/) noexcept(nxCopyCtor) + : datum_(rhs.datum_) {} + + Synchronized(Synchronized&& rhs, const LockedPtr& /*guard*/) noexcept( + nxMoveCtor) + : datum_(std::move(rhs.datum_)) {} + + // Synchronized data members T datum_; mutable Mutex mutex_; }; +template +class ScopedUnlocker; + +/** + * A helper base class for implementing LockedPtr. + * + * The main reason for having this as a separate class is so we can specialize + * it for std::mutex, so we can expose a std::unique_lock to the caller + * when std::mutex is being used. This allows callers to use a + * std::condition_variable with the mutex from a Synchronized. + * + * We don't use std::unique_lock with other Mutex types since it makes the + * LockedPtr class slightly larger, and it makes the logic to support + * ScopedUnlocker slightly more complicated. std::mutex is the only one that + * really seems to benefit from the unique_lock. std::condition_variable + * itself only supports std::unique_lock, so there doesn't seem to + * be any real benefit to exposing the unique_lock with other mutex types. + * + * Note that the SynchronizedType template parameter may or may not be const + * qualified. + */ +template +class LockedPtrBase { + public: + using MutexType = Mutex; + friend class folly::ScopedUnlocker; + + /** + * Destructor releases. + */ + ~LockedPtrBase() { + if (parent_) { + LockPolicy::unlock(parent_->mutex_); + } + } + + protected: + LockedPtrBase() {} + explicit LockedPtrBase(SynchronizedType* parent) : parent_(parent) { + LockPolicy::lock(parent_->mutex_); + } + template + LockedPtrBase( + SynchronizedType* parent, + const std::chrono::duration& timeout) { + if (LockPolicy::try_lock_for(parent->mutex_, timeout)) { + this->parent_ = parent; + } + } + LockedPtrBase(LockedPtrBase&& rhs) noexcept : parent_(rhs.parent_) { + rhs.parent_ = nullptr; + } + LockedPtrBase& operator=(LockedPtrBase&& rhs) noexcept { + if (parent_) { + LockPolicy::unlock(parent_->mutex_); + } + + parent_ = rhs.parent_; + rhs.parent_ = nullptr; + return *this; + } + + using UnlockerData = SynchronizedType*; + + /** + * Get a pointer to the Synchronized object from the UnlockerData. + * + * In the generic case UnlockerData is just the Synchronized pointer, + * so we return it as is. (This function is more interesting in the + * std::mutex specialization below.) + */ + static SynchronizedType* getSynchronized(UnlockerData data) { + return data; + } + + UnlockerData releaseLock() { + auto current = parent_; + parent_ = nullptr; + LockPolicy::unlock(current->mutex_); + return current; + } + void reacquireLock(UnlockerData&& data) { + DCHECK(parent_ == nullptr); + parent_ = data; + LockPolicy::lock(parent_->mutex_); + } + + SynchronizedType* parent_ = nullptr; +}; + +/** + * LockedPtrBase specialization for use with std::mutex. + * + * When std::mutex is used we use a std::unique_lock to hold the mutex. + * This makes it possible to use std::condition_variable with a + * Synchronized. + */ +template +class LockedPtrBase { + public: + using MutexType = std::mutex; + friend class folly::ScopedUnlocker; + + /** + * Destructor releases. + */ + ~LockedPtrBase() { + // The std::unique_lock will automatically release the lock when it is + // destroyed, so we don't need to do anything extra here. + } + + LockedPtrBase(LockedPtrBase&& rhs) noexcept + : lock_(std::move(rhs.lock_)), parent_(rhs.parent_) { + rhs.parent_ = nullptr; + } + LockedPtrBase& operator=(LockedPtrBase&& rhs) noexcept { + lock_ = std::move(rhs.lock_); + parent_ = rhs.parent_; + rhs.parent_ = nullptr; + return *this; + } + + /** + * Get a reference to the std::unique_lock. + * + * This is provided so that callers can use Synchronized + * with a std::condition_variable. + * + * While this API could be used to bypass the normal Synchronized APIs and + * manually interact with the underlying unique_lock, this is strongly + * discouraged. + */ + std::unique_lock& getUniqueLock() { + return lock_; + } + + protected: + LockedPtrBase() {} + explicit LockedPtrBase(SynchronizedType* parent) + : lock_(parent->mutex_), parent_(parent) {} + + using UnlockerData = + std::pair, SynchronizedType*>; + + static SynchronizedType* getSynchronized(const UnlockerData& data) { + return data.second; + } + + UnlockerData releaseLock() { + UnlockerData data(std::move(lock_), parent_); + parent_ = nullptr; + data.first.unlock(); + return data; + } + void reacquireLock(UnlockerData&& data) { + lock_ = std::move(data.first); + lock_.lock(); + parent_ = data.second; + } + + // The specialization for std::mutex does have to store slightly more + // state than the default implementation. + std::unique_lock lock_; + SynchronizedType* parent_ = nullptr; +}; + +/** + * This class temporarily unlocks a LockedPtr in a scoped manner. + */ +template +class ScopedUnlocker { + public: + explicit ScopedUnlocker(LockedPtr* p) + : ptr_(p), data_(ptr_->releaseLock()) {} + ScopedUnlocker(const ScopedUnlocker&) = delete; + ScopedUnlocker& operator=(const ScopedUnlocker&) = delete; + ScopedUnlocker(ScopedUnlocker&& other) noexcept + : ptr_(other.ptr_), data_(std::move(other.data_)) { + other.ptr_ = nullptr; + } + ScopedUnlocker& operator=(ScopedUnlocker&& other) = delete; + + ~ScopedUnlocker() { + if (ptr_) { + ptr_->reacquireLock(std::move(data_)); + } + } + + /** + * Return a pointer to the Synchronized object used by this ScopedUnlocker. + */ + SynchronizedType* getSynchronized() const { + return LockedPtr::getSynchronized(data_); + } + + private: + using Data = typename LockedPtr::UnlockerData; + LockedPtr* ptr_{nullptr}; + Data data_; +}; + +/** + * A LockedPtr keeps a Synchronized object locked for the duration of + * LockedPtr's existence. + * + * It provides access the datum's members directly by using operator->() and + * operator*(). + * + * The LockPolicy parameter controls whether or not the lock is acquired in + * exclusive or shared mode. + */ +template +class LockedPtr : public LockedPtrBase< + SynchronizedType, + typename SynchronizedType::MutexType, + LockPolicy> { + private: + using Base = LockedPtrBase< + SynchronizedType, + typename SynchronizedType::MutexType, + LockPolicy>; + using UnlockerData = typename Base::UnlockerData; + // CDataType is the DataType with the appropriate const-qualification + using CDataType = typename std::conditional< + std::is_const::value, + typename SynchronizedType::DataType const, + typename SynchronizedType::DataType>::type; + + public: + using DataType = typename SynchronizedType::DataType; + using MutexType = typename SynchronizedType::MutexType; + using Synchronized = typename std::remove_const::type; + friend class ScopedUnlocker; + + /** + * Creates an uninitialized LockedPtr. + * + * Dereferencing an uninitialized LockedPtr is not allowed. + */ + LockedPtr() {} + + /** + * Takes a Synchronized and locks it. + */ + explicit LockedPtr(SynchronizedType* parent) : Base(parent) {} + + /** + * Takes a Synchronized and attempts to lock it, within the specified + * timeout. + * + * Blocks until the lock is acquired or until the specified timeout expires. + * If the timeout expired without acquiring the lock, the LockedPtr will be + * null, and LockedPtr::isNull() will return true. + */ + template + LockedPtr( + SynchronizedType* parent, + const std::chrono::duration& timeout) + : Base(parent, timeout) {} + + /** + * Move constructor. + */ + LockedPtr(LockedPtr&& rhs) noexcept = default; + + /** + * Move assignment operator. + */ + LockedPtr& operator=(LockedPtr&& rhs) noexcept = default; + + /* + * Copy constructor and assignment operator are deleted. + */ + LockedPtr(const LockedPtr& rhs) = delete; + LockedPtr& operator=(const LockedPtr& rhs) = delete; + + /** + * Destructor releases. + */ + ~LockedPtr() {} + + /** + * Check if this LockedPtr is uninitialized, or points to valid locked data. + * + * This method can be used to check if a timed-acquire operation succeeded. + * If an acquire operation times out it will result in a null LockedPtr. + * + * A LockedPtr is always either null, or holds a lock to valid data. + * Methods such as scopedUnlock() reset the LockedPtr to null for the + * duration of the unlock. + */ + bool isNull() const { + return this->parent_ == nullptr; + } + + /** + * Explicit boolean conversion. + * + * Returns !isNull() + */ + explicit operator bool() const { + return this->parent_ != nullptr; + } + + /** + * Access the locked data. + * + * This method should only be used if the LockedPtr is valid. + */ + CDataType* operator->() const { + return &this->parent_->datum_; + } + + /** + * Access the locked data. + * + * This method should only be used if the LockedPtr is valid. + */ + CDataType& operator*() const { + return this->parent_->datum_; + } + + /** + * Temporarily unlock the LockedPtr, and reset it to null. + * + * Returns an helper object that will re-lock and restore the LockedPtr when + * the helper is destroyed. The LockedPtr may not be dereferenced for as + * long as this helper object exists. + */ + ScopedUnlocker scopedUnlock() { + return ScopedUnlocker(this); + } +}; + +namespace detail { +/* + * A helper alias to select a ConstLockedPtr if the template parameter is a + * const Synchronized, or a LockedPtr if the parameter is not const. + */ +template +using LockedPtrType = typename std::conditional< + std::is_const::value, + typename SynchronizedType::ConstLockedPtr, + typename SynchronizedType::LockedPtr>::type; +} // detail + +/** + * Acquire locks for multiple Synchronized objects, in a deadlock-safe + * manner. + * + * The locks are acquired in order from lowest address to highest address. + * (Note that this is not necessarily the same algorithm used by std::lock().) + * + * For parameters that are const and support shared locks, a read lock is + * acquired. Otherwise an exclusive lock is acquired. + * + * TODO: Extend acquireLocked() with variadic template versions that + * allow for more than 2 Synchronized arguments. (I haven't given too much + * thought about how to implement this. It seems like it would be rather + * complicated, but I think it should be possible.) + */ +template +std::tuple, detail::LockedPtrType> +acquireLocked(Sync1& l1, Sync2& l2) { + if (static_cast(&l1) < static_cast(&l2)) { + auto p1 = l1.contextualLock(); + auto p2 = l2.contextualLock(); + return std::make_tuple(std::move(p1), std::move(p2)); + } else { + auto p2 = l2.contextualLock(); + auto p1 = l1.contextualLock(); + return std::make_tuple(std::move(p1), std::move(p2)); + } +} + +/** + * A version of acquireLocked() that returns a std::pair rather than a + * std::tuple, which is easier to use in many places. + */ +template +std::pair, detail::LockedPtrType> +acquireLockedPair(Sync1& l1, Sync2& l2) { + auto lockedPtrs = acquireLocked(l1, l2); + return {std::move(std::get<0>(lockedPtrs)), + std::move(std::get<1>(lockedPtrs))}; +} + +/************************************************************************ + * NOTE: All APIs below this line will be deprecated in upcoming diffs. + ************************************************************************/ + // Non-member swap primitive template void swap(Synchronized& lhs, Synchronized& rhs) { @@ -537,7 +915,9 @@ void swap(Synchronized& lhs, Synchronized& rhs) { !SYNCHRONIZED_state; \ SYNCHRONIZED_state = true) \ for (auto FB_VA_GLUE(FB_ARG_1, (__VA_ARGS__)) = \ - SYNCHRONIZED_lockedPtr.operator->(); \ + (!SYNCHRONIZED_lockedPtr \ + ? nullptr \ + : SYNCHRONIZED_lockedPtr.operator->()); \ !SYNCHRONIZED_state; \ SYNCHRONIZED_state = true) @@ -560,47 +940,37 @@ void swap(Synchronized& lhs, Synchronized& rhs) { /** * Temporarily disables synchronization inside a SYNCHRONIZED block. + * + * Note: This macro is deprecated, and kind of broken. The input parameter + * does not control what it unlocks--it always unlocks the lock acquired by the + * most recent SYNCHRONIZED scope. If you have two nested SYNCHRONIZED blocks, + * UNSYNCHRONIZED always unlocks the inner-most, even if you pass in the + * variable name used in the outer SYNCHRONIZED block. + * + * This macro will be removed soon in a subsequent diff. */ -#define UNSYNCHRONIZED(name) \ - for (decltype(SYNCHRONIZED_lockedPtr.typeHackDoNotUse()) \ - SYNCHRONIZED_state3(&SYNCHRONIZED_lockedPtr); \ - !SYNCHRONIZED_state; SYNCHRONIZED_state = true) \ - for (auto& name = *SYNCHRONIZED_state3.operator->(); \ - !SYNCHRONIZED_state; SYNCHRONIZED_state = true) - -/** - * Locks two objects in increasing order of their addresses. - */ -template -void lockInOrder(P1& p1, P2& p2) { - if (static_cast(p1.operator->()) > - static_cast(p2.operator->())) { - p2.acquire(); - p1.acquire(); - } else { - p1.acquire(); - p2.acquire(); - } -} +#define UNSYNCHRONIZED(name) \ + for (auto SYNCHRONIZED_state3 = SYNCHRONIZED_lockedPtr.scopedUnlock(); \ + !SYNCHRONIZED_state; \ + SYNCHRONIZED_state = true) \ + for (auto& name = *SYNCHRONIZED_state3.getSynchronized(); \ + !SYNCHRONIZED_state; \ + SYNCHRONIZED_state = true) /** * Synchronizes two Synchronized objects (they may encapsulate * different data). Synchronization is done in increasing address of * object order, so there is no deadlock risk. */ -#define SYNCHRONIZED_DUAL(n1, e1, n2, e2) \ - if (bool SYNCHRONIZED_state = false) {} else \ - for (auto SYNCHRONIZED_lp1 = (e1).internalDoNotUse(); \ - !SYNCHRONIZED_state; SYNCHRONIZED_state = true) \ - for (auto& n1 = *SYNCHRONIZED_lp1.operator->(); \ - !SYNCHRONIZED_state; SYNCHRONIZED_state = true) \ - for (auto SYNCHRONIZED_lp2 = (e2).internalDoNotUse(); \ - !SYNCHRONIZED_state; SYNCHRONIZED_state = true) \ - for (auto& n2 = *SYNCHRONIZED_lp2.operator->(); \ - !SYNCHRONIZED_state; SYNCHRONIZED_state = true) \ - if ((::folly::lockInOrder( \ - SYNCHRONIZED_lp1, SYNCHRONIZED_lp2), \ - false)) {} \ - else +#define SYNCHRONIZED_DUAL(n1, e1, n2, e2) \ + if (bool SYNCHRONIZED_state = false) { \ + } else \ + for (auto SYNCHRONIZED_ptrs = acquireLockedPair(e1, e2); \ + !SYNCHRONIZED_state; \ + SYNCHRONIZED_state = true) \ + for (auto& n1 = *SYNCHRONIZED_ptrs.first; !SYNCHRONIZED_state; \ + SYNCHRONIZED_state = true) \ + for (auto& n2 = *SYNCHRONIZED_ptrs.second; !SYNCHRONIZED_state; \ + SYNCHRONIZED_state = true) } /* namespace folly */ diff --git a/folly/test/SynchronizedTest.cpp b/folly/test/SynchronizedTest.cpp index d89b9f44..3c07f57e 100644 --- a/folly/test/SynchronizedTest.cpp +++ b/folly/test/SynchronizedTest.cpp @@ -59,10 +59,22 @@ TYPED_TEST(SynchronizedTest, Basic) { testBasic(); } +TYPED_TEST(SynchronizedTest, Deprecated) { + testDeprecated(); +} + TYPED_TEST(SynchronizedTest, Concurrency) { testConcurrency(); } +TYPED_TEST(SynchronizedTest, AcquireLocked) { + testAcquireLocked(); +} + +TYPED_TEST(SynchronizedTest, AcquireLockedWithConst) { + testAcquireLockedWithConst(); +} + TYPED_TEST(SynchronizedTest, DualLocking) { testDualLocking(); } @@ -94,6 +106,10 @@ using SynchronizedTimedTestTypes = testing::Types< folly::SharedMutexWritePriority>; TYPED_TEST_CASE(SynchronizedTimedTest, SynchronizedTimedTestTypes); +TYPED_TEST(SynchronizedTimedTest, Timed) { + testTimed(); +} + TYPED_TEST(SynchronizedTimedTest, TimedSynchronized) { testTimedSynchronized(); } @@ -114,6 +130,10 @@ using SynchronizedTimedWithConstTestTypes = testing::Types< TYPED_TEST_CASE( SynchronizedTimedWithConstTest, SynchronizedTimedWithConstTestTypes); +TYPED_TEST(SynchronizedTimedWithConstTest, TimedShared) { + testTimedShared(); +} + TYPED_TEST(SynchronizedTimedWithConstTest, TimedSynchronizeWithConst) { testTimedSynchronizedWithConst(); } @@ -179,7 +199,7 @@ TEST_F(SynchronizedLockTest, SyncUnSync) { EXPECT_EQ((CountPair{2, 2}), FakeMutex::getLockUnlockCount()); } -// Nested SYNCHRONIZED UNSYNCHRONIZED test, 2 levels for each are used here +// Nested SYNCHRONIZED UNSYNCHRONIZED test, 2 levels of synchronization TEST_F(SynchronizedLockTest, NestedSyncUnSync) { folly::Synchronized, FakeMutex> obj; EXPECT_EQ((CountPair{0, 0}), FakeMutex::getLockUnlockCount()); @@ -187,12 +207,15 @@ TEST_F(SynchronizedLockTest, NestedSyncUnSync) { EXPECT_EQ((CountPair{1, 0}), FakeMutex::getLockUnlockCount()); SYNCHRONIZED(obj) { EXPECT_EQ((CountPair{2, 0}), FakeMutex::getLockUnlockCount()); + // Note: UNSYNCHRONIZED has always been kind of broken here. + // The input parameter is ignored (other than to overwrite what the input + // variable name refers to), and it unlocks the most object acquired in + // the most recent SYNCHRONIZED scope. UNSYNCHRONIZED(obj) { EXPECT_EQ((CountPair{2, 1}), FakeMutex::getLockUnlockCount()); - UNSYNCHRONIZED(obj) { - EXPECT_EQ((CountPair{2, 2}), - FakeMutex::getLockUnlockCount()); - } + } + EXPECT_EQ((CountPair{3, 1}), FakeMutex::getLockUnlockCount()); + UNSYNCHRONIZED(obj) { EXPECT_EQ((CountPair{3, 2}), FakeMutex::getLockUnlockCount()); } EXPECT_EQ((CountPair{4, 2}), FakeMutex::getLockUnlockCount()); @@ -202,7 +225,7 @@ TEST_F(SynchronizedLockTest, NestedSyncUnSync) { EXPECT_EQ((CountPair{4, 4}), FakeMutex::getLockUnlockCount()); } -// Different nesting behavior, UNSYNCHRONIZED called on differen depth of +// Different nesting behavior, UNSYNCHRONIZED called on different depth of // SYNCHRONIZED TEST_F(SynchronizedLockTest, NestedSyncUnSync2) { folly::Synchronized, FakeMutex> obj; diff --git a/folly/test/SynchronizedTestLib-inl.h b/folly/test/SynchronizedTestLib-inl.h index 9c380bce..3166de8a 100644 --- a/folly/test/SynchronizedTestLib-inl.h +++ b/folly/test/SynchronizedTestLib-inl.h @@ -60,30 +60,22 @@ void runParallel(size_t numThreads, const Function& function) { // Variables used to synchronize all threads to try and start them // as close to the same time as possible - // - // TODO: At the moment Synchronized doesn't work with condition variables. - // Update this to use Synchronized once the condition_variable support lands. - std::mutex threadsReadyMutex; - size_t threadsReady = 0; + folly::Synchronized threadsReady(0); std::condition_variable readyCV; - std::mutex goMutex; - bool go = false; + folly::Synchronized go(false); std::condition_variable goCV; auto worker = [&](size_t threadIndex) { // Signal that we are ready - { - std::lock_guard lock(threadsReadyMutex); - ++threadsReady; - } + ++(*threadsReady.lock()); readyCV.notify_one(); // Wait until we are given the signal to start // The purpose of this is to try and make sure all threads start // as close to the same time as possible. { - std::unique_lock lock(goMutex); - goCV.wait(lock, [&] { return go; }); + auto lockedGo = go.lock(); + goCV.wait(lockedGo.getUniqueLock(), [&] { return *lockedGo; }); } function(threadIndex); @@ -96,14 +88,13 @@ void runParallel(size_t numThreads, const Function& function) { // Wait for all threads to become ready { - std::unique_lock lock(threadsReadyMutex); - readyCV.wait(lock, [&] { return threadsReady == numThreads; }); - } - { - std::lock_guard lock(goMutex); - go = true; + auto readyLocked = threadsReady.lock(); + readyCV.wait(readyLocked.getUniqueLock(), [&] { + return *readyLocked == numThreads; + }); } // Now signal the threads that they can go + go = true; goCV.notify_all(); // Wait for all threads to finish @@ -112,8 +103,99 @@ void runParallel(size_t numThreads, const Function& function) { } } +// testBasic() version for shared lock types +template +typename std::enable_if::is_shared>::type +testBasicImpl() { + folly::Synchronized, Mutex> obj; + + obj.wlock()->resize(1000); + + folly::Synchronized, Mutex> obj2{*obj.wlock()}; + EXPECT_EQ(1000, obj2.rlock()->size()); + + { + auto lockedObj = obj.wlock(); + lockedObj->push_back(10); + EXPECT_EQ(1001, lockedObj->size()); + EXPECT_EQ(10, lockedObj->back()); + EXPECT_EQ(1000, obj2.wlock()->size()); + EXPECT_EQ(1000, obj2.rlock()->size()); + + { + auto unlocker = lockedObj.scopedUnlock(); + EXPECT_EQ(1001, obj.wlock()->size()); + } + } + + { + auto lockedObj = obj.rlock(); + EXPECT_EQ(1001, lockedObj->size()); + EXPECT_EQ(1001, obj.rlock()->size()); + { + auto unlocker = lockedObj.scopedUnlock(); + EXPECT_EQ(1001, obj.wlock()->size()); + } + } + + obj.wlock()->front() = 2; + + { + const auto& constObj = obj; + // contextualLock() on a const reference should grab a shared lock + auto lockedObj = constObj.contextualLock(); + EXPECT_EQ(2, lockedObj->front()); + EXPECT_EQ(2, constObj.rlock()->front()); + EXPECT_EQ(2, obj.rlock()->front()); + } + + EXPECT_EQ(1001, obj.rlock()->size()); + EXPECT_EQ(2, obj.rlock()->front()); + EXPECT_EQ(10, obj.rlock()->back()); + EXPECT_EQ(1000, obj2.rlock()->size()); +} + +// testBasic() version for non-shared lock types +template +typename std::enable_if::is_shared>::type +testBasicImpl() { + folly::Synchronized, Mutex> obj; + + obj.lock()->resize(1000); + + folly::Synchronized, Mutex> obj2{*obj.lock()}; + EXPECT_EQ(1000, obj2.lock()->size()); + + { + auto lockedObj = obj.lock(); + lockedObj->push_back(10); + EXPECT_EQ(1001, lockedObj->size()); + EXPECT_EQ(10, lockedObj->back()); + EXPECT_EQ(1000, obj2.lock()->size()); + + { + auto unlocker = lockedObj.scopedUnlock(); + EXPECT_EQ(1001, obj.lock()->size()); + } + } + + obj.lock()->front() = 2; + + EXPECT_EQ(1001, obj.lock()->size()); + EXPECT_EQ(2, obj.lock()->front()); + EXPECT_EQ(2, obj.contextualLock()->front()); + EXPECT_EQ(10, obj.lock()->back()); + EXPECT_EQ(1000, obj2.lock()->size()); +} + template void testBasic() { + testBasicImpl(); +} + +// Testing the deprecated SYNCHRONIZED and SYNCHRONIZED_CONST APIs +template +void testDeprecated() { folly::Synchronized, Mutex> obj; obj->resize(1000); @@ -163,7 +245,7 @@ template void testConcurrency() { auto pushNumbers = [&](size_t threadIdx) { // Test lock() for (size_t n = 0; n < itersPerThread; ++n) { - v->push_back((itersPerThread * threadIdx) + n); + v.contextualLock()->push_back((itersPerThread * threadIdx) + n); sched_yield(); } }; @@ -180,6 +262,71 @@ template void testConcurrency() { } } +template +void testAcquireLocked() { + folly::Synchronized, Mutex> v; + folly::Synchronized, Mutex> m; + + auto dualLockWorker = [&](size_t threadIdx) { + // Note: this will be less awkward with C++ 17's structured + // binding functionality, which will make it easier to use the returned + // std::tuple. + if (threadIdx & 1) { + auto ret = acquireLocked(v, m); + std::get<0>(ret)->push_back(threadIdx); + (*std::get<1>(ret))[threadIdx] = threadIdx + 1; + } else { + auto ret = acquireLocked(m, v); + std::get<1>(ret)->push_back(threadIdx); + (*std::get<0>(ret))[threadIdx] = threadIdx + 1; + } + }; + static const size_t numThreads = 100; + runParallel(numThreads, dualLockWorker); + + std::vector result; + v.swap(result); + + EXPECT_EQ(numThreads, result.size()); + sort(result.begin(), result.end()); + + for (size_t i = 0; i < numThreads; ++i) { + EXPECT_EQ(i, result[i]); + } +} + +template +void testAcquireLockedWithConst() { + folly::Synchronized, Mutex> v; + folly::Synchronized, Mutex> m; + + auto dualLockWorker = [&](size_t threadIdx) { + const auto& cm = m; + if (threadIdx & 1) { + auto ret = acquireLocked(v, cm); + (void)std::get<1>(ret)->size(); + std::get<0>(ret)->push_back(threadIdx); + } else { + auto ret = acquireLocked(cm, v); + (void)std::get<0>(ret)->size(); + std::get<1>(ret)->push_back(threadIdx); + } + }; + static const size_t numThreads = 100; + runParallel(numThreads, dualLockWorker); + + std::vector result; + v.swap(result); + + EXPECT_EQ(numThreads, result.size()); + sort(result.begin(), result.end()); + + for (size_t i = 0; i < numThreads; ++i) { + EXPECT_EQ(i, result[i]); + } +} + +// Testing the deprecated SYNCHRONIZED_DUAL API template void testDualLocking() { folly::Synchronized, Mutex> v; folly::Synchronized, Mutex> m; @@ -211,6 +358,7 @@ template void testDualLocking() { } } +// Testing the deprecated SYNCHRONIZED_DUAL API template void testDualLockingWithConst() { folly::Synchronized, Mutex> v; folly::Synchronized, Mutex> m; @@ -243,6 +391,117 @@ template void testDualLockingWithConst() { } } +template +void testTimed() { + folly::Synchronized, Mutex> v; + folly::Synchronized numTimeouts; + + auto worker = [&](size_t threadIdx) { + // Test directly using operator-> on the lock result + v.contextualLock()->push_back(2 * threadIdx); + + // Test using lock with a timeout + for (;;) { + auto lv = v.contextualLock(std::chrono::milliseconds(5)); + if (!lv) { + ++(*numTimeouts.contextualLock()); + continue; + } + + // Sleep for a random time to ensure we trigger timeouts + // in other threads + randomSleep(std::chrono::milliseconds(5), std::chrono::milliseconds(15)); + lv->push_back(2 * threadIdx + 1); + break; + } + }; + + static const size_t numThreads = 100; + runParallel(numThreads, worker); + + std::vector result; + v.swap(result); + + EXPECT_EQ(2 * numThreads, result.size()); + sort(result.begin(), result.end()); + + for (size_t i = 0; i < 2 * numThreads; ++i) { + EXPECT_EQ(i, result[i]); + } + // We generally expect a large number of number timeouts here. + // I'm not adding a check for it since it's theoretically possible that + // we might get 0 timeouts depending on the CPU scheduling if our threads + // don't get to run very often. + LOG(INFO) << "testTimed: " << *numTimeouts.contextualRLock() << " timeouts"; + + // Make sure we can lock with various timeout duration units + { + auto lv = v.contextualLock(std::chrono::milliseconds(5)); + EXPECT_TRUE(lv); + EXPECT_FALSE(lv.isNull()); + auto lv2 = v.contextualLock(std::chrono::microseconds(5)); + // We may or may not acquire lv2 successfully, depending on whether + // or not this is a recursive mutex type. + } + { + auto lv = v.contextualLock(std::chrono::seconds(1)); + EXPECT_TRUE(lv); + } +} + +template +void testTimedShared() { + folly::Synchronized, Mutex> v; + folly::Synchronized numTimeouts; + + auto worker = [&](size_t threadIdx) { + // Test directly using operator-> on the lock result + v.wlock()->push_back(threadIdx); + + // Test lock() with a timeout + for (;;) { + auto lv = v.rlock(std::chrono::milliseconds(10)); + if (!lv) { + ++(*numTimeouts.contextualLock()); + continue; + } + + // Sleep while holding the lock. + // + // This will block other threads from acquiring the write lock to add + // their thread index to v, but it won't block threads that have entered + // the for loop and are trying to acquire a read lock. + // + // For lock types that give preference to readers rather than writers, + // this will tend to serialize all threads on the wlock() above. + randomSleep(std::chrono::milliseconds(5), std::chrono::milliseconds(15)); + auto found = std::find(lv->begin(), lv->end(), threadIdx); + CHECK(found != lv->end()); + break; + } + }; + + static const size_t numThreads = 100; + runParallel(numThreads, worker); + + std::vector result; + v.swap(result); + + EXPECT_EQ(numThreads, result.size()); + sort(result.begin(), result.end()); + + for (size_t i = 0; i < numThreads; ++i) { + EXPECT_EQ(i, result[i]); + } + // We generally expect a small number of timeouts here. + // For locks that give readers preference over writers this should usually + // be 0. With locks that give writers preference we do see a small-ish + // number of read timeouts. + LOG(INFO) << "testTimedShared: " << *numTimeouts.contextualRLock() + << " timeouts"; +} + +// Testing the deprecated TIMED_SYNCHRONIZED API template void testTimedSynchronized() { folly::Synchronized, Mutex> v; folly::Synchronized numTimeouts; @@ -263,9 +522,7 @@ template void testTimedSynchronized() { return; } - SYNCHRONIZED(numTimeouts) { - ++numTimeouts; - } + ++(*numTimeouts.contextualLock()); } }; @@ -285,13 +542,11 @@ template void testTimedSynchronized() { // I'm not adding a check for it since it's theoretically possible that // we might get 0 timeouts depending on the CPU scheduling if our threads // don't get to run very often. - uint64_t finalNumTimeouts = 0; - SYNCHRONIZED(numTimeouts) { - finalNumTimeouts = numTimeouts; - } - LOG(INFO) << "testTimedSynchronized: " << finalNumTimeouts << " timeouts"; + LOG(INFO) << "testTimedSynchronized: " << *numTimeouts.contextualRLock() + << " timeouts"; } +// Testing the deprecated TIMED_SYNCHRONIZED_CONST API template void testTimedSynchronizedWithConst() { folly::Synchronized, Mutex> v; folly::Synchronized numTimeouts; @@ -318,9 +573,7 @@ template void testTimedSynchronizedWithConst() { CHECK(found != lv->end()); return; } else { - SYNCHRONIZED(numTimeouts) { - ++numTimeouts; - } + ++(*numTimeouts.contextualLock()); } } } @@ -342,12 +595,8 @@ template void testTimedSynchronizedWithConst() { // For locks that give readers preference over writers this should usually // be 0. With locks that give writers preference we do see a small-ish // number of read timeouts. - uint64_t finalNumTimeouts = 0; - SYNCHRONIZED(numTimeouts) { - finalNumTimeouts = numTimeouts; - } - LOG(INFO) << "testTimedSynchronizedWithConst: " << finalNumTimeouts - << " timeouts"; + LOG(INFO) << "testTimedSynchronizedWithConst: " + << *numTimeouts.contextualRLock() << " timeouts"; } template void testConstCopy() { diff --git a/folly/test/SynchronizedTestLib.h b/folly/test/SynchronizedTestLib.h index 5bf96955..fe37f204 100644 --- a/folly/test/SynchronizedTestLib.h +++ b/folly/test/SynchronizedTestLib.h @@ -30,14 +30,34 @@ namespace folly { namespace sync_tests { -template void testBasic(); -template void testConcurrency(); -template void testDualLocking(); -template void testDualLockingWithConst(); -template void testTimedSynchronized(); -template void testTimedSynchronizedWithConst(); -template void testConstCopy(); -template void testInPlaceConstruction(); +template +void testBasic(); +template +void testDeprecated(); +template +void testConcurrency(); +template +void testAcquireLocked(); +template +void testAcquireLockedWithConst(); +template +void testDualLockingWithConst(); +template +void testDualLocking(); +template +void testDualLockingWithConst(); +template +void testTimed(); +template +void testTimedShared(); +template +void testTimedSynchronizedDeprecated(); +template +void testTimedSynchronizedWithConst(); +template +void testConstCopy(); +template +void testInPlaceConstruction(); } } -- 2.34.1