Commit a238a687 authored by Giuseppe Ottaviano's avatar Giuseppe Ottaviano Committed by Facebook Github Bot

Disable non-const access from upgrade locks

Summary:
Upgrade locks allow access concurrently with readers, so mutating the state under an upgrade lock can potentially introduce a race unless proven otherwise.
The main goal of this diff is to make the accessors to `Synchronized` state `const` under upgrade lock, as it happens under a shared lock. To achieve so, the diff changes the mechanism by which `const` is added: instead of unconditionally closing a `const Synchronized` in the `LockedPtr`, it captures the same constness as the `Synchronized`, and then the `const` decision is done at access time.

This enables having an extra method `asNonConstUnsafe()` that gives non-const access when needed without requiring a `const_cast`. This is now provided for shared locks as well.

Reviewed By: yfeldblum, aary, davidtgoldblatt

Differential Revision: D13817413

fbshipit-source-id: 8c315a925db9b1da8821984c59e55e90571b194e
parent 5f535514
...@@ -370,6 +370,8 @@ struct LockTraitsImpl<Mutex, MutexLevel::UPGRADE, true> ...@@ -370,6 +370,8 @@ struct LockTraitsImpl<Mutex, MutexLevel::UPGRADE, true>
*/ */
template <template <typename...> class LockTraits> template <template <typename...> class LockTraits>
struct UnlockPolicyExclusive { struct UnlockPolicyExclusive {
constexpr static bool allows_concurrent_access = false;
template <typename Mutex> template <typename Mutex>
static void unlock(Mutex& mutex) { static void unlock(Mutex& mutex) {
LockTraits<Mutex>::unlock(mutex); LockTraits<Mutex>::unlock(mutex);
...@@ -377,6 +379,8 @@ struct UnlockPolicyExclusive { ...@@ -377,6 +379,8 @@ struct UnlockPolicyExclusive {
}; };
template <template <typename...> class LockTraits> template <template <typename...> class LockTraits>
struct UnlockPolicyShared { struct UnlockPolicyShared {
constexpr static bool allows_concurrent_access = true;
template <typename Mutex> template <typename Mutex>
static void unlock(Mutex& mutex) { static void unlock(Mutex& mutex) {
LockTraits<Mutex>::unlock_shared(mutex); LockTraits<Mutex>::unlock_shared(mutex);
...@@ -384,6 +388,8 @@ struct UnlockPolicyShared { ...@@ -384,6 +388,8 @@ struct UnlockPolicyShared {
}; };
template <template <typename...> class LockTraits> template <template <typename...> class LockTraits>
struct UnlockPolicyUpgrade { struct UnlockPolicyUpgrade {
constexpr static bool allows_concurrent_access = true;
template <typename Mutex> template <typename Mutex>
static void unlock(Mutex& mutex) { static void unlock(Mutex& mutex) {
LockTraits<Mutex>::unlock_upgrade(mutex); LockTraits<Mutex>::unlock_upgrade(mutex);
......
...@@ -83,15 +83,25 @@ class SynchronizedBase; ...@@ -83,15 +83,25 @@ class SynchronizedBase;
template <class Subclass> template <class Subclass>
class SynchronizedBase<Subclass, detail::MutexLevel::SHARED> { class SynchronizedBase<Subclass, detail::MutexLevel::SHARED> {
public: public:
using LockedPtr = ::folly::LockedPtr<Subclass, LockPolicyExclusive>; using WLockedPtr = ::folly::LockedPtr<Subclass, LockPolicyExclusive>;
using ConstWLockedPtr = using ConstWLockedPtr =
::folly::LockedPtr<const Subclass, LockPolicyExclusive>; ::folly::LockedPtr<const Subclass, LockPolicyExclusive>;
using ConstLockedPtr = ::folly::LockedPtr<const Subclass, LockPolicyShared>;
using RLockedPtr = ::folly::LockedPtr<const Subclass, LockPolicyShared>;
using ConstRLockedPtr = ::folly::LockedPtr<const Subclass, LockPolicyShared>;
using TryWLockedPtr = ::folly::LockedPtr<Subclass, LockPolicyTryExclusive>; using TryWLockedPtr = ::folly::LockedPtr<Subclass, LockPolicyTryExclusive>;
using ConstTryWLockedPtr = using ConstTryWLockedPtr =
::folly::LockedPtr<const Subclass, LockPolicyTryExclusive>; ::folly::LockedPtr<const Subclass, LockPolicyTryExclusive>;
using TryRLockedPtr = ::folly::LockedPtr<const Subclass, LockPolicyTryShared>;
using TryRLockedPtr = ::folly::LockedPtr<Subclass, LockPolicyTryShared>;
using ConstTryRLockedPtr =
::folly::LockedPtr<const Subclass, LockPolicyTryShared>;
// These aliases are deprecated.
// TODO: Codemod them away.
using LockedPtr = WLockedPtr;
using ConstLockedPtr = ConstRLockedPtr;
/** /**
* Acquire an exclusive lock, and return a LockedPtr that can be used to * Acquire an exclusive lock, and return a LockedPtr that can be used to
...@@ -122,9 +132,13 @@ class SynchronizedBase<Subclass, detail::MutexLevel::SHARED> { ...@@ -122,9 +132,13 @@ class SynchronizedBase<Subclass, detail::MutexLevel::SHARED> {
} }
/** /**
* Acquire a read lock, and return a ConstLockedPtr that can be used to * Acquire a read lock. The returned LockedPtr will have force const
* safely access the datum. * access to the data unless the lock is acquired in non-const
* context and asNonConstUnsafe() is used.
*/ */
RLockedPtr rlock() {
return RLockedPtr(static_cast<Subclass*>(this));
}
ConstLockedPtr rlock() const { ConstLockedPtr rlock() const {
return ConstLockedPtr(static_cast<const Subclass*>(this)); return ConstLockedPtr(static_cast<const Subclass*>(this));
} }
...@@ -136,8 +150,11 @@ class SynchronizedBase<Subclass, detail::MutexLevel::SHARED> { ...@@ -136,8 +150,11 @@ class SynchronizedBase<Subclass, detail::MutexLevel::SHARED> {
* (Use LockedPtr::operator bool() or LockedPtr::isNull() to check for * (Use LockedPtr::operator bool() or LockedPtr::isNull() to check for
* validity.) * validity.)
*/ */
TryRLockedPtr tryRLock() const { TryRLockedPtr tryRLock() {
return TryRLockedPtr{static_cast<const Subclass*>(this)}; return TryRLockedPtr{static_cast<Subclass*>(this)};
}
ConstTryRLockedPtr tryRLock() const {
return ConstTryRLockedPtr{static_cast<const Subclass*>(this)};
} }
/** /**
...@@ -221,6 +238,11 @@ class SynchronizedBase<Subclass, detail::MutexLevel::SHARED> { ...@@ -221,6 +238,11 @@ class SynchronizedBase<Subclass, detail::MutexLevel::SHARED> {
return function(*rlock()); return function(*rlock());
} }
template <class Function>
auto withRLockPtr(Function&& function) {
return function(rlock());
}
template <class Function> template <class Function>
auto withRLockPtr(Function&& function) const { auto withRLockPtr(Function&& function) const {
return function(rlock()); return function(rlock());
...@@ -248,14 +270,16 @@ class SynchronizedBase<Subclass, detail::MutexLevel::UPGRADE> ...@@ -248,14 +270,16 @@ class SynchronizedBase<Subclass, detail::MutexLevel::UPGRADE>
::folly::LockedPtr<const Subclass, LockPolicyTryUpgrade>; ::folly::LockedPtr<const Subclass, LockPolicyTryUpgrade>;
/** /**
* Acquire an upgrade lock and return a LockedPtr that can be used to safely * Acquire an upgrade lock. The returned LockedPtr will have force
* access the datum * const access to the data unless the lock is acquired in non-const
* * context and asNonConstUnsafe() is used.
* And the const version
*/ */
UpgradeLockedPtr ulock() { UpgradeLockedPtr ulock() {
return UpgradeLockedPtr(static_cast<Subclass*>(this)); return UpgradeLockedPtr(static_cast<Subclass*>(this));
} }
ConstUpgradeLockedPtr ulock() const {
return ConstUpgradeLockedPtr(static_cast<const Subclass*>(this));
}
/** /**
* Attempts to acquire the lock in upgrade mode. If acquisition is * Attempts to acquire the lock in upgrade mode. If acquisition is
...@@ -303,6 +327,10 @@ class SynchronizedBase<Subclass, detail::MutexLevel::UPGRADE> ...@@ -303,6 +327,10 @@ class SynchronizedBase<Subclass, detail::MutexLevel::UPGRADE>
auto withULock(Function&& function) { auto withULock(Function&& function) {
return function(*ulock()); return function(*ulock());
} }
template <class Function>
auto withULock(Function&& function) const {
return function(*ulock());
}
/** /**
* Invoke a function while holding the lock exclusively. * Invoke a function while holding the lock exclusively.
...@@ -320,6 +348,10 @@ class SynchronizedBase<Subclass, detail::MutexLevel::UPGRADE> ...@@ -320,6 +348,10 @@ class SynchronizedBase<Subclass, detail::MutexLevel::UPGRADE>
auto withULockPtr(Function&& function) { auto withULockPtr(Function&& function) {
return function(ulock()); return function(ulock());
} }
template <class Function>
auto withULockPtr(Function&& function) const {
return function(ulock());
}
}; };
/** /**
...@@ -797,9 +829,9 @@ namespace detail { ...@@ -797,9 +829,9 @@ namespace detail {
* A helper alias that resolves to "const T" if the template parameter * A helper alias that resolves to "const T" if the template parameter
* is a const Synchronized<T>, or "T" if the parameter is not const. * is a const Synchronized<T>, or "T" if the parameter is not const.
*/ */
template <class SynchronizedType> template <class SynchronizedType, bool AllowsConcurrentAccess>
using SynchronizedDataType = typename std::conditional< using SynchronizedDataType = typename std::conditional<
std::is_const<SynchronizedType>::value, AllowsConcurrentAccess || std::is_const<SynchronizedType>::value,
typename SynchronizedType::DataType const, typename SynchronizedType::DataType const,
typename SynchronizedType::DataType>::type; typename SynchronizedType::DataType>::type;
/* /*
...@@ -1311,9 +1343,12 @@ class LockedPtr : public LockedPtrBase< ...@@ -1311,9 +1343,12 @@ class LockedPtr : public LockedPtrBase<
SynchronizedType, SynchronizedType,
typename SynchronizedType::MutexType, typename SynchronizedType::MutexType,
LockPolicy>; LockPolicy>;
constexpr static bool AllowsConcurrentAccess =
LockPolicy::allows_concurrent_access;
using UnlockerData = typename Base::UnlockerData; using UnlockerData = typename Base::UnlockerData;
// CDataType is the DataType with the appropriate const-qualification // CDataType is the DataType with the appropriate const-qualification
using CDataType = detail::SynchronizedDataType<SynchronizedType>; using CDataType =
detail::SynchronizedDataType<SynchronizedType, AllowsConcurrentAccess>;
// Enable only if the unlock policy of the other LockPolicy is the same as // Enable only if the unlock policy of the other LockPolicy is the same as
// ours // ours
template <typename LockPolicyOther> template <typename LockPolicyOther>
...@@ -1432,6 +1467,34 @@ class LockedPtr : public LockedPtrBase< ...@@ -1432,6 +1467,34 @@ class LockedPtr : public LockedPtrBase<
return this->parent_->datum_; return this->parent_->datum_;
} }
/**
* Locks that allow concurrent access (shared, upgrade) force const
* access with the standard accessors even if the Synchronized
* object is non-const.
*
* In some cases non-const access can be needed, for example:
*
* - Under an upgrade lock, to get references that will be mutated
* after upgrading to a write lock.
*
* - Under an read lock, if some mutating operations on the data
* are thread safe (e.g. mutating the value in an associative
* container with reference stability).
*
* asNonConstUnsafe() returns a non-const reference to the data if
* the parent Synchronized object was non-const at the point of lock
* acquisition.
*/
template <typename = void>
DataType& asNonConstUnsafe() const {
static_assert(
AllowsConcurrentAccess && !std::is_const<SynchronizedType>::value,
"asNonConstUnsafe() is only available on non-exclusive locks"
" acquired in a non-const context");
return this->parent_->datum_;
}
/** /**
* Temporarily unlock the LockedPtr, and reset it to null. * Temporarily unlock the LockedPtr, and reset it to null.
* *
......
...@@ -89,10 +89,12 @@ bool RequestContext::doSetContextData( ...@@ -89,10 +89,12 @@ bool RequestContext::doSetContextData(
std::unique_ptr<RequestData>& data, std::unique_ptr<RequestData>& data,
DoSetBehaviour behaviour) { DoSetBehaviour behaviour) {
auto ulock = state_.ulock(); auto ulock = state_.ulock();
// Need non-const iterators to use under write lock.
auto& state = ulock.asNonConstUnsafe();
bool conflict = false; bool conflict = false;
auto it = ulock->requestData_.find(val); auto it = state.requestData_.find(val);
if (it != ulock->requestData_.end()) { if (it != state.requestData_.end()) {
if (behaviour == DoSetBehaviour::SET_IF_ABSENT) { if (behaviour == DoSetBehaviour::SET_IF_ABSENT) {
return false; return false;
} else if (behaviour == DoSetBehaviour::SET) { } else if (behaviour == DoSetBehaviour::SET) {
...@@ -179,8 +181,10 @@ void RequestContext::clearContextData(const RequestToken& val) { ...@@ -179,8 +181,10 @@ void RequestContext::clearContextData(const RequestToken& val) {
// RequestData destructors will try to grab the lock again. // RequestData destructors will try to grab the lock again.
{ {
auto ulock = state_.ulock(); auto ulock = state_.ulock();
auto it = ulock->requestData_.find(val); // Need non-const iterators to use under write lock.
if (it == ulock->requestData_.end()) { auto& state = ulock.asNonConstUnsafe();
auto it = state.requestData_.find(val);
if (it == state.requestData_.end()) {
return; return;
} }
......
...@@ -385,6 +385,9 @@ TEST_F(SynchronizedLockTest, UpgradeLocking) { ...@@ -385,6 +385,9 @@ TEST_F(SynchronizedLockTest, UpgradeLocking) {
{ {
auto ulock = sync.ulock(); auto ulock = sync.ulock();
EXPECT_TRUE((std::is_same<decltype(*ulock), const int&>::value));
EXPECT_TRUE(
(std::is_same<decltype(ulock.asNonConstUnsafe()), int&>::value));
EXPECT_EQ( EXPECT_EQ(
globalAllPowerfulAssertingMutex.lock_state, globalAllPowerfulAssertingMutex.lock_state,
FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UPGRADE); FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UPGRADE);
...@@ -399,6 +402,7 @@ TEST_F(SynchronizedLockTest, UpgradeLocking) { ...@@ -399,6 +402,7 @@ TEST_F(SynchronizedLockTest, UpgradeLocking) {
{ {
auto ulock = sync.ulock(); auto ulock = sync.ulock();
auto wlock = ulock.moveFromUpgradeToWrite(); auto wlock = ulock.moveFromUpgradeToWrite();
EXPECT_TRUE((std::is_same<decltype(*wlock), int&>::value));
EXPECT_EQ(static_cast<bool>(ulock), false); EXPECT_EQ(static_cast<bool>(ulock), false);
EXPECT_EQ( EXPECT_EQ(
globalAllPowerfulAssertingMutex.lock_state, globalAllPowerfulAssertingMutex.lock_state,
...@@ -430,6 +434,9 @@ TEST_F(SynchronizedLockTest, UpgradeLocking) { ...@@ -430,6 +434,9 @@ TEST_F(SynchronizedLockTest, UpgradeLocking) {
auto wlock = sync.wlock(); auto wlock = sync.wlock();
auto ulock = wlock.moveFromWriteToUpgrade(); auto ulock = wlock.moveFromWriteToUpgrade();
EXPECT_EQ(static_cast<bool>(wlock), false); EXPECT_EQ(static_cast<bool>(wlock), false);
EXPECT_TRUE((std::is_same<decltype(*ulock), const int&>::value));
EXPECT_TRUE(
(std::is_same<decltype(ulock.asNonConstUnsafe()), int&>::value));
EXPECT_EQ( EXPECT_EQ(
globalAllPowerfulAssertingMutex.lock_state, globalAllPowerfulAssertingMutex.lock_state,
FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UPGRADE); FakeAllPowerfulAssertingMutexInternal::CurrentLockState::UPGRADE);
...@@ -445,6 +452,9 @@ TEST_F(SynchronizedLockTest, UpgradeLocking) { ...@@ -445,6 +452,9 @@ TEST_F(SynchronizedLockTest, UpgradeLocking) {
auto wlock = sync.wlock(); auto wlock = sync.wlock();
auto slock = wlock.moveFromWriteToRead(); auto slock = wlock.moveFromWriteToRead();
EXPECT_EQ(static_cast<bool>(wlock), false); EXPECT_EQ(static_cast<bool>(wlock), false);
EXPECT_TRUE((std::is_same<decltype(*slock), const int&>::value));
EXPECT_TRUE(
(std::is_same<decltype(slock.asNonConstUnsafe()), int&>::value));
EXPECT_EQ( EXPECT_EQ(
globalAllPowerfulAssertingMutex.lock_state, globalAllPowerfulAssertingMutex.lock_state,
FakeAllPowerfulAssertingMutexInternal::CurrentLockState::SHARED); FakeAllPowerfulAssertingMutexInternal::CurrentLockState::SHARED);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment