Skip to content

Commit

Permalink
Create ReadHolder::unlock
Browse files Browse the repository at this point in the history
Summary:
Currently you need to depend on the destructor of `ReadHolder` (using closures as in code block #1 below or empty assignment as in code block #2 below) to ensure that a `ReadHolder` goes out of scope (and unlocks) in order to subsequently acquire a write lock via `WriteHolder` without deadlocking.
This diff introduces a way of unlocking a `ReadHolder` while it's still in scope such that a `WriteHolder` can be acquired. This makes the code more straight forward (reducing the risk of deadlock due to a programmer's misunderstanding of how `SharedMutex` / the holder framework works) => see code block # 3 below
Also add some documentation about why `WriteHolder::WriteHolder(ReadHolder&&)` doesn't exist

Code Block #1 : Use of closures
```
class foo {
 public:
  std::string getMemoizedData() {
    {
      folly::SharedMutex::ReadHolder readHolder(lock_);
      if (!data_.empty()) {
        // important to return by value, otherwise caller might access
        // data_ after we release the read lock
        return data_;
      }
    }
    {
      // try again with a write lock
      folly::SharedMutex::WriteHolder writeHolder(lock_);
      if (data_.empty()) {
        data_ = "my awesome string";
      }
      return data_;
    }
  }

 private:
  folly::SharedMutex lock_;
  std::string data_;
};
```

Code Block #2 : Use of empty assignment
```
class foo {
 public:
  std::string getMemoizedData() {
    folly::SharedMutex::ReadHolder readHolder(lock_);
    if (!data_.empty()) {
      // important to return by value, otherwise caller might access
      // data_ after we release the read lock
      return data_;
    }
    readHolder = {};

    // try again with a write lock
    folly::SharedMutex::WriteHolder writeHolder(lock_);
    if (data_.empty()) {
      data_ = "my awesome string";
    }
    return data_;
  }

 private:
  folly::SharedMutex lock_;
  std::string data_;
};
```

Code Block #3 : Use of unlock()
```
class foo {
 public:
  std::string getMemoizedData() {
    folly::SharedMutex::ReadHolder readHolder(lock_);
    if (!data_.empty()) {
      // important to return by value, otherwise caller might access
      // data_ after we release the read lock
      return data_;
    }
    readHolder->unlock();

    // try again with a write lock
    folly::SharedMutex::WriteHolder writeHolder(lock_);
    if (data_.empty()) {
      data_ = "my awesome string";
    }
    return data_;
  }

 private:
  folly::SharedMutex lock_;
  std::string data_;
};
```

Reviewed By: yfeldblum

Differential Revision: D3176025

fb-gh-sync-id: c7d47ca71df08673c7c1f1fd5ed9e01a663c1797
fbshipit-source-id: c7d47ca71df08673c7c1f1fd5ed9e01a663c1797
  • Loading branch information
abirchall authored and Facebook Github Bot 8 committed Apr 28, 2016
1 parent ab389fe commit 95e7a3b
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 0 deletions.
39 changes: 39 additions & 0 deletions folly/SharedMutex.h
Original file line number Diff line number Diff line change
Expand Up @@ -1283,8 +1283,13 @@ class SharedMutexImpl {
ReadHolder& operator=(const ReadHolder& rhs) = delete;

~ReadHolder() {
unlock();
}

void unlock() {
if (lock_) {
lock_->unlock_shared(token_);
lock_ = nullptr;
}
}

Expand Down Expand Up @@ -1325,8 +1330,13 @@ class SharedMutexImpl {
UpgradeHolder& operator=(const UpgradeHolder& rhs) = delete;

~UpgradeHolder() {
unlock();
}

void unlock() {
if (lock_) {
lock_->unlock_upgrade();
lock_ = nullptr;
}
}

Expand All @@ -1353,6 +1363,30 @@ class SharedMutexImpl {
lock_->unlock_upgrade_and_lock();
}

// README:
//
// It is intended that WriteHolder(ReadHolder&& rhs) do not exist.
//
// Shared locks (read) can not safely upgrade to unique locks (write).
// That upgrade path is a well-known recipe for deadlock, so we explicitly
// disallow it.
//
// If you need to do a conditional mutation, you have a few options:
// 1. Check the condition under a shared lock and release it.
// Then maybe check the condition again under a unique lock and maybe do
// the mutation.
// 2. Check the condition once under an upgradeable lock.
// Then maybe upgrade the lock to a unique lock and do the mutation.
// 3. Check the condition and maybe perform the mutation under a unique
// lock.
//
// Relevant upgradeable lock notes:
// * At most one upgradeable lock can be held at a time for a given shared
// mutex, just like a unique lock.
// * An upgradeable lock may be held concurrently with any number of shared
// locks.
// * An upgradeable lock may be upgraded atomically to a unique lock.

WriteHolder(WriteHolder&& rhs) noexcept : lock_(rhs.lock_) {
rhs.lock_ = nullptr;
}
Expand All @@ -1366,8 +1400,13 @@ class SharedMutexImpl {
WriteHolder& operator=(const WriteHolder& rhs) = delete;

~WriteHolder() {
unlock();
}

void unlock() {
if (lock_) {
lock_->unlock();
lock_ = nullptr;
}
}

Expand Down
20 changes: 20 additions & 0 deletions folly/test/SharedMutexTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,19 +92,39 @@ void runBasicHoldersTest() {
SharedMutexToken token;

{
// create an exclusive write lock via holder
typename Lock::WriteHolder holder(lock);
EXPECT_FALSE(lock.try_lock());
EXPECT_FALSE(lock.try_lock_shared(token));

// move ownership to another write holder via move constructor
typename Lock::WriteHolder holder2(std::move(holder));
EXPECT_FALSE(lock.try_lock());
EXPECT_FALSE(lock.try_lock_shared(token));

// move ownership to another write holder via assign operator
typename Lock::WriteHolder holder3;
holder3 = std::move(holder2);
EXPECT_FALSE(lock.try_lock());
EXPECT_FALSE(lock.try_lock_shared(token));

// downgrade from exclusive to upgrade lock via move constructor
typename Lock::UpgradeHolder holder4(std::move(holder3));

// ensure we can lock from a shared source
EXPECT_FALSE(lock.try_lock());
EXPECT_TRUE(lock.try_lock_shared(token));
lock.unlock_shared(token);

// promote from upgrade to exclusive lock via move constructor
typename Lock::WriteHolder holder5(std::move(holder4));
EXPECT_FALSE(lock.try_lock());
EXPECT_FALSE(lock.try_lock_shared(token));

// downgrade exclusive to shared lock via move constructor
typename Lock::ReadHolder holder6(std::move(holder5));

// ensure we can lock from another shared source
EXPECT_FALSE(lock.try_lock());
EXPECT_TRUE(lock.try_lock_shared(token));
lock.unlock_shared(token);
Expand Down

0 comments on commit 95e7a3b

Please sign in to comment.