Skip to content

Commit

Permalink
1. modify the interface of RedLock; 2. fix compiling errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@sewenew
sewenew committed Nov 11, 2019
1 parent 15e2926 commit 10ca2b2
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 56 deletions.
52 changes: 33 additions & 19 deletions src/sw/redis++/recipes/redlock.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,43 +45,57 @@ std::chrono::milliseconds RedMutex::try_lock(const std::string &val,
// Failed to lock more than half masters, or no time left for the lock.

This comment has been minimized.

Sign in to view

Copy link
@wingunder

wingunder Nov 13, 2019

Contributor

I don't know if the return value will ever be of use, in terms of knowing how long try_lockovershot the ttl, but if so the following might be useful. One use-case could probably be for testing the lib.
It also make it slightly easier to read the code.

if (!lock_ok) {
    // Failed to lock more than half masters.
    unlock(_resource);
    return std::chrono::milliseconds(-1);
}
else if (time_left < std::chrono::milliseconds(0)) {
    // Failed to lock as the time window for locking expired.
    unlock(_resource);
    return time_left;
}

This comment has been minimized.

Sign in to view

Copy link
@sewenew

sewenew Nov 13, 2019

Author Owner

That sounds good! I'll take it :)
unlock(_resource);

throw Error("failed to lock: " + _resource);
return std::chrono::milliseconds(-1);
}

return time_left;
}

bool RedMutex::try_lock(const std::string &val,
std::chrono::milliseconds RedMutex::try_lock(const std::string &val,
const std::chrono::time_point<std::chrono::system_clock> &tp) {
try {
try_lock(val, RedLockUtils::ttl(tp));
} catch (const Error &err) {
return false;
}

return true;
return try_lock(val, RedLockUtils::ttl(tp));
}

bool RedMutex::extend_lock(const std::string &val,
const std::chrono::time_point<std::chrono::system_clock> &tp) {
std::chrono::milliseconds RedMutex::extend_lock(const std::string &val,
const std::chrono::milliseconds &ttl) {
// TODO: this method is almost duplicate with `try_lock`. I'll refactor it soon.
auto start = std::chrono::steady_clock::now();

auto lock_cnt = 0U;
for (auto &master : _masters) {
if (_extend_lock_master(master.get(), val, tp)) {
if (_extend_lock_master(master.get(), val, ttl)) {
++lock_cnt;
}
}

return lock_cnt >= _quorum();
auto lock_ok = lock_cnt >= _quorum();

auto stop = std::chrono::steady_clock::now();
auto elapse = stop - start;

auto time_left = std::chrono::duration_cast<std::chrono::milliseconds>(ttl - elapse);

if (!lock_ok || time_left < std::chrono::milliseconds(0)) {
// Failed to lock more than half masters, or no time left for the lock.
unlock(_resource);

return std::chrono::milliseconds(-1);
}

return time_left;
}

std::chrono::milliseconds RedMutex::extend_lock(const std::string &val,
const std::chrono::time_point<std::chrono::system_clock> &tp) {
return extend_lock(val, RedLockUtils::ttl(tp));
}

bool RedMutex::_extend_lock_master(Redis &master,
const std::string &val,
const std::chrono::time_point<std::chrono::system_clock> &tp) {
const std::chrono::milliseconds &ttl) {
auto tx = master.transaction(true);
auto r = tx.redis();
try {
auto ttl = RedLockUtils::ttl(tp);

r.watch(_resource);

auto id = r.get(_resource);
Expand Down Expand Up @@ -119,7 +133,7 @@ void RedMutex::_unlock_master(Redis &master, const std::string &val) {
if (id && *id == val) {
auto reply = tx.del(_resource).exec();
if (reply.get<long long>(0) != 1) {
throw Error("this should not happen");
throw Error("Redis internal error: WATCH " + _resource + "failed");
}
}
} catch (const WatchError &err) {
Expand Down Expand Up @@ -170,15 +184,15 @@ std::string RedLockUtils::lock_id() {
} else if (idx < 10 + 26 + 26) {
id.push_back('A' + idx - 10 - 26);
} else {
assert(false);
assert(false && "Index out of range in RedLock::_lock_id()");
}
}

return id;
}

RedLockMutexVessel::RedLockMutexVessel(Redis& instance) :
RedLockMutexVessel({instance})
RedLockMutexVessel({{instance}})
{
}

Expand Down
90 changes: 57 additions & 33 deletions src/sw/redis++/recipes/redlock.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,13 @@ class RedMutex {
std::chrono::milliseconds try_lock(const std::string &val,
const std::chrono::milliseconds &ttl);

bool try_lock(const std::string &val,
std::chrono::milliseconds try_lock(const std::string &val,
const std::chrono::time_point<std::chrono::system_clock> &tp);

bool extend_lock(const std::string &val,
std::chrono::milliseconds extend_lock(const std::string &val,
const std::chrono::milliseconds &ttl);

std::chrono::milliseconds extend_lock(const std::string &val,
const std::chrono::time_point<std::chrono::system_clock> &tp);

void unlock(const std::string &val);
Expand All @@ -77,7 +80,7 @@ class RedMutex {

bool _extend_lock_master(Redis &master,
const std::string &val,
const std::chrono::time_point<std::chrono::system_clock> &tp);
const std::chrono::milliseconds &ttl);

std::size_t _quorum() const {
return _masters.size() / 2 + 1;
Expand All @@ -96,56 +99,76 @@ class RedLock {
RedLock(RedisInstance &mut, std::defer_lock_t) : _mut(mut), _lock_val(RedLockUtils::lock_id()) {}

~RedLock() {
if (_owned) {
if (owns_lock()) {
unlock();
}
}

// Try to acquire the lock for *ttl* milliseconds.
// Returns how much time still left for the lock, i.e. lock validity time.
std::chrono::milliseconds try_lock(const std::chrono::milliseconds &ttl) {
bool try_lock(const std::chrono::milliseconds &ttl) {
auto time_left = _mut.try_lock(_lock_val, ttl);
_owned = true;
return time_left;
if (time_left <= std::chrono::milliseconds(0)) {
return false;
}

_release_tp = std::chrono::steady_clock::now() + time_left;

return true;
}

// Try to acquire the lock, and hold until *tp*.
bool try_lock(const std::chrono::time_point<std::chrono::system_clock> &tp) {
if (_mut.try_lock(_lock_val, tp)) {
_owned = true;
}

return _owned;
return try_lock(RedLockUtils::ttl(tp));
}

// Try to extend the lock, and hold it until *tp*.
bool extend_lock(const std::chrono::time_point<std::chrono::system_clock> &tp) {
if (_mut.extend_lock(_lock_val, tp)) {
_owned = true;
} else {
_owned = false;
bool extend_lock(const std::chrono::milliseconds &ttl) {
// TODO: this method is almost duplicate with `try_lock`, and I'll refactor it soon.
auto time_left = _mut.extend_lock(_lock_val, ttl);
if (time_left <= std::chrono::milliseconds(0)) {

This comment has been minimized.

Sign in to view

Copy link
@wingunder

wingunder Nov 13, 2019

Contributor

Should it not be < instead of <=, in order to be consistent with the rest of the code?
eg.

if (time_left < std::chrono::milliseconds(0)) {
    return false;
}

This comment has been minimized.

Sign in to view

Copy link
@sewenew

sewenew Nov 13, 2019

Author Owner

Yes, I mixed < and <=. I'll make the rest of code to use <=, since if time_left is 0ms, it, in fact, doesn't acquired the lock.
return false;
}

return _owned;
_release_tp = std::chrono::steady_clock::now() + time_left;

return true;
}

bool extend_lock(const std::chrono::time_point<std::chrono::system_clock> &tp) {
return extend_lock(RedLockUtils::ttl(tp));
}

void unlock() {
try {
_mut.unlock(_lock_val);
_owned = false;
_release_tp = std::chrono::time_point<std::chrono::steady_clock>{};
} catch (const Error &err) {
_owned = false;
_release_tp = std::chrono::time_point<std::chrono::steady_clock>{};
throw;
}
}

private:
bool owns_lock() const {
if (ttl() <= std::chrono::milliseconds(0)) {

This comment has been minimized.

Sign in to view

Copy link
@wingunder

wingunder Nov 13, 2019

Contributor

Should it not be < instead of <=, in order to be consistent with the rest of the code?
eg.

if (ttl() < std::chrono::milliseconds(0)) {
    return false;
}
return false;
}

RedisInstance &_mut;
return true;
}

bool _owned = false;
std::chrono::milliseconds ttl() const {
auto t = std::chrono::steady_clock::now();
return std::chrono::duration_cast<std::chrono::milliseconds>(_release_tp - t);
}

private:
RedisInstance &_mut;

std::string _lock_val;

// The time point that we must release the lock.
std::chrono::time_point<std::chrono::steady_clock> _release_tp{};
};

class RedLockMutexVessel
Expand Down Expand Up @@ -241,33 +264,34 @@ class RedLockMutex
RedLockMutex(RedLockMutex &&) = delete;
RedLockMutex& operator=(RedLockMutex &&) = delete;

virtual ~RedLockMutex() = default;

std::chrono::milliseconds try_lock(const std::string& random_string,
const std::chrono::milliseconds& ttl)
{
const auto lock_info = _redlock_mutex.lock(_resource, random_string, ttl, 1);
if (!lock_info.locked) {
throw Error("failed to lock: " + _resource);
return std::chrono::milliseconds(-1);
}
return lock_info.time_remaining;
}

bool try_lock(const std::string &random_string,
std::chrono::milliseconds try_lock(const std::string &random_string,
const std::chrono::time_point<std::chrono::system_clock> &tp)
{
const auto lock_info = _redlock_mutex.lock(_resource, random_string, RedLockUtils::ttl(tp), 1);
return lock_info.locked;
return try_lock(random_string, RedLockUtils::ttl(tp));
}

bool extend_lock(const std::string &random_string,
const std::chrono::time_point<std::chrono::system_clock> &tp)
std::chrono::milliseconds extend_lock(const std::string &random_string,
const std::chrono::milliseconds &ttl)
{
const auto ttl = RedLockUtils::ttl(tp);
const RedLockMutexVessel::LockInfo lock_info =
{true, std::chrono::steady_clock::now(), ttl, _resource, random_string};
const auto result = _redlock_mutex.extend_lock(lock_info, ttl);
return result.locked;
return result.time_remaining;
}

std::chrono::milliseconds extend_lock(const std::string &random_string,
const std::chrono::time_point<std::chrono::system_clock> &tp) {
return extend_lock(random_string, RedLockUtils::ttl(tp));
}

void unlock(const std::string &random_string)
Expand Down
8 changes: 4 additions & 4 deletions test/src/sw/redis++/redlock_test.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,11 @@ class RandomBuffer : public RandomBufferInterface
#else // !USE_OPENSSL

template <int N = 20>
class RandomBuffer : public sw::redis::RandomBufferInterface
class RandomBuffer : public RandomBufferInterface
{
public:
std::string get_updated_string() {
return RedLockMutexVessel::lock_id();
return RedLockUtils::lock_id();
}
};

Expand Down Expand Up @@ -150,7 +150,7 @@ void RedLockTest<Redis>::run() {
for (int i=0; i<n; i++) {
mutex_list.push(new RedLockMutex(std::ref(_redis), RedLockUtils::lock_id()));
const std::chrono::time_point<std::chrono::system_clock> tp = std::chrono::system_clock::now() + multi_lock_ttl;
if (!(mutex_list.back()->try_lock(random_string, tp))) {
if (mutex_list.back()->try_lock(random_string, tp) < std::chrono::milliseconds(0)) {
std::cout << "Num locks = " << i << std::endl;;
REDIS_ASSERT(0, "unable to obtain a lock");
}
Expand All @@ -172,7 +172,7 @@ void RedLockTest<Redis>::run() {
for (int i=0; i<n; i++) {
const std::chrono::time_point<std::chrono::system_clock> tp = std::chrono::system_clock::now() + multi_lock_ttl;
mutex_list.push(new RedMutex(std::ref(_redis), RedLockUtils::lock_id()));
if (!(mutex_list.back()->try_lock(random_string, tp))) {
if (mutex_list.back()->try_lock(random_string, tp) < std::chrono::milliseconds(0)) {
REDIS_ASSERT(0, "unable to obtain a lock");
}
}
Expand Down

2 comments on commit 10ca2b2

@wingunder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sewenew,

It all looks ok to me. I however added a few minor comments above.

Another question: I saw that you hardly ever specify const auto, even though the variables could be const. Is there a reason for this?
eg.

redis-plus-plus/src/sw/redis++/recipes/redlock.cpp

Lines 35 to 42 in 10ca2b2

auto start = std::chrono::steady_clock::now();
auto lock_ok = _try_lock(val, ttl);
auto stop = std::chrono::steady_clock::now();
auto elapse = stop - start;
auto time_left = std::chrono::duration_cast<std::chrono::milliseconds>(ttl - elapse);

Regards

@sewenew
Copy link
Owner Author

@sewenew sewenew commented on 10ca2b2 Nov 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wingunder

First of all, you can write less code :) Secondly, you can check this discussion.

Normally, I don't make local variables const, unless it's compile time const:

const auto *script = "return 1;";

// Or even better:
constexpr auto *another_script = "return 2;";

Regards

Please sign in to comment.