Handle clients going away without relinquishing the lock

[timgaleckas]

We had an issue where a process would get killed while still holding on to a lock. I've introduced the idea of a timeout where we can clean up locks from clients which haven't been heard from in a long time.

Let me know if you have any concerns.

Thanks,
Tim

[dv]

Thanks for the pull request tim, very interesting!

I noticed you removed this part:
@redis.getset(exists_name, 1)

which guarantees that the semaphore is created in Redis only once. Is there a reason you removed this?

I did notice you replaced it with a multi block that at first sight should guarantee the same atomicity, but I think in the following situation it would cause a problem:

• process A checks if semaphore exists (returns false)
• process B checks if semaphore exists (returns false)
• process A starts building the MULTI-block to generate the semaphore
• process B does the same
• process A EXEC's the transaction
• process A pops a token from the list and starts doing semaphore-allowed work
• process B EXEC's the transaction

Now there are N+1 tokens in the system, and as soon as process A finishes and puts the token back there will be N+1 tokens in the semaphore. Additionally, your timeout will not work for process A for that token since the list has been discarded. This would then actually solve the problem again if process A times out and never returns the token.

Since we're using Redis I feel we should use the powerful atomicity guarantee of getset() to avoid the previous situation. I don't think it interferes with your added feature.

Let me know what you think!

[marclennox]

What is the status of this pull request. I would really like to use redis-semaphore on my project but my last attempt was a bust due to the fact that killed workers were not releasing their semaphore locks which would lead to deadlocks.

I'd love to see an ability for a timeout to be provided on lock creation, such that they would be assumed dead after the timeout and removed.

[dv]

Since timgaleckas hasn't yet responded I'll soon go through the code myself and check if I can bring back the atomicity as described in my previous comment, and then merge his pull request with any potential changes I might need to make.

I can't promise you any timeframe though I'm thinking it'll be in a week or two.In the meantime you can use timgaleckas repository as the gem's source to work with his changes. Although there might be some risks involved regarding the integrity of the resources when multiple clients start the list at the same time using timgaleckas' code, I think the actual chance of it happening as I described are really, really minor.

[marclennox]

Thanks, I'll look forward to the official version and in the meantime use timgaleckas version.

[timgaleckas]

Hey dv,
You were completely right. I was trying to make the exists key do too much. (That's why I was using a list instead of a string and you can't getset a list) I cleaned that up and I found a better way to protect against different versions of the client running on the same semaphore (which was what I was trying to get to before)

Take a look.

And beware if you were using the previous code, you need to stop all your clients delete the exists key in redis and restart clients.

Tim

[timgaleckas]

It was my commit to open source day, so I did a bunch of stuff. This should also solve the issue: #8. I realized I changed the api quite a bit. I think it's nicer now, but I should, I wrote it. Feel free to chop this up however you wish or give me feedback on whatever you don't like.

[marclennox]

Still waiting for this to get accepted and merged. What's the status?