A RedLock implementation using EVALSHA for locking.

[wingunder]

This commit is based on sewenew's recipes branch and implements and
tests the redlock algorithm.

This is a squash-commit that includes several suggestions from
sewenew.

This implementation was heavily based on the following implementation:
https://github.com/antirez/redlock-rb/blob/master/redlock.rb

[sewenew]

Hi @wingunder

Since your changes spans multiple commits, I just reviewed your final code. And I'll leave the review comments here:

• There's no need to have a RedMutexInterface (no need to pay for virtual pointer), instead, we can make RedLock to be a template. So that it can hold either RedMutex or RedLockMutex. That is what std::unique_lock does:

template <typename Mutex>
class RedLock {
public:
    RedLock(Mutex &mut, std::defer_lock_t);

private:
    Mutex _mut;
};

RedMutex m1;
RedLock<RedMutex> locker1(m1, std::defer_lock);
RedLockMutex m2;
RedLock<RedLockMutex> locker2(m2, std::defer_lock);

• The constructors of RedLockMutex should be explicit to avoid implicit conversion.

explicit RedLockMutex(Redis& instance);
explicit RedLockMutex(std::initializer_list<std::reference_wrapper<Redis>> instances);

• Try to avoid macros, e.g. DEFAULT_REDLOCK_RETRY_CNT. In fact, in the case of default parameter, hard-code is not a big deal :)
• There's no need to pass int and double by const reference, just pass them by value. Also I think in the case of int and double, pass
  by value should be faster than pass by reference.
• _unlockScript and _extendLockScript can be const.
• sha1 of RedLockMutex::_extend_lock_instance method is not thread-safe. In the catch clause, you might write the variable in mulitple threads. In fact, the both scripts are small, I think there's no need to use EVALSHA command. Since Redis has an internal cache for Lua script, it won't compile the script again and again. We use EVALSHA only for saving bandwidth. In our case, we might not benefit from it, and make the code logic complex. There won't be too much gain.
• Instead of usleep, you can try the C++ 11 feature: std::this_thread::sleep_for(std::microseconds(xxx)).
• One last thing: can you name these variable with the underscore_style, instead of the camelCaseStyle. So that the code style will be consistent with the original code.

Hope these helps :)

Regards

[wingunder]

Hi @sewenew,

Thanks for taking the time for the review. I really appreciate it.
Sorry for the many commits, but I'll make a squash-commit once you think it is ripe for merging.
You could also comment directly in here: https://github.com/sewenew/redis-plus-plus/pull/33/files
I made all your requested/suggested changes.

The RedLockKeyMutex and the RedMutex class can be used with the RedLock template. However the RedLockMutex class is not compatible, due to the fact that it works with the RedLockMutex::LockInfo struct.

Regards

[wingunder]

Hi @sewenew,

The RedLockMutex class is now usable with the RedLock template.

Regards

[sewenew]

Hi @wingunder

It's my bad! You are right, RedLockMutex is incompatible with RedLock. I used the wrong name.

I think it's good to keep both of your mutex implementations. So that end-user can have a choice on a simpler interface and a full-featured interface. In the future, if most users like the full-feature interface, I'll discard the simple one, and merge your interface into the master branch. However, RedLockKeyMutex is a little confusing, do you have a better name?

I think commit bf82419 is good enough, can you make a squash-commit based on that. And I'll merge it into the recipe branch. Also you can remove the virtual destructor, and rename the _key member to be _resource.

After merging your changes, I might modify some of the RedLock interfaces, to make it more like std::unique_lock.

Regards

[wingunder]

Hi @sewenew,

Sorry for the delay and thanks for your input.
I agree with you and made the changes, but I didn't squash it yet, as I am struggling to find a proper new name for RedLockKeyMutex .

Names that I was thinking of:

• RedLockMutexResource
• RedMutexFast (as it it almost 3 times faster than RedMutex, based on the test-cases.)

Do you like one of these, or do you have a better name for it?

Once I have a proper name, I'll refactor the class and squash-commit.
Regards

[sewenew]

Hi @wingunder ,

How about renaming RedLockMutex to FledgedRedLockMutex and renaming RedLockKeyMutex to RedLockMutex?

I'm not a native speaker, and FledgedRedLockMutex seems a little long. Hope you can find a shorter one :)

as it it almost 3 times faster than RedMutex, based on the test-cases.

Great work!

I think that because in order to unlock, RedMutex needs to send multiple commands to Redis, while RedLockMutex only needs to send one command.

Regards

[wingunder]

Hi @sewenew ,

Thanks for your suggestions.
I finally got to the following renames:

• RedLockMutex -> RedLockMutexVessel
• RedLockRedMutex -> RedLockMutex

I think that because in order to unlock, RedMutex needs to send multiple commands to Redis, while RedLockMutex only needs to send one command.

Yes, you are right. This is however only relevant if you have a use-case with several workers, all trying to obtain several locks. For the normal case, RedMutex is legitimate.

Please let me know if you need me to do some more work on this patch.

Regards

[sewenew]

Hi @wingunder ,

This patch looks good to me. I'll merge it into the recipes branch.

Thanks again for your great work!

Regards