Commit 2b06a716 authored by Andrii Grynenko's avatar Andrii Grynenko Committed by Facebook Github Bot

Fix TimedMutex deadlock when used both from fiber and main context

Summary: TimedMutex is a fair mutex, which can cause a deadlock if same mutex is requested first in a fiber, and then in main context.

Reviewed By: yfeldblum

Differential Revision: D4209155

fbshipit-source-id: 0623d9a2e6a0b5cc310fb71ad1b1cf33afd6a30e
parent 9660cdb0
...@@ -22,80 +22,127 @@ namespace fibers { ...@@ -22,80 +22,127 @@ namespace fibers {
// TimedMutex implementation // TimedMutex implementation
// //
template <typename BatonType> template <typename WaitFunc>
void TimedMutex<BatonType>::lock() { TimedMutex::LockResult TimedMutex::lockHelper(WaitFunc&& waitFunc) {
pthread_spin_lock(&lock_); std::unique_lock<folly::SpinLock> lock(lock_);
if (!locked_) { if (!locked_) {
locked_ = true; locked_ = true;
pthread_spin_unlock(&lock_); return LockResult::SUCCESS;
return; }
const auto isOnFiber = onFiber();
if (!isOnFiber && notifiedFiber_ != nullptr) {
// lock() was called on a thread and while some other fiber was already
// notified, it hasn't be run yet. We steal the lock from that fiber then
// to avoid potential deadlock.
DCHECK(threadWaiters_.empty());
notifiedFiber_ = nullptr;
return LockResult::SUCCESS;
} }
// Delay constructing the waiter until it is actually required. // Delay constructing the waiter until it is actually required.
// This makes a huge difference, at least in the benchmarks, // This makes a huge difference, at least in the benchmarks,
// when the mutex isn't locked. // when the mutex isn't locked.
MutexWaiter waiter; MutexWaiter waiter;
waiters_.push_back(waiter); if (isOnFiber) {
pthread_spin_unlock(&lock_); fiberWaiters_.push_back(waiter);
waiter.baton.wait(); } else {
threadWaiters_.push_back(waiter);
}
lock.unlock();
if (!waitFunc(waiter)) {
return LockResult::TIMEOUT;
}
if (isOnFiber) {
auto lockStolen = [&] {
std::lock_guard<folly::SpinLock> lg(lock_);
auto lockStolen = notifiedFiber_ != &waiter;
notifiedFiber_ = nullptr;
return lockStolen;
}();
if (lockStolen) {
return LockResult::STOLEN;
}
}
return LockResult::SUCCESS;
} }
template <typename BatonType> inline void TimedMutex::lock() {
template <typename Rep, typename Period> auto result = lockHelper([](MutexWaiter& waiter) {
bool TimedMutex<BatonType>::timed_lock( waiter.baton.wait();
const std::chrono::duration<Rep, Period>& duration) {
pthread_spin_lock(&lock_);
if (!locked_) {
locked_ = true;
pthread_spin_unlock(&lock_);
return true; return true;
});
DCHECK(result != LockResult::TIMEOUT);
if (result == LockResult::SUCCESS) {
return;
} }
lock();
}
MutexWaiter waiter; template <typename Rep, typename Period>
waiters_.push_back(waiter); bool TimedMutex::timed_lock(
pthread_spin_unlock(&lock_); const std::chrono::duration<Rep, Period>& duration) {
auto result = lockHelper([&](MutexWaiter& waiter) {
if (!waiter.baton.timed_wait(duration)) {
// We timed out. Two cases:
// 1. We're still in the waiter list and we truly timed out
// 2. We're not in the waiter list anymore. This could happen if the baton
// times out but the mutex is unlocked before we reach this code. In
// this
// case we'll pretend we got the lock on time.
std::lock_guard<folly::SpinLock> lg(lock_);
if (waiter.hook.is_linked()) {
waiter.hook.unlink();
return false;
}
}
return true;
});
if (!waiter.baton.timed_wait(duration)) { switch (result) {
// We timed out. Two cases: case LockResult::SUCCESS:
// 1. We're still in the waiter list and we truly timed out return true;
// 2. We're not in the waiter list anymore. This could happen if the baton case LockResult::TIMEOUT:
// times out but the mutex is unlocked before we reach this code. In this
// case we'll pretend we got the lock on time.
pthread_spin_lock(&lock_);
if (waiter.hook.is_linked()) {
waiters_.erase(waiters_.iterator_to(waiter));
pthread_spin_unlock(&lock_);
return false; return false;
} case LockResult::STOLEN:
pthread_spin_unlock(&lock_); // We don't respect the duration if lock was stolen
lock();
return true;
} }
return true; assume_unreachable();
} }
template <typename BatonType> inline bool TimedMutex::try_lock() {
bool TimedMutex<BatonType>::try_lock() { std::lock_guard<folly::SpinLock> lg(lock_);
pthread_spin_lock(&lock_);
if (locked_) { if (locked_) {
pthread_spin_unlock(&lock_);
return false; return false;
} }
locked_ = true; locked_ = true;
pthread_spin_unlock(&lock_);
return true; return true;
} }
template <typename BatonType> inline void TimedMutex::unlock() {
void TimedMutex<BatonType>::unlock() { std::lock_guard<folly::SpinLock> lg(lock_);
pthread_spin_lock(&lock_); if (!threadWaiters_.empty()) {
if (waiters_.empty()) { auto& to_wake = threadWaiters_.front();
threadWaiters_.pop_front();
to_wake.baton.post();
} else if (!fiberWaiters_.empty()) {
auto& to_wake = fiberWaiters_.front();
fiberWaiters_.pop_front();
notifiedFiber_ = &to_wake;
to_wake.baton.post();
} else {
locked_ = false; locked_ = false;
pthread_spin_unlock(&lock_);
return;
} }
MutexWaiter& to_wake = waiters_.front();
waiters_.pop_front();
to_wake.baton.post();
pthread_spin_unlock(&lock_);
} }
// //
......
...@@ -17,6 +17,8 @@ ...@@ -17,6 +17,8 @@
#include <pthread.h> #include <pthread.h>
#include <folly/IntrusiveList.h>
#include <folly/SpinLock.h>
#include <folly/fibers/GenericBaton.h> #include <folly/fibers/GenericBaton.h>
namespace folly { namespace folly {
...@@ -27,15 +29,14 @@ namespace fibers { ...@@ -27,15 +29,14 @@ namespace fibers {
* *
* Like mutex but allows timed_lock in addition to lock and try_lock. * Like mutex but allows timed_lock in addition to lock and try_lock.
**/ **/
template <typename BatonType>
class TimedMutex { class TimedMutex {
public: public:
TimedMutex() { TimedMutex() {}
pthread_spin_init(&lock_, PTHREAD_PROCESS_PRIVATE);
}
~TimedMutex() { ~TimedMutex() {
pthread_spin_destroy(&lock_); DCHECK(threadWaiters_.empty());
DCHECK(fiberWaiters_.empty());
DCHECK(notifiedFiber_ == nullptr);
} }
TimedMutex(const TimedMutex& rhs) = delete; TimedMutex(const TimedMutex& rhs) = delete;
...@@ -59,28 +60,25 @@ class TimedMutex { ...@@ -59,28 +60,25 @@ class TimedMutex {
void unlock(); void unlock();
private: private:
typedef boost::intrusive::list_member_hook<> MutexWaiterHookType; enum class LockResult { SUCCESS, TIMEOUT, STOLEN };
template <typename WaitFunc>
LockResult lockHelper(WaitFunc&& waitFunc);
// represents a waiter waiting for the lock. The waiter waits on the // represents a waiter waiting for the lock. The waiter waits on the
// baton until it is woken up by a post or timeout expires. // baton until it is woken up by a post or timeout expires.
struct MutexWaiter { struct MutexWaiter {
BatonType baton; Baton baton;
MutexWaiterHookType hook; folly::IntrusiveListHook hook;
}; };
typedef boost::intrusive:: using MutexWaiterList = folly::IntrusiveList<MutexWaiter, &MutexWaiter::hook>;
member_hook<MutexWaiter, MutexWaiterHookType, &MutexWaiter::hook>
MutexWaiterHook;
typedef boost::intrusive::list<
MutexWaiter,
MutexWaiterHook,
boost::intrusive::constant_time_size<true>>
MutexWaiterList;
pthread_spinlock_t lock_; //< lock to protect waiter list folly::SpinLock lock_; //< lock to protect waiter list
bool locked_ = false; //< is this locked by some thread? bool locked_ = false; //< is this locked by some thread?
MutexWaiterList waiters_; //< list of waiters MutexWaiterList threadWaiters_; //< list of waiters
MutexWaiterList fiberWaiters_; //< list of waiters
MutexWaiter* notifiedFiber_{nullptr}; //< Fiber waiter which has been notified
}; };
/** /**
......
...@@ -31,6 +31,7 @@ ...@@ -31,6 +31,7 @@
#include <folly/fibers/GenericBaton.h> #include <folly/fibers/GenericBaton.h>
#include <folly/fibers/Semaphore.h> #include <folly/fibers/Semaphore.h>
#include <folly/fibers/SimpleLoopController.h> #include <folly/fibers/SimpleLoopController.h>
#include <folly/fibers/TimedMutex.h>
#include <folly/fibers/WhenN.h> #include <folly/fibers/WhenN.h>
#include <folly/io/async/ScopedEventBaseThread.h> #include <folly/io/async/ScopedEventBaseThread.h>
#include <folly/portability/GTest.h> #include <folly/portability/GTest.h>
...@@ -2072,6 +2073,64 @@ TEST(FiberManager, VirtualEventBase) { ...@@ -2072,6 +2073,64 @@ TEST(FiberManager, VirtualEventBase) {
EXPECT_TRUE(done2); EXPECT_TRUE(done2);
} }
TEST(TimedMutex, ThreadFiberDeadlockOrder) {
folly::EventBase evb;
auto& fm = getFiberManager(evb);
TimedMutex mutex;
mutex.lock();
std::thread unlockThread([&] {
/* sleep override */ std::this_thread::sleep_for(
std::chrono::milliseconds{100});
mutex.unlock();
});
fm.addTask([&] { std::lock_guard<TimedMutex> lg(mutex); });
fm.addTask([&] {
runInMainContext([&] {
auto locked = mutex.timed_lock(std::chrono::seconds{1});
EXPECT_TRUE(locked);
if (locked) {
mutex.unlock();
}
});
});
evb.loopOnce();
EXPECT_EQ(0, fm.hasTasks());
unlockThread.join();
}
TEST(TimedMutex, ThreadFiberDeadlockRace) {
folly::EventBase evb;
auto& fm = getFiberManager(evb);
TimedMutex mutex;
mutex.lock();
fm.addTask([&] {
auto locked = mutex.timed_lock(std::chrono::seconds{1});
EXPECT_TRUE(locked);
if (locked) {
mutex.unlock();
}
});
fm.addTask([&] {
mutex.unlock();
runInMainContext([&] {
auto locked = mutex.timed_lock(std::chrono::seconds{1});
EXPECT_TRUE(locked);
if (locked) {
mutex.unlock();
}
});
});
evb.loopOnce();
EXPECT_EQ(0, fm.hasTasks());
}
/** /**
* Test that we can properly track fiber stack usage. * Test that we can properly track fiber stack usage.
* *
......
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