extent

[biancalana]

Shouldn't extent also check if the value of the key used as lock is the same provided before acquiring the lock ??

Hi,

Do you have a test case that demonstrates when this is a problem? I have a patch that Simon might like to merge, but I'd like to confirm with a specific test case first that it matters.

In theory though, I think you might be right, biancalana.

[biancalana]

Hi Ryan,

When there are more than one process trying to acquire the lock and extend param is specified, the tentatives will always succeed. Otherwise if the code would check if the value set on that key and only acquire the lock if the value match, the same way done in release lua code.

Sent from my iPhone

On 16/05/2015, at 13:22, Ryan Bastic notifications@github.com wrote:

Hi,

Do you have a test case that demonstrates when this is a problem? I have a patch that Simon might like to merge, but I'd like to confirm with a specific test case first that it matters.

In theory though, I think you might be right, biancalana.

—
Reply to this email directly or view it on GitHub.

(For context, I've spoken to Simon already about this issue now that you've reported it. I never implemented the extent stuff originally but I'm taking a look at it for him.)

Right, so - I modified t/008_extend.t to extend for 100 iterations instead of 3 and then did the following:

# redis-server

(opened a new Terminal)

# cd redis-distlock; PERL5LIB=lib TEST_SERVER=127.0.0.1:6379 perl t/008_extend.t

and then opening a new Terminal and running the same above command will result in failing to lock but succeeding in extending the lock.

Part of the problem here is we don't use the randomized SHA at all after the initial ->lock() call. We really need to retain it for if we extend the lock in the future, so that we can reuse the same SHA, as you mentioned. I'm digging into the implementation for the best way to solve this now.

(note: I also modified t/008_extend.t to not use ""THAT" but undef as value, so that it would generate the random SHA. It seems both instances exhibit the behavior you describe.

[biancalana]

Here is the code that i'm using:
#3

What about the code you're using to call Redis::DistLock?

After applying your patch to the latest trunk, I had to write the following: http://pastebin.com/apJiPQxk As you can see I have to check the newly returned lock value against the initial value that I got when I first locked.

This is not quite obvious at all by the API without playing around, so I think we should look at fixing up the test and improving the documentation as well.

Oh, also, you should basically always say something like: die if !$lock right after you call ->lock()..

Otherwise you could try to extend a lock that you didn't ever successfully acquire.

in other words:

my $foo = $rd->lock(...);
die if !$foo;

[biancalana]

Sorry my ignorance, but reading the doc on lock with extend parameter:

"Same as lock(), but given $extend is true it extends an existing lock or creates a new one instead of having to unlock first."

My understanding is that it should already extend an existing locks, the only issue is that without the patch the code always acquire the lock without check if the value is the same.

Looks more a bug to me than a change in the concept of this function call.

I'm using Redis::Distlock module to run a kind of election with distributed components, to choose who should be the leader and have access to special datastructures on Redis. Without the patch all the candidates always acquire the lock (thinking that there are the elected master) as soon they try, turning that in total anarchy... 😲

[sbertrang]

This reminds me of the reason for this - dying machines that would leave locks in place would prevent others from timely pick up work again. As I wanted to have manual control over this to speed things up in case of an emergency I came up with the extend idea.
So I would say the problem is that the documentation does not menton this properly as there is at least this one use-case? Maybe the API is just not good enough and we need to improve it?

[biancalana]

it has been committed on redlock-rb, what about release a new version of redis-distlock with the fix ?

[biancalana]

@rbastic @sbertrang Any news about this ?

[biancalana]

Hello @rbastic @sbertrang !

Kindly pinging you, can this be committed ?? Almost 1 year with this issue open....